patterncsharpMinor
Finding all pairs of numbers in a collection whose sum is some particular number
Viewed 0 times
sumnumberwhoseallnumberscollectionsomefindingparticularpairs
Problem
I had to write the method that extract all pairs of numbers from collection of numbers, which sum matches the particular number.
I think my algorithm should be more productive, but not sure in which way. Any suggestion and/or code will be helpful.
static void Main(string[] args)
{
var inputCollection = new List { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 };
var targetNumber = 12;
var pairs = ExtractPairs.Extract(inputCollection, targetNumber);
foreach (var pair in pairs)
{
Console.WriteLine(pair.First + ", " + pair.Second);
}
}
public class ExtractPairs
{
public struct Pair
{
public int First;
public int Second;
}
public static List Extract(List input, int targetNumber)
{
var pairs = new List();
var current = input[0];
do
{
for (int i = current; i < input.Count; i++)
{
if (current + input[i] != targetNumber) continue;
var pair = new Pair
{
First = current,
Second = input[i]
};
pairs.Add(pair);
}
current++;
} while (current < input.Count);
return pairs;
}
}I think my algorithm should be more productive, but not sure in which way. Any suggestion and/or code will be helpful.
Solution
You haven't clarified what you're expecting your input to be but your code doesn't work unless the input is a set of positive integers starting at 1 and increasing 1 at a time.
For example:
Clearly, 5 + 7 = 12 but your code returns no pairs! That's either a huge bug or the algorithm is expecting a very restricted input. I'd put my money on a huge bug.
Don't make structs with public mutable fields (unless you're in a very specific performance critical scenario). As pointed out in comments, I should have backed up this advice with some evidence. Here's an SO question about it or simply quoting the MS guidelines for struct design
X DO NOT define mutable value types.
Mutable value types have several problems. For example, when a property getter returns a value type, the caller receives a copy. Because the copy is created implicitly, developers might not be aware that they are mutating the copy, and not the original value. Also, some languages (dynamic languages, in particular) have problems using mutable value types because even local variables, when dereferenced, cause a copy to be made.
Make them
Prefer formatting strings over concatenation:
could be:
In C#6 we also have string intperpolation:
In this simple case, the concatenation is actually fine. It's clear because it's short. As things become longer and more complex, the format string approach can make the final string clearer to see for a human. There are also a great many options for things like padding and formatting numbers and dates that are easily done in the format string.
Just an FYI, the concatenation is turned into a call to
For example:
var inputCollection = new List { 5, 6, 7 };
var targetNumber = 12;
var pairs = ExtractPairs.Extract(inputCollection, targetNumber);Clearly, 5 + 7 = 12 but your code returns no pairs! That's either a huge bug or the algorithm is expecting a very restricted input. I'd put my money on a huge bug.
Don't make structs with public mutable fields (unless you're in a very specific performance critical scenario). As pointed out in comments, I should have backed up this advice with some evidence. Here's an SO question about it or simply quoting the MS guidelines for struct design
X DO NOT define mutable value types.
Mutable value types have several problems. For example, when a property getter returns a value type, the caller receives a copy. Because the copy is created implicitly, developers might not be aware that they are mutating the copy, and not the original value. Also, some languages (dynamic languages, in particular) have problems using mutable value types because even local variables, when dereferenced, cause a copy to be made.
public struct Pair
{
public int First;
public int Second;
}Make them
readonly properties and add a constructor. I'd probably just use a Tuple though.Prefer formatting strings over concatenation:
Console.WriteLine(pair.First + ", " + pair.Second);could be:
Console.WriteLine("{0}, {1}", pair.First, pair.Second);In C#6 we also have string intperpolation:
Console.WriteLine($"{pair.First}, {pair.Second}");In this simple case, the concatenation is actually fine. It's clear because it's short. As things become longer and more complex, the format string approach can make the final string clearer to see for a human. There are also a great many options for things like padding and formatting numbers and dates that are easily done in the format string.
Just an FYI, the concatenation is turned into a call to
string.Concat:Console.WriteLine(string.Concat(new[] { pair.First, ", ", pair.Second }));Code Snippets
var inputCollection = new List<int> { 5, 6, 7 };
var targetNumber = 12;
var pairs = ExtractPairs.Extract(inputCollection, targetNumber);public struct Pair
{
public int First;
public int Second;
}Console.WriteLine(pair.First + ", " + pair.Second);Console.WriteLine("{0}, {1}", pair.First, pair.Second);Console.WriteLine($"{pair.First}, {pair.Second}");Context
StackExchange Code Review Q#123428, answer score: 7
Revisions (0)
No revisions yet.