Is this madness? The pursuit of single-statement methods
-
My code started out as something fairly easy to follow like this:
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e)
{
// Create a set to hold a collection of lines that have already been processed.
SortedSet seen = new SortedSet();
// Create a StringBuilder to avoid the performance penalty of concatenating strings repeatedly.
StringBuilder stringBuilder = new StringBuilder();
// Split the text from the text box by lines and examine each one.
foreach (string line in this.InputTextBox.Text.Split(new char[] { '\r', '\n' }))
{
// If the string is successfully added to the set, then it has not been processed before.
if (seen.Add(line))
{
// Add the line to the output.
stringBuilder.AppendLine(line);
}
}
// Build the string and assign it to the text box.
this.InputTextBox.Text = stringBuilder.ToString();
}Then I factored out some of the code into extension methods, removed the unnecessary curly braces, and got this:
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e)
{
// Create a StringBuilder to avoid the performance penalty of concatenating strings repeatedly.
StringBuilder stringBuilder = new StringBuilder();
// Split the text from the text box into a collection of unique lines.
foreach (string line in this.InputTextBox.Text.SplitLines().Unique())
// Add the line to the output.
stringBuilder.AppendLine(line);
// Build the string and assign it to the text box.
this.InputTextBox.Text = stringBuilder.ToString();
}And then I took it a step further.
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e) =>
// Take the text from the text box, split it by lines, remove the duplicates, and assign the results to the text box.
this.InputTextBox.Text = this.InputTextBox.Text.SplitLines().Unique().Aggregate(
new StringBuilder(),
(StringBuilder stringBuilder, string line) => stringBuilder.AppendLine(line),
(StringBuilder stringBuilder) => stringBuilder.ToString());And the one-liner:
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e) => this.InputTextBox.Text = this.InputTextBox.Text.SplitLines().Unique().Aggregate(new StringBuilder(), (sb, line) => sb.AppendLine(line), sb => sb.ToString());
I get great joy a
-
My code started out as something fairly easy to follow like this:
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e)
{
// Create a set to hold a collection of lines that have already been processed.
SortedSet seen = new SortedSet();
// Create a StringBuilder to avoid the performance penalty of concatenating strings repeatedly.
StringBuilder stringBuilder = new StringBuilder();
// Split the text from the text box by lines and examine each one.
foreach (string line in this.InputTextBox.Text.Split(new char[] { '\r', '\n' }))
{
// If the string is successfully added to the set, then it has not been processed before.
if (seen.Add(line))
{
// Add the line to the output.
stringBuilder.AppendLine(line);
}
}
// Build the string and assign it to the text box.
this.InputTextBox.Text = stringBuilder.ToString();
}Then I factored out some of the code into extension methods, removed the unnecessary curly braces, and got this:
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e)
{
// Create a StringBuilder to avoid the performance penalty of concatenating strings repeatedly.
StringBuilder stringBuilder = new StringBuilder();
// Split the text from the text box into a collection of unique lines.
foreach (string line in this.InputTextBox.Text.SplitLines().Unique())
// Add the line to the output.
stringBuilder.AppendLine(line);
// Build the string and assign it to the text box.
this.InputTextBox.Text = stringBuilder.ToString();
}And then I took it a step further.
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e) =>
// Take the text from the text box, split it by lines, remove the duplicates, and assign the results to the text box.
this.InputTextBox.Text = this.InputTextBox.Text.SplitLines().Unique().Aggregate(
new StringBuilder(),
(StringBuilder stringBuilder, string line) => stringBuilder.AppendLine(line),
(StringBuilder stringBuilder) => stringBuilder.ToString());And the one-liner:
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e) => this.InputTextBox.Text = this.InputTextBox.Text.SplitLines().Unique().Aggregate(new StringBuilder(), (sb, line) => sb.AppendLine(line), sb => sb.ToString());
I get great joy a
You shouldn't be doing that directly in the Click event handler. You need to refactor that functionality into a library of such routines. :jig:
-
My code started out as something fairly easy to follow like this:
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e)
{
// Create a set to hold a collection of lines that have already been processed.
SortedSet seen = new SortedSet();
// Create a StringBuilder to avoid the performance penalty of concatenating strings repeatedly.
StringBuilder stringBuilder = new StringBuilder();
// Split the text from the text box by lines and examine each one.
foreach (string line in this.InputTextBox.Text.Split(new char[] { '\r', '\n' }))
{
// If the string is successfully added to the set, then it has not been processed before.
if (seen.Add(line))
{
// Add the line to the output.
stringBuilder.AppendLine(line);
}
}
// Build the string and assign it to the text box.
this.InputTextBox.Text = stringBuilder.ToString();
}Then I factored out some of the code into extension methods, removed the unnecessary curly braces, and got this:
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e)
{
// Create a StringBuilder to avoid the performance penalty of concatenating strings repeatedly.
StringBuilder stringBuilder = new StringBuilder();
// Split the text from the text box into a collection of unique lines.
foreach (string line in this.InputTextBox.Text.SplitLines().Unique())
// Add the line to the output.
stringBuilder.AppendLine(line);
// Build the string and assign it to the text box.
this.InputTextBox.Text = stringBuilder.ToString();
}And then I took it a step further.
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e) =>
// Take the text from the text box, split it by lines, remove the duplicates, and assign the results to the text box.
this.InputTextBox.Text = this.InputTextBox.Text.SplitLines().Unique().Aggregate(
new StringBuilder(),
(StringBuilder stringBuilder, string line) => stringBuilder.AppendLine(line),
(StringBuilder stringBuilder) => stringBuilder.ToString());And the one-liner:
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e) => this.InputTextBox.Text = this.InputTextBox.Text.SplitLines().Unique().Aggregate(new StringBuilder(), (sb, line) => sb.AppendLine(line), sb => sb.ToString());
I get great joy a
The first method is a good example of "evil" comments - they just repeat the code in different words. :) Fighting Evil in Your Code: Comments on Comments - Simple Talk[^] I'd be inclined to move the final
Aggregateto its own extension method -JoinLinesperhaps? You can also simplify it by using String.Join[^]:public static string JoinLines(this IEnumerable<string> lines) => string.Join(Environment.NewLine, lines);
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e) => this.InputTextBox.Text = this.InputTextBox.Text.SplitLines().Unique().JoinLines();
"These people looked deep within my soul and assigned me a number based on the order in which I joined." - Homer
-
My code started out as something fairly easy to follow like this:
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e)
{
// Create a set to hold a collection of lines that have already been processed.
SortedSet seen = new SortedSet();
// Create a StringBuilder to avoid the performance penalty of concatenating strings repeatedly.
StringBuilder stringBuilder = new StringBuilder();
// Split the text from the text box by lines and examine each one.
foreach (string line in this.InputTextBox.Text.Split(new char[] { '\r', '\n' }))
{
// If the string is successfully added to the set, then it has not been processed before.
if (seen.Add(line))
{
// Add the line to the output.
stringBuilder.AppendLine(line);
}
}
// Build the string and assign it to the text box.
this.InputTextBox.Text = stringBuilder.ToString();
}Then I factored out some of the code into extension methods, removed the unnecessary curly braces, and got this:
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e)
{
// Create a StringBuilder to avoid the performance penalty of concatenating strings repeatedly.
StringBuilder stringBuilder = new StringBuilder();
// Split the text from the text box into a collection of unique lines.
foreach (string line in this.InputTextBox.Text.SplitLines().Unique())
// Add the line to the output.
stringBuilder.AppendLine(line);
// Build the string and assign it to the text box.
this.InputTextBox.Text = stringBuilder.ToString();
}And then I took it a step further.
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e) =>
// Take the text from the text box, split it by lines, remove the duplicates, and assign the results to the text box.
this.InputTextBox.Text = this.InputTextBox.Text.SplitLines().Unique().Aggregate(
new StringBuilder(),
(StringBuilder stringBuilder, string line) => stringBuilder.AppendLine(line),
(StringBuilder stringBuilder) => stringBuilder.ToString());And the one-liner:
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e) => this.InputTextBox.Text = this.InputTextBox.Text.SplitLines().Unique().Aggregate(new StringBuilder(), (sb, line) => sb.AppendLine(line), sb => sb.ToString());
I get great joy a
Kevin Li (Li, Ken-un) wrote:
but at which point should I have stopped for this example?
I think what you've done is fantastic. It's considerably more readable than the first implementation.
Kevin Li (Li, Ken-un) wrote:
Should I perhaps switch to another programming language that’s more conducive to this style of writing?
Your only other option to achieve this level of elegance (as far as I know and without dealing with arcane symbols) is F#, IMO. But I'm with Richard -- I think the Aggregate is unnecessary. Marc
Latest Article - Create a Dockerized Python Fiddle Web App Learning to code with python is like learning to swim with those little arm floaties. It gives you undeserved confidence and will eventually drown you. - DangerBunny Artificial intelligence is the only remedy for natural stupidity. - CDP1802
-
My code started out as something fairly easy to follow like this:
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e)
{
// Create a set to hold a collection of lines that have already been processed.
SortedSet seen = new SortedSet();
// Create a StringBuilder to avoid the performance penalty of concatenating strings repeatedly.
StringBuilder stringBuilder = new StringBuilder();
// Split the text from the text box by lines and examine each one.
foreach (string line in this.InputTextBox.Text.Split(new char[] { '\r', '\n' }))
{
// If the string is successfully added to the set, then it has not been processed before.
if (seen.Add(line))
{
// Add the line to the output.
stringBuilder.AppendLine(line);
}
}
// Build the string and assign it to the text box.
this.InputTextBox.Text = stringBuilder.ToString();
}Then I factored out some of the code into extension methods, removed the unnecessary curly braces, and got this:
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e)
{
// Create a StringBuilder to avoid the performance penalty of concatenating strings repeatedly.
StringBuilder stringBuilder = new StringBuilder();
// Split the text from the text box into a collection of unique lines.
foreach (string line in this.InputTextBox.Text.SplitLines().Unique())
// Add the line to the output.
stringBuilder.AppendLine(line);
// Build the string and assign it to the text box.
this.InputTextBox.Text = stringBuilder.ToString();
}And then I took it a step further.
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e) =>
// Take the text from the text box, split it by lines, remove the duplicates, and assign the results to the text box.
this.InputTextBox.Text = this.InputTextBox.Text.SplitLines().Unique().Aggregate(
new StringBuilder(),
(StringBuilder stringBuilder, string line) => stringBuilder.AppendLine(line),
(StringBuilder stringBuilder) => stringBuilder.ToString());And the one-liner:
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e) => this.InputTextBox.Text = this.InputTextBox.Text.SplitLines().Unique().Aggregate(new StringBuilder(), (sb, line) => sb.AppendLine(line), sb => sb.ToString());
I get great joy a
In Backus' paper introducing Fortran (1957), the primary focus was on the speed of writing code; a secondary focus was on debugging. Uncle Bob's most famous book "Clean Code" (2008) focuses on readability. Why do we see that paradigm shift? In 1957, most projects were green-field projects: for a problem, new code was written, the problem was solved and the code was no more needed. Old code need not be maintained. Nowadays, maintenance of code (brown-field projects) dominate the scene. We have to read code for more often than we write code. And now simply ask yourself: when someone else - or even you after a couple of weeks - reads that code, how long will it take him to understand it?
-
My code started out as something fairly easy to follow like this:
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e)
{
// Create a set to hold a collection of lines that have already been processed.
SortedSet seen = new SortedSet();
// Create a StringBuilder to avoid the performance penalty of concatenating strings repeatedly.
StringBuilder stringBuilder = new StringBuilder();
// Split the text from the text box by lines and examine each one.
foreach (string line in this.InputTextBox.Text.Split(new char[] { '\r', '\n' }))
{
// If the string is successfully added to the set, then it has not been processed before.
if (seen.Add(line))
{
// Add the line to the output.
stringBuilder.AppendLine(line);
}
}
// Build the string and assign it to the text box.
this.InputTextBox.Text = stringBuilder.ToString();
}Then I factored out some of the code into extension methods, removed the unnecessary curly braces, and got this:
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e)
{
// Create a StringBuilder to avoid the performance penalty of concatenating strings repeatedly.
StringBuilder stringBuilder = new StringBuilder();
// Split the text from the text box into a collection of unique lines.
foreach (string line in this.InputTextBox.Text.SplitLines().Unique())
// Add the line to the output.
stringBuilder.AppendLine(line);
// Build the string and assign it to the text box.
this.InputTextBox.Text = stringBuilder.ToString();
}And then I took it a step further.
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e) =>
// Take the text from the text box, split it by lines, remove the duplicates, and assign the results to the text box.
this.InputTextBox.Text = this.InputTextBox.Text.SplitLines().Unique().Aggregate(
new StringBuilder(),
(StringBuilder stringBuilder, string line) => stringBuilder.AppendLine(line),
(StringBuilder stringBuilder) => stringBuilder.ToString());And the one-liner:
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e) => this.InputTextBox.Text = this.InputTextBox.Text.SplitLines().Unique().Aggregate(new StringBuilder(), (sb, line) => sb.AppendLine(line), sb => sb.ToString());
I get great joy a
-
My code started out as something fairly easy to follow like this:
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e)
{
// Create a set to hold a collection of lines that have already been processed.
SortedSet seen = new SortedSet();
// Create a StringBuilder to avoid the performance penalty of concatenating strings repeatedly.
StringBuilder stringBuilder = new StringBuilder();
// Split the text from the text box by lines and examine each one.
foreach (string line in this.InputTextBox.Text.Split(new char[] { '\r', '\n' }))
{
// If the string is successfully added to the set, then it has not been processed before.
if (seen.Add(line))
{
// Add the line to the output.
stringBuilder.AppendLine(line);
}
}
// Build the string and assign it to the text box.
this.InputTextBox.Text = stringBuilder.ToString();
}Then I factored out some of the code into extension methods, removed the unnecessary curly braces, and got this:
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e)
{
// Create a StringBuilder to avoid the performance penalty of concatenating strings repeatedly.
StringBuilder stringBuilder = new StringBuilder();
// Split the text from the text box into a collection of unique lines.
foreach (string line in this.InputTextBox.Text.SplitLines().Unique())
// Add the line to the output.
stringBuilder.AppendLine(line);
// Build the string and assign it to the text box.
this.InputTextBox.Text = stringBuilder.ToString();
}And then I took it a step further.
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e) =>
// Take the text from the text box, split it by lines, remove the duplicates, and assign the results to the text box.
this.InputTextBox.Text = this.InputTextBox.Text.SplitLines().Unique().Aggregate(
new StringBuilder(),
(StringBuilder stringBuilder, string line) => stringBuilder.AppendLine(line),
(StringBuilder stringBuilder) => stringBuilder.ToString());And the one-liner:
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e) => this.InputTextBox.Text = this.InputTextBox.Text.SplitLines().Unique().Aggregate(new StringBuilder(), (sb, line) => sb.AppendLine(line), sb => sb.ToString());
I get great joy a
One minor point, you could avoid the
StringBuilderin your first example with the use ofstring.Join. I'm not saying you should, just that you could.SortedSet<string> seen = new SortedSet<string>();
foreach (string line in longString.Split('\r', '\n'))
{
seen.Add(line);
}
return string.Join(Environment.NewLine, seen);If you're interested, this returns this IL:
.maxstack 5 .locals init ( \[0\] class \[System\]System.Collections.Generic.SortedSet\`1<string> seen, \[1\] string\[\] strArray, \[2\] int32 num, \[3\] string line, \[4\] string str) L\_0000: nop L\_0001: newobj instance void \[System\]System.Collections.Generic.SortedSet\`1<string>::.ctor() L\_0006: stloc.0 L\_0007: nop L\_0008: ldarg.1 L\_0009: ldc.i4.2 L\_000a: newarr char L\_000f: dup L\_0010: ldc.i4.0 L\_0011: ldc.i4.s 13 L\_0013: stelem.i2 L\_0014: dup L\_0015: ldc.i4.1 L\_0016: ldc.i4.s 10 L\_0018: stelem.i2 L\_0019: callvirt instance string\[\] \[mscorlib\]System.String::Split(char\[\]) L\_001e: stloc.1 L\_001f: ldc.i4.0 L\_0020: stloc.2 L\_0021: br.s L\_0035 L\_0023: ldloc.1 L\_0024: ldloc.2 L\_0025: ldelem.ref L\_0026: stloc.3 L\_0027: nop L\_0028: ldloc.0 L\_0029: ldloc.3 L\_002a: callvirt instance bool \[System\]System.Collections.Generic.SortedSet\`1<string>::Add(!0) L\_002f: pop L\_0030: nop L\_0031: ldloc.2 L\_0032: ldc.i4.1 L\_0033: add L\_0034: stloc.2 L\_0035: ldloc.2 L\_0036: ldloc.1 L\_0037: ldlen L\_0038: conv.i4 L\_0039: blt.s L\_0023 L\_003b: call string \[mscorlib\]System.Environment::get\_NewLine() L\_0040: ldloc.0 L\_0041: call string \[mscorlib\]System.String::Join(string, class \[mscorlib\]System.Collections.Generic.IEnumerable\`1<string>) L\_0046: stloc.s str L\_0048: br.s L\_004a L\_004a: ldloc.s str L\_004c: retThis space for rent
-
My code started out as something fairly easy to follow like this:
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e)
{
// Create a set to hold a collection of lines that have already been processed.
SortedSet seen = new SortedSet();
// Create a StringBuilder to avoid the performance penalty of concatenating strings repeatedly.
StringBuilder stringBuilder = new StringBuilder();
// Split the text from the text box by lines and examine each one.
foreach (string line in this.InputTextBox.Text.Split(new char[] { '\r', '\n' }))
{
// If the string is successfully added to the set, then it has not been processed before.
if (seen.Add(line))
{
// Add the line to the output.
stringBuilder.AppendLine(line);
}
}
// Build the string and assign it to the text box.
this.InputTextBox.Text = stringBuilder.ToString();
}Then I factored out some of the code into extension methods, removed the unnecessary curly braces, and got this:
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e)
{
// Create a StringBuilder to avoid the performance penalty of concatenating strings repeatedly.
StringBuilder stringBuilder = new StringBuilder();
// Split the text from the text box into a collection of unique lines.
foreach (string line in this.InputTextBox.Text.SplitLines().Unique())
// Add the line to the output.
stringBuilder.AppendLine(line);
// Build the string and assign it to the text box.
this.InputTextBox.Text = stringBuilder.ToString();
}And then I took it a step further.
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e) =>
// Take the text from the text box, split it by lines, remove the duplicates, and assign the results to the text box.
this.InputTextBox.Text = this.InputTextBox.Text.SplitLines().Unique().Aggregate(
new StringBuilder(),
(StringBuilder stringBuilder, string line) => stringBuilder.AppendLine(line),
(StringBuilder stringBuilder) => stringBuilder.ToString());And the one-liner:
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e) => this.InputTextBox.Text = this.InputTextBox.Text.SplitLines().Unique().Aggregate(new StringBuilder(), (sb, line) => sb.AppendLine(line), sb => sb.ToString());
I get great joy a
Okay, I pondered and pondered and realised that you could refactor to this:
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e) => this.InputTextBox.Text = string.Join(Environment.NewLine, new SortedSet<string>(longString.Split('\r', '\n')));
It's as easy as that.
This space for rent
-
The first method is a good example of "evil" comments - they just repeat the code in different words. :) Fighting Evil in Your Code: Comments on Comments - Simple Talk[^] I'd be inclined to move the final
Aggregateto its own extension method -JoinLinesperhaps? You can also simplify it by using String.Join[^]:public static string JoinLines(this IEnumerable<string> lines) => string.Join(Environment.NewLine, lines);
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e) => this.InputTextBox.Text = this.InputTextBox.Text.SplitLines().Unique().JoinLines();
"These people looked deep within my soul and assigned me a number based on the order in which I joined." - Homer
Great minds think alike :)
This space for rent
-
My code started out as something fairly easy to follow like this:
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e)
{
// Create a set to hold a collection of lines that have already been processed.
SortedSet seen = new SortedSet();
// Create a StringBuilder to avoid the performance penalty of concatenating strings repeatedly.
StringBuilder stringBuilder = new StringBuilder();
// Split the text from the text box by lines and examine each one.
foreach (string line in this.InputTextBox.Text.Split(new char[] { '\r', '\n' }))
{
// If the string is successfully added to the set, then it has not been processed before.
if (seen.Add(line))
{
// Add the line to the output.
stringBuilder.AppendLine(line);
}
}
// Build the string and assign it to the text box.
this.InputTextBox.Text = stringBuilder.ToString();
}Then I factored out some of the code into extension methods, removed the unnecessary curly braces, and got this:
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e)
{
// Create a StringBuilder to avoid the performance penalty of concatenating strings repeatedly.
StringBuilder stringBuilder = new StringBuilder();
// Split the text from the text box into a collection of unique lines.
foreach (string line in this.InputTextBox.Text.SplitLines().Unique())
// Add the line to the output.
stringBuilder.AppendLine(line);
// Build the string and assign it to the text box.
this.InputTextBox.Text = stringBuilder.ToString();
}And then I took it a step further.
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e) =>
// Take the text from the text box, split it by lines, remove the duplicates, and assign the results to the text box.
this.InputTextBox.Text = this.InputTextBox.Text.SplitLines().Unique().Aggregate(
new StringBuilder(),
(StringBuilder stringBuilder, string line) => stringBuilder.AppendLine(line),
(StringBuilder stringBuilder) => stringBuilder.ToString());And the one-liner:
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e) => this.InputTextBox.Text = this.InputTextBox.Text.SplitLines().Unique().Aggregate(new StringBuilder(), (sb, line) => sb.AppendLine(line), sb => sb.ToString());
I get great joy a
I must be stupid, since I wind up having to "decompose" this stuff when the "intermediate" results and "lazy execution" starts yielding other than the results I expect (even if it was due to my own "mind fog"). Then what? Put it "back together" again for the next oaf? Perhaps "any idiot" can figured this one out, but at what point is it "too much"? And who says so? This is the opposite extreme of posters who have been chasticed for failing to cater to the lowest common denominator (when "they" used LINQ instead of something less "obtuse").
"(I) am amazed to see myself here rather than there ... now rather than then". ― Blaise Pascal
-
My code started out as something fairly easy to follow like this:
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e)
{
// Create a set to hold a collection of lines that have already been processed.
SortedSet seen = new SortedSet();
// Create a StringBuilder to avoid the performance penalty of concatenating strings repeatedly.
StringBuilder stringBuilder = new StringBuilder();
// Split the text from the text box by lines and examine each one.
foreach (string line in this.InputTextBox.Text.Split(new char[] { '\r', '\n' }))
{
// If the string is successfully added to the set, then it has not been processed before.
if (seen.Add(line))
{
// Add the line to the output.
stringBuilder.AppendLine(line);
}
}
// Build the string and assign it to the text box.
this.InputTextBox.Text = stringBuilder.ToString();
}Then I factored out some of the code into extension methods, removed the unnecessary curly braces, and got this:
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e)
{
// Create a StringBuilder to avoid the performance penalty of concatenating strings repeatedly.
StringBuilder stringBuilder = new StringBuilder();
// Split the text from the text box into a collection of unique lines.
foreach (string line in this.InputTextBox.Text.SplitLines().Unique())
// Add the line to the output.
stringBuilder.AppendLine(line);
// Build the string and assign it to the text box.
this.InputTextBox.Text = stringBuilder.ToString();
}And then I took it a step further.
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e) =>
// Take the text from the text box, split it by lines, remove the duplicates, and assign the results to the text box.
this.InputTextBox.Text = this.InputTextBox.Text.SplitLines().Unique().Aggregate(
new StringBuilder(),
(StringBuilder stringBuilder, string line) => stringBuilder.AppendLine(line),
(StringBuilder stringBuilder) => stringBuilder.ToString());And the one-liner:
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e) => this.InputTextBox.Text = this.InputTextBox.Text.SplitLines().Unique().Aggregate(new StringBuilder(), (sb, line) => sb.AppendLine(line), sb => sb.ToString());
I get great joy a
-
My code started out as something fairly easy to follow like this:
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e)
{
// Create a set to hold a collection of lines that have already been processed.
SortedSet seen = new SortedSet();
// Create a StringBuilder to avoid the performance penalty of concatenating strings repeatedly.
StringBuilder stringBuilder = new StringBuilder();
// Split the text from the text box by lines and examine each one.
foreach (string line in this.InputTextBox.Text.Split(new char[] { '\r', '\n' }))
{
// If the string is successfully added to the set, then it has not been processed before.
if (seen.Add(line))
{
// Add the line to the output.
stringBuilder.AppendLine(line);
}
}
// Build the string and assign it to the text box.
this.InputTextBox.Text = stringBuilder.ToString();
}Then I factored out some of the code into extension methods, removed the unnecessary curly braces, and got this:
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e)
{
// Create a StringBuilder to avoid the performance penalty of concatenating strings repeatedly.
StringBuilder stringBuilder = new StringBuilder();
// Split the text from the text box into a collection of unique lines.
foreach (string line in this.InputTextBox.Text.SplitLines().Unique())
// Add the line to the output.
stringBuilder.AppendLine(line);
// Build the string and assign it to the text box.
this.InputTextBox.Text = stringBuilder.ToString();
}And then I took it a step further.
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e) =>
// Take the text from the text box, split it by lines, remove the duplicates, and assign the results to the text box.
this.InputTextBox.Text = this.InputTextBox.Text.SplitLines().Unique().Aggregate(
new StringBuilder(),
(StringBuilder stringBuilder, string line) => stringBuilder.AppendLine(line),
(StringBuilder stringBuilder) => stringBuilder.ToString());And the one-liner:
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e) => this.InputTextBox.Text = this.InputTextBox.Text.SplitLines().Unique().Aggregate(new StringBuilder(), (sb, line) => sb.AppendLine(line), sb => sb.ToString());
I get great joy a
-
Okay, I pondered and pondered and realised that you could refactor to this:
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e) => this.InputTextBox.Text = string.Join(Environment.NewLine, new SortedSet<string>(longString.Split('\r', '\n')));
It's as easy as that.
This space for rent
Does the fact that you pondered so long not suggest you might be over-engineering it? There will probably be an equal amount of pondering to working out how it works. Each to their own, but I much prefer a few simple lines to one clever one. I suspect you've lost the preservation of order of the original implementation, and why are people using a SortedSet rather than a HashSet?
Regards, Rob Philpott.
-
My code started out as something fairly easy to follow like this:
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e)
{
// Create a set to hold a collection of lines that have already been processed.
SortedSet seen = new SortedSet();
// Create a StringBuilder to avoid the performance penalty of concatenating strings repeatedly.
StringBuilder stringBuilder = new StringBuilder();
// Split the text from the text box by lines and examine each one.
foreach (string line in this.InputTextBox.Text.Split(new char[] { '\r', '\n' }))
{
// If the string is successfully added to the set, then it has not been processed before.
if (seen.Add(line))
{
// Add the line to the output.
stringBuilder.AppendLine(line);
}
}
// Build the string and assign it to the text box.
this.InputTextBox.Text = stringBuilder.ToString();
}Then I factored out some of the code into extension methods, removed the unnecessary curly braces, and got this:
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e)
{
// Create a StringBuilder to avoid the performance penalty of concatenating strings repeatedly.
StringBuilder stringBuilder = new StringBuilder();
// Split the text from the text box into a collection of unique lines.
foreach (string line in this.InputTextBox.Text.SplitLines().Unique())
// Add the line to the output.
stringBuilder.AppendLine(line);
// Build the string and assign it to the text box.
this.InputTextBox.Text = stringBuilder.ToString();
}And then I took it a step further.
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e) =>
// Take the text from the text box, split it by lines, remove the duplicates, and assign the results to the text box.
this.InputTextBox.Text = this.InputTextBox.Text.SplitLines().Unique().Aggregate(
new StringBuilder(),
(StringBuilder stringBuilder, string line) => stringBuilder.AppendLine(line),
(StringBuilder stringBuilder) => stringBuilder.ToString());And the one-liner:
void RemoveDuplicatesButton_Click(object sender, RoutedEventArgs e) => this.InputTextBox.Text = this.InputTextBox.Text.SplitLines().Unique().Aggregate(new StringBuilder(), (sb, line) => sb.AppendLine(line), sb => sb.ToString());
I get great joy a
imho, you are doing a lot of extra work:
textBox1.Lines = textBox1.Lines.Distinct().OrderBy(str => str).ToArray();
However, if you anticipate that in the future your code will be read by people who are Linq-illiterate, then: whatever.
«Beauty is in the eye of the beholder, and it may be necessary from time to time to give a stupid or misinformed beholder a black eye.» Miss Piggy
-
Does the fact that you pondered so long not suggest you might be over-engineering it? There will probably be an equal amount of pondering to working out how it works. Each to their own, but I much prefer a few simple lines to one clever one. I suspect you've lost the preservation of order of the original implementation, and why are people using a SortedSet rather than a HashSet?
Regards, Rob Philpott.
You do realise that this was in response to the original poster don't you? I'm not a big fan of "clever code" so I wouldn't tend to write my code like this, this was just a way to show how this could have been done with the basics that were already present without relying on the extra scaffolding the OP put in. As for why a SortedSet, that's what the OP used so I have followed that; presumably he needs the output to be sorted, hence the SortedSet.
This space for rent
-
imho, you are doing a lot of extra work:
textBox1.Lines = textBox1.Lines.Distinct().OrderBy(str => str).ToArray();
However, if you anticipate that in the future your code will be read by people who are Linq-illiterate, then: whatever.
«Beauty is in the eye of the beholder, and it may be necessary from time to time to give a stupid or misinformed beholder a black eye.» Miss Piggy
Hi Bill, but ah.. You're ordering the strings alphabetically. Original implementation preserved the order just removing duplicates. Nice implementation though (TextBox.Lines not requiring line delimiting). If you drop the OrderBy(), I *think* there are no guarantees about preserving order. I have an inkling there's a select method which has a second 'index' parameter which you could order by.
Regards, Rob Philpott.
-
Hi Bill, but ah.. You're ordering the strings alphabetically. Original implementation preserved the order just removing duplicates. Nice implementation though (TextBox.Lines not requiring line delimiting). If you drop the OrderBy(), I *think* there are no guarantees about preserving order. I have an inkling there's a select method which has a second 'index' parameter which you could order by.
Regards, Rob Philpott.
Hi, Rob, I assumed, based on the OP's use of a SortedSet that sorting was required. If a more fancy sort is required, then, of course, you could write a custom sort function. My observation of the behavior of 'Distinct is that eliminates duplicates whose ordinal position is greater in the structure, but, there could well be dimensions of its behavior I am unaware of for other Types/Collections. cheers, Bill
«Beauty is in the eye of the beholder, and it may be necessary from time to time to give a stupid or misinformed beholder a black eye.» Miss Piggy
-
Hi Bill, but ah.. You're ordering the strings alphabetically. Original implementation preserved the order just removing duplicates. Nice implementation though (TextBox.Lines not requiring line delimiting). If you drop the OrderBy(), I *think* there are no guarantees about preserving order. I have an inkling there's a select method which has a second 'index' parameter which you could order by.
Regards, Rob Philpott.
Rob Philpott wrote:
If you drop the OrderBy(), I think there are no guarantees about preserving order.
The documentation[^] doesn't seem to explicitly mention it, other than saying it "returns an unordered sequence". But looking at the source code[^], the sequence returned from
Distinctwill be in the same order as the input sequence:static IEnumerable<TSource> DistinctIterator<TSource>(IEnumerable<TSource> source, IEqualityComparer<TSource> comparer) {
Set<TSource> set = new Set<TSource>(comparer);
foreach (TSource element in source)
if (set.Add(element)) yield return element;
}
"These people looked deep within my soul and assigned me a number based on the order in which I joined." - Homer
-
Rob Philpott wrote:
If you drop the OrderBy(), I think there are no guarantees about preserving order.
The documentation[^] doesn't seem to explicitly mention it, other than saying it "returns an unordered sequence". But looking at the source code[^], the sequence returned from
Distinctwill be in the same order as the input sequence:static IEnumerable<TSource> DistinctIterator<TSource>(IEnumerable<TSource> source, IEqualityComparer<TSource> comparer) {
Set<TSource> set = new Set<TSource>(comparer);
foreach (TSource element in source)
if (set.Add(element)) yield return element;
}
"These people looked deep within my soul and assigned me a number based on the order in which I joined." - Homer
I am relieved to know (for once) the source matches my observation of a very limited sample-set :) cheers, Bill
«Beauty is in the eye of the beholder, and it may be necessary from time to time to give a stupid or misinformed beholder a black eye.» Miss Piggy
-
Rob Philpott wrote:
If you drop the OrderBy(), I think there are no guarantees about preserving order.
The documentation[^] doesn't seem to explicitly mention it, other than saying it "returns an unordered sequence". But looking at the source code[^], the sequence returned from
Distinctwill be in the same order as the input sequence:static IEnumerable<TSource> DistinctIterator<TSource>(IEnumerable<TSource> source, IEqualityComparer<TSource> comparer) {
Set<TSource> set = new Set<TSource>(comparer);
foreach (TSource element in source)
if (set.Add(element)) yield return element;
}
"These people looked deep within my soul and assigned me a number based on the order in which I joined." - Homer
Yeah, it's tricky isn't it. Documentation doesn't seem to state that order is guaranteed not to change, but implementation indicates that's the case. I can't really imagine how you can improve much on that implementation either (which is strikingly similar to the initial posted implementation), so it's probably fair to assume no reordering will occur. But without that cast-iron guarantee, a future version of .NET could scupper things. Well hey, that's what consultancy rates are for. The bit which has me intrigued now is the Set class. Didn't know there was such a thing. HashSet would be my go to choice, so I presume its an internal-to-framework class.
Regards, Rob Philpott.
