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
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.
-
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.
Rob Philpott wrote:
The bit which has me intrigued now is the Set<t> 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.
Yes, it's an internal class within the
Enumerableclass: Enumerable.Set<TElement>[^] I'd have used aHashSet<T>as well, but I guess MS were probably writing theDistinctmethod before theHashSetclass was finished. Either that, or there were some very specific performance issues they were trying to work around. But if that was the case, I'd have expected to see a comment explaining the problem they were trying to solve.
"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
Interesting!! I have never seen a method, like your RemoveDuplicatesButton_Click() use the => syntax at this point?! That actually works?
Ben Scharbach Temporalwars.Com YouTube:Ben Scharbach
