Clean Code

I've been reading Clean Code lately and have been finding it a bit inspiring.  It's interesting in that it's a fairly quick read, but at the same time it's taking me a while to get through it.  This is primarily because each new subject prompts me to think about my own work and my own development practices.  Many times I'll learn something new (well, not new per se, but a simpler way of looking at something that I always knew but never quite articulated), other times I'll find validation of something I've been trying to instill on other developers.  With each passing section I'll stop, put the book down for a moment, think about what it means, and determine my own opinion of related material.  Many times I'll find that opinion stated in the following paragraph (though far more eloquently than I've been able to as of yet).

The book is even laid out in the manner of clean code.  Short chapters sticking to specific topics, broken up into very small sections each of which maintains a discrete piece of that topic.  The chapters and sections of the book are like classes and functions in code, each maintaining its specific responsibility and elegantly fitting into the larger picture to form a complex topic.  It's a complex thing made of many simple things.

In any event, I figured I'd run through a quick exercise to apply these techniques.  To see where I stand as a clean coder, as it were.  So I jumped on a short example of some code online.  Namely, the C# implementation of the Dining Philosophers Problem at Rosetta Code.  The working implementation, in its entirety, is here:

using System;
using System.Collections.Generic;
using System.Text;
using System.Threading;
 
namespace DiningPhilosophers
{
  enum DinerState { Eat, Get, Pon }
 
  class Fork                    //       Using an object for locking purposes
  {                             //                         instead of an int.
      public int holder = -1;   //    initialized without a holder (on table)
  }
 
  class Diner
  {
    private static int        instanceCount;
    public         Fork       left;
    public         Fork       right;
    public         int        id;
    public         DinerState state             = new DinerState();
    const          int        maxWaitMs         = 100;
    public         bool       end               = false;
 
    public Diner()
    {
             id     = instanceCount++;
             left   = Program.forks[id];
             right  = Program.forks[(id + 1) % Program.dinerCount];
             state  = DinerState.Get;
      Thread thread = new Thread(new ThreadStart(doStuff));
      thread.Start();             // start instance's thread to doStuff()
    }
 
    public void doStuff()
    {
      do
      {
        if (state == DinerState.Get)
        {
          bool lockedL = false;
          Monitor.TryEnter(left, ref lockedL);      //     try lock L
 
          if (lockedL)                              //  got left fork
          {
            left.holder = id;           //  left fork holder = this
            bool lockedR = false;
            Monitor.TryEnter(right, ref lockedR);     // try lock R
 
            if (lockedR)                //    lock R succeeded too.
            {                           //           got both forks
              right.holder = id;      //      list this as holder
              state = DinerState.Eat; //                     Eat.
              Thread.Sleep(Program.rand.Next(maxWaitMs));
              right.holder = -1;      // no right fork holder now
              Monitor.Exit(right);    //                 unlock R
              left.holder = -1;       //  no left fork holder now
              Monitor.Exit(left);     //                 unlock L
              state = DinerState.Pon; //                  Ponder.
              Thread.Sleep(Program.rand.Next(maxWaitMs));
            }
            else       // got left, but not right, so put left down
            {
              left.holder = -1;       //            no holder now
              System.Threading.Monitor.Exit(left);    // unlock L
              Thread.Sleep(Program.rand.Next(maxWaitMs));
            }
          }
          else           //                 could not get either fork
          {              //                                wait a bit
              Thread.Sleep(Program.rand.Next(maxWaitMs));
          }
        }
        else               //                   state == DinerState.Pon
        {                  //      done pondering, go back to get forks
          state = DinerState.Get;         //      trying to get forks
        }
      } while (!end);
    } 
  }
 
  class Program
  {
    public const  int         dinerCount = 5;
           const  int         runSeconds = 60;
    public static List<diner> diners     = new List<diner>();
    public static List<fork>  forks      = new List<fork>();
    public static Random      rand       = new Random();
 
    static void Main(string[] args)
    {
      for (int i = 0; i < dinerCount; i++) forks.Add(new Fork());
      for (int i = 0; i < dinerCount; i++) diners.Add(new Diner());
      DateTime endTime = DateTime.Now + new TimeSpan(0,0,runSeconds);
      Console.Write("|");                               //   write header
      foreach (Diner d in diners) Console.Write("D " + d.id + "|");
      Console.Write("    |");
      for (int i = 0; i < dinerCount; i++) Console.Write("F" + i + "|");
      Console.WriteLine();
 
      do
      {                                                 // display status
        Console.Write("|");
        foreach (Diner d in diners) Console.Write(d.state + "|");
        Console.Write("    |");
        foreach (Fork f in forks) Console.Write(
            (f.holder < 0 ? "  " : "D" + f.holder) + "|");
        Console.WriteLine();
        Thread.Sleep(1000);                           //   milliseconds
      } while (DateTime.Now < endTime);
 
      foreach (Diner d in diners) d.end = true;         // signal to quit
    }
  }
}

It effectively presents a solution to the given problem, and it does so in a reasonably small web-display-friendly manner.  But it seems kind of... messy.  Let's take a look at the problems found here:

1. Functions are long.  Too long.
2. Too much nesting.  Low-level concerns are polluting higher-level concerns.
3. The comments are formatted any which way, and are small enough and obvious enough that they shouldn't even be needed if the code could just be a little more expressive.
4. Function names and variable names are ineffective.  doStuff()?  Really?
5. Classes inter-depend in odd ways.  Lots of tight coupling.
6. Classes and functions have multiple responsibilities.

There may be more, but that's certainly enough to merit a good cleaning.  So I set about refactoring, but with a specific goal in mind... The resulting output should exactly match the current output.  This is often a very important goal in refactoring code, keeping the efforts transparent to the end user.  After all, the program does what it's supposed to do.  From a business perspective, it shouldn't change.  But from a code perspective, it certainly needs to be cleaned.

It took a couple hours of my time, but the process was actually a lot of fun.  I intentionally didn't document each step along the way, though.  I'd like to, and I will.  But first I need to verify my work.  I intend to do this by setting it aside for a while and then going back to the original and attempting the process again.  Then I'll compare my two results and see how consistent I was in the process.  Depending on the results of that, I may attempt it a third time.

Eventually, I'll be happy with the results and will step through it in a more documented and tracked way, isolating individual improvements to the code and describe what they mean to the overall process.  This will end up in the form of a slide deck and several code samples, perhaps also a screen cast, which I intend to share here at a later time.  But for now at least I can present my initial output of refactoring and cleaning the code.  Let's take a look at each class...

using System;
using System.Threading;

namespace DiningPhilosophers
{
  class Program
  {
    private const int PHILOSOPHER_COUNT = 5;
    private const int SECONDS_TO_RUN = 60;

    private static Random rand;
    private static DateTime endTime;

    static void Main(string[] args)
    {
      InitializeObjects();
      DisplayFormatter.WriteOutputHeader();
      do
      {
        DisplayFormatter.WriteStatus();
        Thread.Sleep(1000);
      } while (DateTime.Now < endTime);
      Terminate();
    }

    private static void InitializeObjects()
    {
      rand = new Random();

      for (var i = 0; i < PHILOSOPHER_COUNT; i++)
        Fork.KnownForks.Add(new Fork());
            
      for (var i = 0; i < PHILOSOPHER_COUNT; i++)
        Philosopher.KnownPhilosophers.Add(
            new Philosopher(
                id: i,
                randomizer: rand,
                leftFork: Fork.KnownForks[i],
                rightFork: Fork.KnownForks[(i + 1) % PHILOSOPHER_COUNT]));
            
      endTime = DateTime.Now.AddSeconds(SECONDS_TO_RUN);
    }

    private static void Terminate()
    {
      foreach (var philosopher in Philosopher.KnownPhilosophers)
        philosopher.Dispose();
    }
  }
}

The Program class is just maintaining the program.  It initializes the global data, determines when to write output elements, and ends the program.  Simple.  Part of cleaning it up included moving the formatting of the output to a separate class to maintain that responsibility...

using System;

namespace DiningPhilosophers
{
  public static class DisplayFormatter
  {
    private const string PHILOSOPHER = "D";
    private const string FORK = "F";
    private const string ITEM_BORDER = "|";
    private const string LIST_DIVIDER = "    ";

    private const string PHILOSOPHER_HEADER_FORMAT = "{0} {1}";
    private const string FORK_HEADER_FORMAT = "{0}{1}";
    private const string PHILOSOPHER_ITEM_FORMAT = "{0}";
    private const string FORK_ITEM_FORMAT = "{0}{1}";

    public static void WriteOutputHeader()
    {
      Console.Write(ITEM_BORDER);

      foreach (var philosopher in Philosopher.KnownPhilosophers)
      {
        Console.Write(string.Format(PHILOSOPHER_HEADER_FORMAT, PHILOSOPHER, philosopher.ID));
        Console.Write(ITEM_BORDER);
      }

      Console.Write(LIST_DIVIDER + ITEM_BORDER);

      for (var i = 0; i < Philosopher.KnownPhilosophers.Count; i++)
      {
        Console.Write(string.Format(FORK_HEADER_FORMAT, FORK, i));
        Console.Write(ITEM_BORDER);
      }

      Console.WriteLine();
    }

    public static void WriteStatus()
    {
      Console.Write(ITEM_BORDER);

      foreach (var philosopher in Philosopher.KnownPhilosophers)
        WritePhilospherStatus(philosopher);

      Console.Write(LIST_DIVIDER + ITEM_BORDER);

      foreach (var fork in Fork.KnownForks)
        WriteForkStatus(fork);

      Console.WriteLine();
    }

    private static void WritePhilospherStatus(Philosopher philosopher)
    {
      var state = FormatDinerStateForDisplay(philosopher.State);
      Console.Write(string.Format(PHILOSOPHER_ITEM_FORMAT, state));
      Console.Write(ITEM_BORDER);
    }

    private static void WriteForkStatus(Fork fork)
    {
      var state = fork.IsHeld ?
                  string.Format(FORK_ITEM_FORMAT, PHILOSOPHER, fork.Holder) :
                  string.Format(FORK_ITEM_FORMAT, " ", " ");
      Console.Write(state);
      Console.Write(ITEM_BORDER);
    }

    private static string FormatDinerStateForDisplay(Philosopher.DinerState state)
    {
      switch (state)
      {
        case Philosopher.DinerState.Eating:
          return "Eat";
        case Philosopher.DinerState.GettingForks:
          return "Get";
        case Philosopher.DinerState.Pondering:
          return "Pon";
      }
      return string.Empty;
    }
  }
}

The DisplayFormatter class is responsible for maintaining the output format.  It primarily just writes the output header and each output line, following formatting specifications in its internal constants.  This and the previous Program class make up the application part of the whole system.  We also have two models within the system; Forks and Philosophers...

using System.Collections.Generic;

namespace DiningPhilosophers
{
  public class Fork
  {
    private static List<fork> _knownForks;
    public static IList<fork> KnownForks
    {
      get
      {
        if (_knownForks == null)
          _knownForks = new List<fork>();
        return _knownForks;
      }
    }

    private const int NOT_HELD = -1;

    public int Holder { get; private set; }

    public bool IsHeld
    {
      get
      {
        return (Holder != NOT_HELD);
      }
    }

    public Fork()
    {
      Holder = NOT_HELD;
    }

    public void TakeFork(int holder)
    {
      Holder = holder;
    }

    public void DropFork()
    {
      Holder = NOT_HELD;
    }
  }
}

The fork is simple enough (as it should be, it's not a complex real-world concept).  It maintains the state of the fork, providing information on who is holding it as well as methods to pick it up and drop it.

using System;
using System.Collections.Generic;
using System.Threading;

namespace DiningPhilosophers
{
  public class Philosopher : IDisposable
  {
    private static List<philosopher> _knownPhilosophers;
    public static IList<philosopher> KnownPhilosophers
    {
      get
      {
        if (_knownPhilosophers == null)
          _knownPhilosophers = new List<philosopher>();
        return _knownPhilosophers;
      }
    }

    public int ID { get; private set; }

    public DinerState State { get; private set; }

    private const int MAX_WAIT_TIME = 100;

    private Fork _leftFork;
    private Fork _rightFork;
    private bool _currentlyHoldingLeftFork;
    private bool _currentlyHoldingRightFork;
    private bool _doneEating;
    private Random _randomizer;

    private Philosopher()
    {
      SetDefaultValues();
      Dine();
    }

    public Philosopher(int id, Random randomizer, Fork leftFork, Fork rightFork) : this()
    {
      ID = id;
      _randomizer = randomizer;
      _leftFork = leftFork;
      _rightFork = rightFork;
    }

    private void SetDefaultValues()
    {
      _currentlyHoldingLeftFork = false;
      _currentlyHoldingRightFork = false;
      _doneEating = false;
      State = DinerState.GettingForks;
    }

    private void Dine()
    {
      var thread = new Thread(new ThreadStart(TryToEat));
      thread.Start();
    }

    private void TryToEat()
    {
      do
      {
        if (State == DinerState.GettingForks)
        {
          GetLeftFork();

          if (_currentlyHoldingLeftFork)
          {
            GetRightFork();

            if (_currentlyHoldingRightFork)
            {
              Eat();
              DropRightFork();
              DropLeftFork();
              Ponder();
            }
            else
            {
              DropLeftFork();
              Wait();
            }
          }
          else
          {
            Wait();
          }
        }
        else
        {
          State = DinerState.GettingForks;
        }
      } while (!_doneEating);
    }

    public void Dispose()
    {
      _doneEating = true;
    }

    private void GetLeftFork()
    {
      Monitor.TryEnter(_leftFork, ref _currentlyHoldingLeftFork);
      if (_currentlyHoldingLeftFork)
        _leftFork.TakeFork(ID);
    }

    private void GetRightFork()
    {
      Monitor.TryEnter(_rightFork, ref _currentlyHoldingRightFork);
      if (_currentlyHoldingRightFork)
        _rightFork.TakeFork(ID);
    }

    private void DropLeftFork()
    {
      _leftFork.DropFork();
      _currentlyHoldingLeftFork = false;
      System.Threading.Monitor.Exit(_leftFork);
    }

    private void DropRightFork()
    {
      _rightFork.DropFork();
      _currentlyHoldingRightFork = false;
      Monitor.Exit(_rightFork);
    }

    private void Eat()
    {
      State = DinerState.Eating;
      Wait();
    }

    private void Ponder()
    {
      State = DinerState.Pondering;
      Wait();
    }

    private void Wait()
    {
      Thread.Sleep(_randomizer.Next(MAX_WAIT_TIME));
    }

    public enum DinerState
    {
      Eating,
      GettingForks,
      Pondering
    }
  }
}

The Philosopher is where the bulk of the algorithm logic resides, naturally.  After all, it's the philosophers themselves who are trying to eat their meals.  So the decision-making process of what to do with forks and food exists in here.

This implementation probably isn't as clean as it could be.  (But then, what is?)  Looking at it now I see a few things I don't entirely like and may want to refactor:

1. The Fork and Philosopher classes also contain their globals.  Those should probably be moved out into a class which manages the globals and nothing else.
2. The TryToEat() method still has an uncomfortable amount of nesting.  It's the main algorithm of the whole thing, so one can expect it to be the most complex unit in the implementation.  But I'd like to re-think how I can write this.  It currently follows the "happy path" pattern of nesting, and I'd like to find a way to invert that.
3. Forks know who their holders are.  That doesn't really model the real-life concept of a fork.  A fork doesn't know who is holding it; It doesn't know anything.  In a real-world scenario, the philosophers would instead know who their neighbors are and be able to perceive by looking at their neighbors whether their forks are in use.  So I'd like to move fork state management out of the Fork class and into the Philosopher class.
4. Should the Program class initialize global state of the application?  Or should that be moved out into an initializer class of some sort?  I didn't think of it while writing the code, but while describing the code any time I find myself using the word "and" in the description of a class' responsibilities I feel like I may be violating Single Responsibility.

Again, I'm sure there's more.  And I welcome the input on what that may be.

It's interesting that the simple act of writing about the code has caused me to critically look at the code even further than when I was writing it.  At the start of this post I had figured that this would be a good initial implementation, and now I'm not so sure.

But then this is the nature of any creative work, is it not?  I said to a colleague yesterday that an ongoing theme in my career for the past couple of years has been that any code I wrote at least three months ago is crap.  This means that anything I write today will be crap three months from now.  This is a good problem to have.  It means that my cycle of learning new things and improving my skills is fairly short and constantly moving.  Now I'm finding that code I wrote yesterday isn't up to par.  Again, it's a good problem to have.

I'm still going to continue with the plan I'd laid out earlier in this post.  But it seems now that I have some more refactoring to do before I start on the second attempt.  It seems also that I may be refining this for a while before I find it to be presentation-ready.  Of course, I also run the risk of an endless loop whereby the time it takes to create the presentation and the act of creating it combine to form a result of not being happy with what I'm presenting.  So this will also be an opportunity to find that professional "good enough" sweet spot.  At the very least, this will be good practice on several levels.