Skip to main content
added 219 characters in body
Source Link

Starting with your specific questions:

  1. Yes, the class is idiomatically wrapped in a namespace. It's not very important for such a small-scale class as this, but that's the accepted way - the global namespace should only have namespaces in it.
  2. Neither the main class not method need to be public. They're called by the loader, not by other code.
  3. Assuming you're using VS2008 or later, you can use var to shorten the declaration syntax. The type is inferred from the right-hand side of the assignment. var counts = new SortedDictionary<string, int>();
  4. The more idiomatic way to do the while check would be to remove it from the condition and simply call it twice, once outside the loop and once inside. There's no modern C#ish replacement. Your way is fine too.
  5. TryGetValue or ContainsKey/Get combination are still the way to go, but can be easily wrapped in an Extension Method, which can then be used cleanly: counts[word] = counts.GetOrDefault(word, 0) + 1;
  6. It's a simple, clear loop. No real need to improve it. If you want, you can use string.Join to create one big string and print it out, but there's no real gain.
  7. What would you be using LINQ for here? LINQ is for querying, and you're not doing too much of that here. You could write a method using iterators that returns the next line from the console as an IEnumerable<string>, and call LINQ functions on it, but it won't be clearer or better performing.

And additionally 8. One thing I've developed an allergy for over the years is calls to ToUpper/ToLower. In 90% of cases, it's done just to do a case insensitive comparison, which is usually a bad idea - it creates new copies of the string, which means memory and GC pressure. If you don't care about representation, just write a case insensitive regex and keep the original strings intact: Regex.Matches(line, wordRegex, RegexOptions.IgnoreCase);

  1. One thing I've developed an allergy for over the years is calls to ToUpper/ToLower. In 90% of cases, it's done just to do a case insensitive comparison, which is usually a bad idea - it creates new copies of the string, which means memory and GC pressure. If you don't care about representation, just write a case insensitive regex and keep the original strings intact: Regex.Matches(line, wordRegex, RegexOptions.IgnoreCase); Also, make sure your dictionary is case-insensitive as well, as @t3chb0t reminds me in the comments: SortedDictionary<string, int> counts = new SortedDictionary<string, int>(StringComparer.CurrentCultureIgnoreCase);

  2. I'm not sure a a SortedDictionary makes sense here. It has a significantly slower Insert performance than a regular dictionary, and it's not as if you need to access it sorted during insertion. I would create a regular Dictionary<string,int>, and after processing is done, sort it once.

Starting with your specific questions:

  1. Yes, the class is idiomatically wrapped in a namespace. It's not very important for such a small-scale class as this, but that's the accepted way - the global namespace should only have namespaces in it.
  2. Neither the main class not method need to be public. They're called by the loader, not by other code.
  3. Assuming you're using VS2008 or later, you can use var to shorten the declaration syntax. The type is inferred from the right-hand side of the assignment. var counts = new SortedDictionary<string, int>();
  4. The more idiomatic way to do the while check would be to remove it from the condition and simply call it twice, once outside the loop and once inside. There's no modern C#ish replacement. Your way is fine too.
  5. TryGetValue or ContainsKey/Get combination are still the way to go, but can be easily wrapped in an Extension Method, which can then be used cleanly: counts[word] = counts.GetOrDefault(word, 0) + 1;
  6. It's a simple, clear loop. No real need to improve it. If you want, you can use string.Join to create one big string and print it out, but there's no real gain.
  7. What would you be using LINQ for here? LINQ is for querying, and you're not doing too much of that here. You could write a method using iterators that returns the next line from the console as an IEnumerable<string>, and call LINQ functions on it, but it won't be clearer or better performing.

And additionally 8. One thing I've developed an allergy for over the years is calls to ToUpper/ToLower. In 90% of cases, it's done just to do a case insensitive comparison, which is usually a bad idea - it creates new copies of the string, which means memory and GC pressure. If you don't care about representation, just write a case insensitive regex and keep the original strings intact: Regex.Matches(line, wordRegex, RegexOptions.IgnoreCase);

Starting with your specific questions:

  1. Yes, the class is idiomatically wrapped in a namespace. It's not very important for such a small-scale class as this, but that's the accepted way - the global namespace should only have namespaces in it.
  2. Neither the main class not method need to be public. They're called by the loader, not by other code.
  3. Assuming you're using VS2008 or later, you can use var to shorten the declaration syntax. The type is inferred from the right-hand side of the assignment. var counts = new SortedDictionary<string, int>();
  4. The more idiomatic way to do the while check would be to remove it from the condition and simply call it twice, once outside the loop and once inside. There's no modern C#ish replacement. Your way is fine too.
  5. TryGetValue or ContainsKey/Get combination are still the way to go, but can be easily wrapped in an Extension Method, which can then be used cleanly: counts[word] = counts.GetOrDefault(word, 0) + 1;
  6. It's a simple, clear loop. No real need to improve it. If you want, you can use string.Join to create one big string and print it out, but there's no real gain.
  7. What would you be using LINQ for here? LINQ is for querying, and you're not doing too much of that here. You could write a method using iterators that returns the next line from the console as an IEnumerable<string>, and call LINQ functions on it, but it won't be clearer or better performing.

And additionally:

  1. One thing I've developed an allergy for over the years is calls to ToUpper/ToLower. In 90% of cases, it's done just to do a case insensitive comparison, which is usually a bad idea - it creates new copies of the string, which means memory and GC pressure. If you don't care about representation, just write a case insensitive regex and keep the original strings intact: Regex.Matches(line, wordRegex, RegexOptions.IgnoreCase); Also, make sure your dictionary is case-insensitive as well, as @t3chb0t reminds me in the comments: SortedDictionary<string, int> counts = new SortedDictionary<string, int>(StringComparer.CurrentCultureIgnoreCase);

  2. I'm not sure a a SortedDictionary makes sense here. It has a significantly slower Insert performance than a regular dictionary, and it's not as if you need to access it sorted during insertion. I would create a regular Dictionary<string,int>, and after processing is done, sort it once.

added 62 characters in body
Source Link

Starting with your specific questions:

  1. Yes, the class is idiomatically wrapped in a namespace. It's not very important for such a small-scale class as this, but that's the accepted way - the global namespace should only have namespaces in it.
  2. Neither the main class not method need to be public. They're called by the loader, not by other code.
  3. Assuming you're using VS2008 or later, you can use var to shorten the declaration syntax. The type is inferred from the right-hand side of the assignment. var counts = new SortedDictionary<string, int>();
  4. The more idiomatic way to do the while check would be to remove it from the condition and simply call it twice, once outside the loop and once inside. There's no modern C#ish replacement. Your way is fine too.
  5. TryGetValue or ContainsKey/Get combination are still the way to go, but can be easily wrapped in an Extension Method, which can then be used cleanly: counts[word] = counts.GetOrDefault(word, 0) + 1;
  6. It's a simple, clear loop. No real need to improve it. If you want, you can use string.Join to create one big string and print it out, but there's no real gain.
  7. What would you be using LINQ for here? LINQ is for querying, and you're not doing too much of that here. You could write a method using iterators that returns the next line from the console as an IEnumerable<string>, and call LINQ functions on it, but it won't be clearer or better performing.

And additionally 8. One thing I've developed an allergy for over the years is calls to ToUpper/ToLower. In 90% of cases, it's done just to do a case insensitive comparison, which is usually a bad idea - it creates new copies of the string, which means memory and GC pressure. If you don't care about representation, just write a case insensitive regex and keep the original strings intact: Regex.Matches(line, wordRegex, RegexOptions.IgnoreCase);

Starting with your specific questions:

  1. Yes, the class is idiomatically wrapped in a namespace. It's not very important for such a small-scale class as this, but that's the accepted way - the global namespace should only have namespaces in it.
  2. Neither the main class not method need to be public. They're called by the loader, not by other code.
  3. Assuming you're using VS2008 or later, you can use var to shorten the declaration syntax. The type is inferred from the right-hand side of the assignment. var counts = new SortedDictionary<string, int>();
  4. The more idiomatic way to do the while check would be to remove it from the condition and simply call it twice, once outside the loop and once inside. There's no modern C#ish replacement. Your way is fine too.
  5. TryGetValue or ContainsKey/Get combination are still the way to go, but can be easily wrapped in an Extension Method, which can then be used cleanly: counts[word] = counts.GetOrDefault(word, 0) + 1;
  6. It's a simple, clear loop. No real need to improve it. If you want, you can use string.Join to create one big string and print it out, but there's no real gain.
  7. What would you be using LINQ for here? LINQ is for querying, and you're not doing too much of that here. You could write a method using iterators that returns the next line from the console as an IEnumerable<string>, and call LINQ functions on it, but it won't be clearer or better performing.

And additionally 8. One thing I've developed an allergy for over the years is calls to ToUpper/ToLower. In 90% of cases, it's done just to do a case insensitive comparison, which is usually a bad idea - it creates new copies of the string, which means memory and GC pressure. If you don't care about representation, just write a case insensitive regex and keep the original strings intact.

Starting with your specific questions:

  1. Yes, the class is idiomatically wrapped in a namespace. It's not very important for such a small-scale class as this, but that's the accepted way - the global namespace should only have namespaces in it.
  2. Neither the main class not method need to be public. They're called by the loader, not by other code.
  3. Assuming you're using VS2008 or later, you can use var to shorten the declaration syntax. The type is inferred from the right-hand side of the assignment. var counts = new SortedDictionary<string, int>();
  4. The more idiomatic way to do the while check would be to remove it from the condition and simply call it twice, once outside the loop and once inside. There's no modern C#ish replacement. Your way is fine too.
  5. TryGetValue or ContainsKey/Get combination are still the way to go, but can be easily wrapped in an Extension Method, which can then be used cleanly: counts[word] = counts.GetOrDefault(word, 0) + 1;
  6. It's a simple, clear loop. No real need to improve it. If you want, you can use string.Join to create one big string and print it out, but there's no real gain.
  7. What would you be using LINQ for here? LINQ is for querying, and you're not doing too much of that here. You could write a method using iterators that returns the next line from the console as an IEnumerable<string>, and call LINQ functions on it, but it won't be clearer or better performing.

And additionally 8. One thing I've developed an allergy for over the years is calls to ToUpper/ToLower. In 90% of cases, it's done just to do a case insensitive comparison, which is usually a bad idea - it creates new copies of the string, which means memory and GC pressure. If you don't care about representation, just write a case insensitive regex and keep the original strings intact: Regex.Matches(line, wordRegex, RegexOptions.IgnoreCase);

Source Link

Starting with your specific questions:

  1. Yes, the class is idiomatically wrapped in a namespace. It's not very important for such a small-scale class as this, but that's the accepted way - the global namespace should only have namespaces in it.
  2. Neither the main class not method need to be public. They're called by the loader, not by other code.
  3. Assuming you're using VS2008 or later, you can use var to shorten the declaration syntax. The type is inferred from the right-hand side of the assignment. var counts = new SortedDictionary<string, int>();
  4. The more idiomatic way to do the while check would be to remove it from the condition and simply call it twice, once outside the loop and once inside. There's no modern C#ish replacement. Your way is fine too.
  5. TryGetValue or ContainsKey/Get combination are still the way to go, but can be easily wrapped in an Extension Method, which can then be used cleanly: counts[word] = counts.GetOrDefault(word, 0) + 1;
  6. It's a simple, clear loop. No real need to improve it. If you want, you can use string.Join to create one big string and print it out, but there's no real gain.
  7. What would you be using LINQ for here? LINQ is for querying, and you're not doing too much of that here. You could write a method using iterators that returns the next line from the console as an IEnumerable<string>, and call LINQ functions on it, but it won't be clearer or better performing.

And additionally 8. One thing I've developed an allergy for over the years is calls to ToUpper/ToLower. In 90% of cases, it's done just to do a case insensitive comparison, which is usually a bad idea - it creates new copies of the string, which means memory and GC pressure. If you don't care about representation, just write a case insensitive regex and keep the original strings intact.