7
\$\begingroup\$

Looking for some feedback on the Two Sum LeetCode problem. Looking for feedback on code style in general, use of var, variable naming and initialization, return placement, and any other feedback or optimizations I could make.

Problem

Given an array of integers, return indices of the two numbers such that they add up to a specific target.

You may assume that each input would have exactly one solution, and you may not use the same element twice.

Example:
Given nums = [2, 7, 11, 15], target = 9,

Because nums[0] + nums[1] = 2 + 7 = 9,
return [0, 1].

My solution

using System.Collections.Generic;

public class Solution {
    public int[] TwoSum(int[] nums, int target) {
        var numsDictionary = new Dictionary<int, int>();

        int complement = 0;
        for(int i=0; i < nums.Count(); i++) 
        {            
            complement = target - nums[i];
            int index = 0;
            if(numsDictionary.TryGetValue(complement, out index))
            {
                int[] twoSumSolution = {index, i};
                return twoSumSolution;
            }                                                         

            if(!numsDictionary.ContainsKey(nums[i]))
            {
                numsDictionary.Add(nums[i], i);
            }            
        }

        return null;
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Was it specified how you needed to solve this? You're solving it by looking for the complement you know you need; but these types of challenges often entail iteratively (brute force) checking all elements of a list; as the iterative logic is often the point of such a challenge. \$\endgroup\$ – Flater Mar 20 '18 at 10:01
  • \$\begingroup\$ it was not, one sample solution did you use brute and another used an approach similar to one pass dictionary / map \$\endgroup\$ – greg Mar 20 '18 at 17:20
10
\$\begingroup\$
  • Most C# developers will place opening braces on a new line. If you do this as well then you make it easier to read the code by others.

  • complement and index should use var "type" as well

  • twoSumSolution isn't really needed and is only adding noise to the code.

  • I like to use if (bool == false) instead of if (!bool) because I can grasp it at first glance without wondering wether I see a exclamation sign or not.

  • Instead of using the Count() extension method I prefer using the Length property for arrays. Using Count() will envolve a soft cast to ICollection<T> and a null check which just isn't needed.

  • checking if complement > 0 will remove unneeded calls to TryGetValue(), but has implications on certain inputs like target = 0 and an array [-3, 7, 3].

Implementing the mentioned points will lead to

public int[] TwoSum(int[] nums, int target)
{
    var numsDictionary = new Dictionary<int, int>();

    var complement = 0;
    for (var i = 0; i < nums.Length; i++)
    {
        complement = target - nums[i];
        var index = 0;
        if (complement > 0 && numsDictionary.TryGetValue(complement, out index))
        {
            return new int[] { index, i };
        }

        if (numsDictionary.ContainsKey(nums[i]) == false)
        {
            numsDictionary.Add(nums[i], i);
        }
    }

    return null;
}
\$\endgroup\$
  • \$\begingroup\$ all great comments, thanks for the feedback. great use of var, much easier to focus in on the output of the function int[]. \$\endgroup\$ – greg Mar 20 '18 at 7:13
  • \$\begingroup\$ Two nitpicks: (1) var isn’t a type. (2) I think your first advice is bad: if this is indeed the general C# style (never heard that, but haven’t used C# much in the last years), then the consensus is simply wrong. It’s objectively less readable because it wastes vertical space on syntactic noise. :-( Similar for your fourth advice: boolean literals have no place outside of initialisations. \$\endgroup\$ – Konrad Rudolph Mar 20 '18 at 14:25
  • \$\begingroup\$ @KonradRudolph: var is an implicit strongly type specification - resolved at compile time. About your argue about brackets, I thing you're up against a whole community. I agree with your last point though ;-) \$\endgroup\$ – Henrik Hansen Mar 20 '18 at 14:39
  • 1
    \$\begingroup\$ @KonradRudolph about boolean, as long as your eyes are good there isn't a problem using ! but if they get worse you will be glad to see false instead. \$\endgroup\$ – Heslacher Mar 20 '18 at 14:42
  • \$\begingroup\$ @Heslacher Regarding var: I know what is. But it isn’t a type, contrary to what your answer says. Regarding the bool: So put a space between the operator and the rest of the expression. Personally I prefer languages that spell the operator out for readability (not instead of !) but since this is C#… \$\endgroup\$ – Konrad Rudolph Mar 20 '18 at 15:16
5
\$\begingroup\$

Some minor points:

1) You don't really need the complement variable, so just do the math in the call to TryGetValue().

2) If int[] nums are all positive you may continue the loop if nums[i] > target.

3) It is stated that there is only one solution per input, so you can skip the check if (!numsDictionary.ContainsKey(nums[i])). This will never be true for the nums that add up to the target. As for the rest of nums it doesn't matter.


All in all it could be reduced to this (for C# 7.0):

public int[] TwoSumReview(int[] nums, int target)
{
  Dictionary<int, int> numsDictionary = new Dictionary<int, int>();

  for (int i = 0; i < nums.Length; i++)
  {
    int num = nums[i];

    // You can uncomment this if nums are all positive
    //if (num > target) { continue; }

    if (numsDictionary.TryGetValue(target - num, out int index))
    {          
      return new [] { index, i }; 
    }

    numsDictionary[num] = i;
  }

  return null;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Good catch about ContainsKey() \$\endgroup\$ – Heslacher Mar 20 '18 at 8:41
1
\$\begingroup\$

Dictionary can be initialized to an initial capacity
No purpose to index = 0

public static int[] TwoSum(int[] nums, int target)
{
    int numsLength = nums.Length;
    if(numsLength <= 1)
    {
        return null;
    }
    Dictionary<int, int> numsDictionary = new Dictionary<int, int>(numsLength);
    int complement;
    int index;
    int num;
    numsDictionary.Add(nums[0], 0);
    for (int i = 1; i < numsLength; i++)
    {
        num = nums[i];
        complement = target - num;
        if (numsDictionary.TryGetValue(complement, out index))
        {
            return new int[] { index, i };
        }
        if (!numsDictionary.ContainsKey(num))
        {
            numsDictionary.Add(num, i);
        }
    }
    return null;
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.