Recently I was posed the following question:

Write a piece of code that prints all odd integer numbers between 1 and 99

This really isn’t a difficult question but it still requires some thought. When I’m posed with any question I like to break things down into their constituent parts.

Here’s the process I went through:

Okay, so I’ll define two variables for a start and end value and there’s going to have to be a loop.

int startValue = 1;
int endValue = 99;
for(int i = startValue;
     i <= endValue;
     i++)
{
   // work out if "i" is an odd number
}

Now, for the odd number detection. And… after a few umms and errrs … I’m going to have to mod 2 (%2) the current value of i to work out if the value is odd. More … umms and errs. Okay, I’ve finally worked out that if something mod 2 is not equal to 0 it’s clearly an odd number. This took me longer than it should have but never mind. Once I’ve detected if i is an odd number I’ll then put the odd number into a list for use later.

int startValue = 1;
int endValue = 99;
IList<int> oddValues = new List&lt;int&gt;();
for(int i = startValue;
     i &lt;= endValue;
     i++)
{
   if(i%2 != 0)
   {
      oddValues.Add(i);
   }
}

Those of you that are good at these little puzzles, or just think this is way too easy, might already be screaming at me about one of the following:

  • Why are you using a IList, why don’t you just print the value?
  • Odd numbers are always 2 apart so why aren’t you just increment i by 2 using i+=2?


I admit it, I missed the second point and that is a bit silly of me. However, what I have starting doing is separating the concerns of the piece of code. The code does two things; it detects the odd number and it prints the odd numbers. Those are two very distinct things. So, my code now looks like this:

int startValue = 1;
int endValue = 99;
IList<int> oddNumbers = new List&lt;int&gt;();
for(int i = startValue;
     i &lt;= endValue;
     i++)
{
   if(i%2 != 0)
   {
      oddNumbers.Add(i);
   }
}

string oddNumbersList = string.Join(",", oddNumbers.ToArray());
Console.WriteLine(oddNumbersList );

I’m now going to refactor this further so I’ll put the two different pieces of functionality into different methods. I’ll rename the i, startValue and endValue variables to be something a bit more useful; say numberToCheck, startNumber and endNumber. I’ll also create another helper for the odd number checking named IsOddNumber:

IList<int> GetOddNumbersBetween(int startNumber, int endNumber)
{
   IList oddValues = new List&lt;int&gt;();
   for(int numberToCheck = startNumber;
        numberToCheck &lt;= endNumber;
        numberToCheck++)
   {
      if(IsOddNumber(numberToCheck) == true)
      {
         oddValues.Add(numberToChecki);
      }
   }
   return oddValues;
}

bool IsOddNumber(int number)
{
   return (number % 2 == 1);
}

void PrintOddNumbersBetween(int startNumber, int endNumber)
{
   IList<int> oddNumbers = GetOddNumbersBetween(startNumber, endNumber);
   string oddNumbersList = string.Join(",", oddNumbers.ToArray());
   Console.WriteLine(oddNumbersList);
}

Let’s say I then notice the second point you’ve been screaming at me about (Odd numbers are always 2 apart so why aren’t you just increment i by 2 using i+=2) that I mentioned above? In practice I should notice this sort of thing either when I give the code a complete review, or one of my peers spots it. When I see this problem I decide to update the for loop, as noted, and I then see that I possibly don’t need the if(IsOddNumber(i) == true) statement. Although it would pain me to do this, since it’s a lovely little method, I would need to consider deleting it. But then it strikes me, I’m no longer just solving the “odd numbers between 1 and 99 problem” so I can’t just assume that the startNumber is going to be an odd number. I need to make sure that it’s an odd number so I’ll create another small utility method for that called EnsureOddNumber which will check if the value passed is an odd number, and if not return the next odd number (I’d like to rethink the name of this method).

IList<int> GetOddNumbersBetween(int startNumber, int endNumber)
{
   startNumber = EnsureOddNumber(startNumber);

   IList<int> oddValues = new List&lt;int&gt;();
   for(int numberToCheck = startNumber;
        numberToCheck &lt;= endNumber;
        numberToCheck+=2)
   {
      oddValues.Add(numberToCheck);
   }
   return oddValues;
}

int EnsureOddNumber(int number)
{
   if( IsOddNumber(startNumber) == false )
   {
      startNumber++;
   }
   return startNumber;
}

bool IsOddNumber(int number)
{
   return (number % 2 == 1);
}

void PrintOddNumbersBetween(int startNumber, int endNumber)
{
   IList<int> oddNumbers = GetOddNumbersBetween(startNumber, endNumber);
   string oddNumbersList = string.Join(",", oddNumbers.ToArray());
   Console.WriteLine(oddNumbersList);
}

Now, when I look at this code I get a warm feeling because I feel that it solves the problem, it’s well engineered, the concerns are separated and the code is completely self documenting.

There are a few comments that people may have here:

The question asked specifically to print odd values between 1 and 99 and you’ve done more than was required.

Although my answer does satisfy the original question have I over engineered things? The question does specifically ask us to print odd values between 1 and 99 so maybe I should have created a function that just satisfied that requirement.

void PrintOddNumbersBetween1And99()
{
   for(int i = 1;
        i &lt;= 99;
        i+=2)
   {
      Console.WriteLine(i + " ");
   }
}

And I’d have to admit that this very short piece of code exactly answers the question. But I’d also argue that there is very little chance of this code being reused. Don’t get me wrong, you definitely shouldn’t over engineer things but there should be some scope for code reuse.

For such a simple problem you’ve over engineered this.

or

There’s a better solutions that that… it’s not efficient

Is creating four methods over engieering? Does my code require any comments to provide documentation? There may well be a more performant solution to this, but that’s not my point. My point is the way of approaching a question: the thought processes involved in understanding the problem and breaking it down to separate concerns, making it easy to read, reusable and self documenting. If the code that worked out the odd numbers is not efficient it could easily be changed in one place without impacting the interface, the other methods within the class, or the overall functionality.

Some people may jump to the simplest solution but I think the way i’ve described approaching and solving the problem demonstrates good practice. If I’m completely honest I would normally approach the development of something such as this by writing a test case first since I practice TDD but that can wait for another blog post.

Related posts:

  1. Problem solving lessons relearnt
  2. Collecta Gets Dispensed: Was It Solving a Hard Enough Problem?
  3. Recent article in .net magazine: WebSockets – Code a real-time survey
Tagged with:
 
  • http://www.defize.co.uk Adam Thornburn

    Good post, couldn’t agree more about the importance of separation of concerns and reusable code.

    I do think you could go one step further though, currently your example reads as a single class with methods for the elements of functionality. I would argue that the problem as described though is composed of two sub-problems:

    1) Identifying a range of odd numbers.
    2) Printing a list of integers.

    To properly separate those two main concerns I would be tempted to extract all the code for identifying odd numbers to its own class. Something like this

    
    
    
    	
    
    
    
    
    class Program{    static void Main(string[] args)    {        PrintOddNumbersBetween(1, 99);        Console.ReadKey();    }
    
        private static void PrintOddNumbersBetween(int startNumber, int endNumber)    {        var oddNumbersList = string.Join(",", OddNumbers.Range(startNumber, endNumber));        Console.WriteLine(oddNumbersList);    }}
    
    static class OddNumbers{    public static IEnumerable<int> Range(int startValue, int endValue)    {        if (startValue > endValue)        {            throw new ArgumentOutOfRangeException("endValue", "endValue must be greater than startValue");        }
    
            for (var currentValue = startValue; currentValue <= endValue; ++currentValue)        {            if (IsOddNumber(currentValue))            {                yield return currentValue;            }        }    }
    
        private static bool IsOddNumber(int value)    {        return (value % 2) != 0;    }}
    


    View code on Pastie
    ? I think this approach also makes the intent program element of the code even clearer and therefore more self-documenting.

  • http://www.leggetter.co.uk Phil Leggetter

    @Adam I completely agree with you. I moved the different problems into methods where the Odd Number functionality would sit very nicely in a class. Also, your use of IEnumerable, where an IList really isn’t required, and using yield is a nice idea.

  • dom

    Yield is very nice, but isn’t that really getting away from the nub of the actual question and getting into ioccc# territory? To me, a simple for loop with the expression inside answers the actual question both succinctly and transparently without over-engineering. x % 2 == 0 is a common enough idiom that it really doesn’t need breaking down.

    I think the key here is that if the problem was more complex then a more engineered solution is appropriate, but idioms are one of the most important tools a programmer has, and expressing code clearly in 5 lines is more maintainable than something that takes over a page to achieve the same thing.

  • http://www.leggetter.co.uk Phil Leggetter

    @dom I had to google ioccc# (International Obfuscated C[#] Code Contest) :)

    I was originally going to call this post something like “the way people think and write code”. My point with this original title was going to be that I believe that some developers think in exactly the way you describe; they see an elegance in being able to solve the problem in very few lines of code (succinctly) and find that code very readable (and maintainable?). Other developers, like me, like things to be broken out as much as possible. In this case I agree that the IsOddNumber may be a bit much.

    My argument for breaking things down would be that code *will* evolve and by creating a structure to begin with encourages other to follow suit. I’d apply the same argument if I were to asked to explain why I’ve made the code handle odd numbers in ranges outside 1 to 99.

  • dom

    I totally agree Phil, we have completely different coding styles and ways of tackling problems :) . I think it’s an artifact of the environment where people start developing – I started with a 40×24 line terminal before graduating to 80×25 variety using an editor not an ide, hence my bias towards brevity and what I perceive to be simplicity.

    Requirements do evolve over time, but they don’t always evolve in the way you envisage them to. For example here you’re finding numbers which aren’t divisible by 2, but my next request is for numbers that are divisible by 27. So you’ll have to completely refactor your code (at this point I’d then create a bool IsMultipleOf() method so I’d have some work as well). Then, I’ll increase the range to 1 …. ULONG_MAX :)

    Premature optimisation is frowned upon, but sometimes premature over-engineering should be as well! (It’s also not very iterative either).

Set your Twitter account name in your settings to use the TwitterBar Section.