Ytdyklly

Can somebody review the code and provide suggestions on where I can improve on the code like use of better data structure etc.

Input: {1,7,2,8,3,6}, 9

Output: [0, 3] [1, 2] [4, 5]

public class TwoSumExactAll {

    public static void main(String args) {

        int nums1 = {1,7,2,8,3,6};

        for (List<Integer> list : twoSumExactWithList(nums1,9)) {

            System.out.println(list);

        }

    }



    public static ArrayList<List<Integer>> twoSumExactWithList(int nums,int target) {

        HashMap<Integer,Integer> map = new HashMap<Integer, Integer>();

        ArrayList<List<Integer>> outerList = new ArrayList<List<Integer>>();

        ArrayList<Integer> innerList=null;

        int complement;

        for(int i=0;i<nums.length;i++) {

            complement = target-nums[i];

            if(map.containsKey(complement)) {

                innerList = new ArrayList<Integer>();

                innerList.add(map.get(complement));

                innerList.add(i);

                outerList.add(innerList);

            }else {

                map.put(nums[i], i);

            }

        }

        return outerList;

    }

}

I guess that the relevant piece of code is the twoSumExactWithList method and for now I will ignore the rest...

I think that depending on the context, e.g. the principles used in the project that you intent to use, this code you could either fully embrace collections or stick to arrays... but your code does a combo of both where the arguments are naked arrays and the returns uses collections... so please choose between the two.

If you choose arrays a possible return type is int where one of the two dimensions is 2. representing first vs second index of the pair. In practice int[2] is potentially more efficient than int[2] but perhaps the Jit may make it irrelevant. Perhaps you could have a IntPair where IntPair is a pair of ints struct-like class.

If you choose collections... then the arguments should be List<Integer> nums, int target. The result is kind-of fine... but don't commit to return an ArrayList but rather a List; don't give any details on the implementation that are not necessary. To encode the index pair, consider the use of a custom class IntPair again... List is correct but seems a bit overkill considering that we know is always two and exactly two elements.

As for the method body... The varible types should be a general as they can be... so map's type should be Map<Integer, Integer> rather than HashMap<...>, outerList's should be List rather than ArrayList and so forth...

I would declare complement within the loop as it is not used outside of it and is rewritten at every iteration. innerList would go within the if within the for.

Whenever possible I would declare variables as final to give an extra hint to the compiler/optimizer that indeed these are not going to be changed... eg. final int nums, final int target, final Map<Integer, Integer> map, final int complement.

I would rename map to something a bit more informative as to what it contains...
so perhaps indexByNumValue?

I think your code would read and look much better with some additional spaces e.g.:

List<Integer> innerList=null; to List<Integer> innerList = null;.

for(int i=0;i<nums.length;i++) { to for (int i = 0; i < nums.length; i++) {.

}else { to } else {.

if( to if (.

nums,int to nums, int.

etc...