Read characters slowly (Threading with delay, .NET Framework 2.0)

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.T

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(IEnumerable 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);
    }
}

StackExchange Code Review Q#158604, answer score: 4