3
\$\begingroup\$

Couple of months ago I was asked to develop a console app to do the following:

Write an application that takes an array of ICharacterReader interfaces, accesses them in parallel, and outputs a list of word frequencies ordered by word count and then alphabetically, every 10 seconds.

For example, if the stream returns: "It was the best of times, it was the worst of times" then the output will be:

it - 2
of - 2
the – 2
times - 2
was - 2
best - 1
worst – 1

The sample project was targeting .NET Framework 2.0 so I had no access to Linq, Task, async-await, etc. And ICharacterReader interface and SlowCharacterReader class had to be kept as is.

The thing is, I got no feedback related with the code quality afterwards. And going through the archive, I wonder what could have been improved.

Can you review the code and let me know in which ways I could possibly improve it?

using System;
using System.IO;
using System.Collections.Generic;
using System.Text;
using System.Threading;

namespace SlowCharacterReader
{
    public interface ICharacterReader : IDisposable
    {
        char GetNextChar();
    }

    public class SlowCharacterReader : ICharacterReader
    {

        private int m_Pos = 0;
        private string m_Content = @"  Alice was beginning to get very tired of sitting by her sister
on the bank, and of having nothing to do:  once or twice she had
peeped into the book her sister was reading, but it had no
pictures or conversations in it, 'and what is the use of a book,'
thought Alice `without pictures or conversation?'

  So she was considering in her own mind (as well as she could,
for the hot day made her feel very sleepy and stupid), whether
the pleasure of making a daisy-chain would be worth the trouble
of getting up and picking the daisies, when suddenly a White
Rabbit with pink eyes ran close by her.";

        Random m_Rnd = new Random();

        public char GetNextChar()
        {
            System.Threading.Thread.Sleep(m_Rnd.Next(200));

            if (m_Pos >= m_Content.Length)
            {
                throw new System.IO.EndOfStreamException();
            }

            return m_Content[m_Pos++];

        }

        public void Dispose()
        {
            //do nothing
        }

    }


    ////Actual code below
    public class Word
    {
        public string Content { get; set; }
        public int Frequency { get; set; }
    }
    public class WordComparer : IComparer<Word>
    {
        //Implemented comparer as Linq methods are unavailable
        //i.e. Enumerable and OrderBy / ThenBy methods
        public int Compare(Word x, Word y)
        {
            //Descending
            int xdiff = y.Frequency.CompareTo(x.Frequency);
            if (xdiff != 0)
                return xdiff;
            else
                //Ascending
                return x.Content.CompareTo(y.Content);
        }
    }

    public class Parallel
    {

        private static List<Word> words = new List<Word>();

        public static void WorkerMethodAsync(ICharacterReader reader)
        {

            StringBuilder stringBuilder = new StringBuilder();
            try
            {
                for (;;)
                {

                    var c = reader.GetNextChar();

                    //Assumed that input is UNICODE and any char which is not a letter is a word splitter
                    //i.e. \t \r \n , - etc.
                    if (Char.IsLetter(c))
                    {
                        stringBuilder.Append(c);
                    }
                    else
                    {
                        Word word = new Word
                        {
                            Content = stringBuilder.ToString().ToLowerInvariant(),
                            Frequency = 1
                        };
                        if (word.Content != string.Empty)
                        {
                            int wordPos = words.FindIndex(w => w.Content == word.Content);

                            lock (words)
                            {
                                if (wordPos < 0)
                                    words.Add(word);
                                else
                                    words[wordPos].Frequency = words[wordPos].Frequency + 1;

                                words.Sort(new WordComparer());
                            }

                        }
                        //StringBuilder.Clear() unavailable
                        stringBuilder.Length = 0;
                    }


                }
            }
            catch (EndOfStreamException ex)
            {
                //I would prefer changing GetNextChar() in a way that 
                //it wouldn't throw EndOfStreamException but a preset 
                //EOF char/sequence to be checked in try block
            }
        }
        public static void ForEach<T>(IEnumerable<T> readers) where T : ICharacterReader
        {
            //Print output every 10 seconds
            Timer t = new Timer(Output, null, 0, 10000);

            foreach (var reader in readers)
            {

                Thread th = new Thread(() => WorkerMethodAsync(reader));
                th.Start();
                th.Join();
            };

            //Stop the timer and print final output
            t.Dispose();
            Output(null);
            Console.WriteLine("Press any key to exit...");
        }
        private static void Output(object state)
        {
            Console.Clear();

            List<Word> tempList = words;

            lock (tempList)
            {
                foreach (Word word in tempList)
                    Console.WriteLine("{0} - {1}", word.Content, word.Frequency);
            }
        }

    }

    public class Program
    {
        public static void Main(string[] args)
        {
            //Sample input
            ICharacterReader[] readers ={
                                            new SlowCharacterReader(),
                                            new SlowCharacterReader(),
                                            new SlowCharacterReader(),
                                            new SlowCharacterReader(),
                                       };


            //Main call
            //Implemented own Parallel.ForEach method
            Parallel.ForEach(readers);

            foreach (ICharacterReader reader in readers)
                reader.Dispose();

            Console.ReadKey();
        }


    }

}
\$\endgroup\$
4
\$\begingroup\$

We can do a check for '\0' on GetNextChar() that will allow us to get rid of the exception

SlowCharacterReader

//...
public char GetNextChar()
{
    System.Threading.Thread.Sleep(m_Rnd.Next(200));
    if (m_Pos >= m_Content.Length)
    {
        return '\0';
    }
    return m_Content[m_Pos++];
}

Parallel

//...
char c;
while ((c = reader.GetNextChar()) != '\0')
{

Object creation
As is, we end up creating a lot of word objects that we do not use (any which already exist in the list). We can defer creating them until we need them

var wordContent = stringBuilder.ToString().ToLowerInvariant();
if (!string.IsNullOrWhiteSpace(wordContent))  
{
    lock (words)
    {
        var theWord = words.Find(w => w.Content == wordContent);

        if (theWord == null)
            words.Add(new Word(wordContent));
        else
            theWord.IncrementCount();
    }
}

We can also tweak Word a bit to emphasise its intended usage.

public class Word
{
    public Word(string content)
    {
        Content = content;
        Frequency = 1;
    }

    public string Content { get; private set;}
    public int Frequency { get; private set;}

    public void IncrementCount()
    {
        Frequency++;
    }
}

I moved finding the word inside the lock because it looks like there is a race condition here.

  • Process 1 Finds the index for a Word.
  • Process 2 Looks for a word, finds that it isn't there, adds a new word
  • Process 1 Updates the wrong word

It looks like we can remove the sort after each addition. It might make the Find a bit faster but given our delays on the input and 10 second timer do we really care?

The timer in ForEach() can be put in a using block and there is no real need to have a dummy parameter for Output(), we can uses a lambda to convert the signature. Also we can pass the words list into the worker. No strong reason other than a personal dislike of reaching out of the method.

public void ForEach<T>(IEnumerable<T> readers) where T : ICharacterReader
{
    //Print output every 10 seconds
    using (var t = new Timer(obj => Output(), null, 0, 10000))
    {
        foreach (var reader in readers)
        {
           Thread th = new Thread(() => WorkerMethodAsync(reader, words));
           th.Start();
           th.Join();
        };
    }

    Output();
    _writer.WriteLine("Press any key to exit...");
}

In Output(), I might be missing something but the tempList seems superfluous. It's not a copy of the original list, it is another reference to it. Locking it should lock the original list (it better as updates to the original list will mess up our foreach). Does tempList perform a function? (always nice to learn something when answering a question :) )

private void Output()
{
    _writer.Clear();

    lock (words)
    {
        words.Sort(Comparer);
        foreach (Word word in words)
           Console.WriteLine("{0} - {1}", word.Content, word.Frequency);
    }
}
\$\endgroup\$
  • \$\begingroup\$ GetNextChar() was not to be modified as per requirement but I'm totally against using exceptions in flow of the code so I commented on that part. I have totally missed that unused Word objects and that could have been a good addition. And now that you point out, it should not have been a reference but new list so that locking it wouldn't affect other threads modifying the actual list. Thanks a lot for your time. \$\endgroup\$ – uTeisT Mar 23 '17 at 14:00

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.