12
\$\begingroup\$

I've been doing a task for an interview that I will have soon. The requirement is to code the following problem in C#.

Write a program (and prove that it works) that: Given a text file, count the occurrence of each unique word in the file. For example; a file containing the string “Go do that thing that you do so well” should find these counts: 1: Go 2: do 2: that 1: thing 1: you 1: so 1: well

I coded my solution and it's working fine for the tests I gave.

I'm looking for any ways that I can improve my code and to get feedback on the solution. Is there's any way I could to the problem more efficiently.

Thank you in advance.

using System;
using System.Collections.Generic;
using System.Text.RegularExpressions;

namespace CountOccurrence
{
    class Program
    {
        static void Main(string[] args)
        {
            string text = System.IO.File.ReadAllText(@"F:\Ex\Myfile.txt");              // User ReadAllText to copy the file's text into a string
            string textToLower = text.ToLower();                                        // Converts the string to lower case string
            Regex reg_exp = new Regex("[^a-z0-9]");                                     // Regular expressions to replace non-letter and non-number characters with spaces. It uses the pattern [^a-z0-9].The a-z0-9 part means any lowercase letter or a digit.
            textToLower = reg_exp.Replace(textToLower, " ");                            // The code uses the Regex object’s Replace method to replace the characters that match the pattern with a space character. 
            string[] Value = textToLower.Split(new char[] {' '},
            StringSplitOptions.RemoveEmptyEntries);                                     // Split the string and remove the empty entries

            Dictionary<string, int> CountTheOccurrences = new Dictionary<string, int>(); // Create a dictionary to keep track of each occurrence of the words in the string
            for (int i = 0; i < Value.Length; i++)                                       // Loop the splited string  
            {
                if (CountTheOccurrences.ContainsKey(Value[i]))                           // Check if word is already in dictionary update the count  
                {
                    int value = CountTheOccurrences[Value[i]];
                    CountTheOccurrences[Value[i]] = value + 1;
                }
                else                                                                     // If we found the same word we just increase the count in the dictionary 
                {
                    CountTheOccurrences.Add(Value[i], 1);  
                }
            }

            Console.WriteLine("The number of counts for each words are:");
            foreach (KeyValuePair<string, int> kvp in CountTheOccurrences)
            {
                Console.WriteLine("Counts: " + kvp.Value + " for " + kvp.Key);           // Print the number of counts for each word
            }
            Console.ReadKey();
        }
    }
}
\$\endgroup\$
16
\$\begingroup\$

Efficiency

I won't say much about efficiency - because without a clear use-case it will be hard to know whether possible changes would be worth the effort - but my main concern would be fact that you force a whole file into a string, of which you immediately produce a second copy. It would be nice to see a version which takes a stream of some description rather than a whole file, as this could (in theory) cope with very large files (ones which can't fit in memory), could have much better memory characteristics, and with a bit of effort could start processing the file before it has read the whole thing, so that you are not stalled waiting for the whole file before you can begin (though an asynchronous implementation would be necessary to fully exploit such possibilities).

API

Don't hard-code the input file-name. Put this code in a nicely packaged method, and take the input as a parameter. This parameter could be a file-name, but it could also be a Stream or String (if you intend to always read the whole thing at the start) or whatever; you can always provide a convenience method to cover important use-cases.

And don't print the output to the console: if the calling code wants to print the counts to the console, let it do that, but give it the information it needs to do what it wants, rather than deciding what to do with the information for it. Returning the dictionary (perhaps as an IDictionary, so that you aren't coupled to the particular class) produces a much more useful interface.

IDictionary<string, int> CountWordOccurances(string text);

If your specification says that you must be printing these counds to the console, then you can write a method to print out the dictionary (perhaps to an arbitrary TextWriter, rather than only Console.WriteLine, which is no fun to test against), and write another method which composes the two.

Comments

Comments should be useful, explaining why code is doing what it is doing or providing some important context. // Converts the string to lower case string says nothing which text.Tolower() doesn't already, and is liable to rot as the method is modified. The code uses the Regex object’s Replace method to replace the characters that match the pattern with a space character. is far too wordy, and just states what the code is doing, without any indication as to why it might be doing that. We can see that it uses a Regex object, and we can see that that is uses the replace method, and we can see that it replaces matches with a " ": none of this needs clarifying.

Variables

I'm not terribly fond of your variable naming. They don't escape the method, so it doesn't really matter what style you use (though everyone uses lowerCamelCase, see the Microsoft Naming Guidelines), but you must be consistent. Value -> value (or values, since it is a collection). CountTheOccurances is not a great name; counts or wordCounts would scan much better. reg_exp encodes no information beyond that which is clear from the type. Something like letterFilter might be better.

I'd be inclined to ditch textToLower, and just replace text: it's so easy to use the old variable accidentally when they have such similar names. If you wanted the separation to be clear, you could put the reading of the text and to-lowering in a different scope (or even method), so that only textToLower appears for the rest of the method; however, you've already confused matters by re-using textToLower as textToLowerAfterRegex.

Splitting

Your regex bit doesn't really make sense; you are replacing every character that doesn't map to a lower-case latin letter or arabic numeral with a space: where is the specification which tells you what counts as a word or not? I'll leave someone who knows more about unicode to comment on how you should do this sort of thing properly, but your code is deficient, not least because it will cut "naïve" into "na" and "ve".

You can use Regex.Split instead of performing a replacement and then splitting, and use something like \p{L} to cover a wider variety of letters (just for example). A LINQ where can then be used to filter out empty entries.

Regex nonLetters = new Regex(@"[^\p{L}]");
return nonLetters.Split(text).Where(s => s.Length > 0);

A more efficient alternative might be to use a regex which matches words, and return captures instead of splitting; however, again, if performance is a concern, then you need to benchmark under realistic conditions.

Regex wordMatcher = new Regex(@"\p{L}+");
return wordMatcher.Matches(text).Select(c => c.Value);

I'd be strongly inclined to separate this 'extracting words' bit from the 'counting words' bit, and to avoid ToLowering at this point. Indeed, rather than using ToLower() to group words, consider supplying the dictionary a case-insensitive string comparer such as StringComparer.CurrentCultureIgnoreCase.

Counting

Though there is merit in using a separate ContainsKey call and [key] lookup, it's more efficient and a bit tidier to use TryGetValue. Here are 2 obvious ways of using it.

if (CountTheOccurrences.TryGetValue(value[i], out int count))
{
    CountTheOccurrences[value[i]] = count + 1;
}
else
{
    CountTheOccurrences.Add(value[i], 1);
}

// or

int count;
if (!CountTheOccurrences.TryGetValue(value[i], out count))
{
    Count = 0;
}
CountTheOccurrences[value[i]] = count + 1;

Personally I prefer the second one (count doesn't leak if not-found), but the first is closer to what you already have.

Another option is to ditch the loop completely, and use a LINQ GroupBy call, reducing the code complexity alot. In the code below, I write a custom general-purpose CountOccurances method, which could be reused for other purposes, and makes the intention of the code plain without compromising on performance (GroupBy would introduce significant overheads).

Example Rewrite

The below incorporates most of the ideas above, with a couple of other adjustments. The separation of concerns is maybe a little excessive, but while the number of lines of code seems to have increased, essentially all the complexity is hidden away in the completely generic (and potentially widely reusable (I wish LINQ had it already)) CountOccurrences method; the other methods are trivial, but none-the-less encapsulate domain information behind a nice API.

/// <summary>
/// Convenience method which prints the number of occurrences of each word in the given file
/// </summary>
public static void PrintWordCountsInFile(string fileName)
{
    var text = System.IO.File.ReadAllText(fileName);
    var words = SplitWords(text);
    var counts = CountWordOccurrences(words);
    WriteWordCounts(counts, System.Console.Out);
}

/// <summary>
/// Splits the given text into individual words, stripping punctuation
/// A word is defined by the regex @"\p{L}+"
/// </summary>
public static IEnumerable<string> SplitWords(string text)
{
    Regex wordMatcher = new Regex(@"\p{L}+");
    return wordMatcher.Matches(text).Select(c => c.Value);
}

/// <summary>
/// Counts the number of occurrences of each word in the given enumerable
/// </summary>
public static IDictionary<string, int> CountWordOccurrences(IEnumerable<string> words)
{
    return CountOccurrences(words, StringComparer.CurrentCultureIgnoreCase);
}

/// <summary>
/// Prints word-counts to the given TextWriter
/// </summary>
public static void WriteWordCounts(IDictionary<string, int> counts, TextWriter writer)
{
    writer.WriteLine("The number of counts for each words are:");
    foreach (KeyValuePair<string, int> kvp in counts)
    {
        writer.WriteLine("Counts: " + kvp.Value + " for " + kvp.Key.ToLower()); // print word in lower-case for consistency
    }
}

/// <summary>
/// Counts the number of occurrences of each distinct item
/// </summary>
public static IDictionary<T, int> CountOccurrences<T>(IEnumerable<T> items, IEqualityComparer<T> comparer)
{
    var counts = new Dictionary<T, int>(comparer);

    foreach (T t in items)
    {
        int count;
        if (!counts.TryGetValue(t, out count))
        {
            count = 0;
        }
        counts[t] = count + 1;
    }

    return counts;
}

Note that every method has inline-documentation, which describes its job (though I'll grant the summaries are not very good; with a proper spec be able to writer better APIs with better documentation).

\$\endgroup\$
  • 5
    \$\begingroup\$ First I want to thank you very much for taking the time to explain to me and write me the feedback. I can see clearly now what I was doing wrong and I gained a lot by reading carefully through your explanation. I must say that I'm at the beginning of my programming career and I have still a lot to learn and improve. But I will try my best to improve my code every time. Thanks again! \$\endgroup\$ – andreiy05 Mar 23 at 13:46
7
\$\begingroup\$

Just a few things to add to VisualMelons answer:

Instead of a for loop, you can use a foreach loop:

  foreach (string word in Value)
  {
    ...

Using Dictionary<string,int>.TryGetValue:

The out int count value defaults to 0 if the word is not present in the dictionary, so it's valid to write:

  foreach (string word in Value)
  {
    CountTheOccurrences.TryGetValue(word, out int count);
    CountTheOccurrences[word] = count + 1;
  }

You don't have to check the returned value from TryGetValue() and you can add a new entry via the indexer.


You have to consider what a word is:

The English term: "doesn't", how would you count that: as one word or as "doesn" + "t" (or even "does" + "not")? I'm not expert in English, but I would consider it to be one word, because neither "doesn" nor "t" can be counted for as words in grammatical sense. (The word counter in Microsoft Word counts "doesn't" and other contractions as one word). You can take that into account by extending VisualMelons regex pattern a little bit:

public static IEnumerable<string> SplitWords(string text)
{
    Regex wordMatcher = new Regex(@"[\p{L}']+");
    return wordMatcher.Matches(text).Cast<Match>().Select(c => c.Value);
}

Here the pattern is extended with [] and a '.

Alternatively you can use the \b anchor, which matches boundaries between alpha-numeric and non-alpha-numeric chars:

@"\b[\w']+\b"
\$\endgroup\$
  • 1
    \$\begingroup\$ Wow, I don't think I've never noticed possible usage of TryGetValue... that or I just didn't like it (I prefer not to depend on that sort of detail) +1 as usual ;) \$\endgroup\$ – VisualMelon Mar 23 at 21:07

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.