Comparing myself to Uncle Bob Martin

Before last week, I attended Roy Osherove’s TDD Masterclass,  I do not think I would have had the “you know what’s” to be able to put up an alternative solution to one of Uncle Bobs (aka Robert Martin) examples of doing Test Driven Development,  but here I am publishing the the solution I arrived at and suggesting that my solution better servers the values of Readability, Maintainability and trustworthiness.

In Uncle Bobs defence his post is dated Apr 2006 and the art of Test Driven Development has moved on leaps and bounds in the past few years.

The Problem

Write a class named “PrimeFactors” that has one static method: generate.

The generate method takes an integer argument and returns a List<Integer>. That list contains the prime factors in numerical sequence.

To see the details and how it was previously solved.

My Solution

Although the original is coded in Java and mine in C#, there is enough similarities for you to be able to understand both – weird that really :)

The Tests

1: using System.Collections.Generic;

2: using NUnit.Framework;

3:

4: namespace Product.Tests

5: {

6: [TestFixture]

7: public class CalculatorTests

8: {

9: private Calculator c;

10:

11: [SetUp]

12: public void SetUp()

13: {

14: c = new Calculator();

15: }

16:

17: [Test]

18: public void Generate_Zero_ReturnEmptyList()

19: {

20: List<int> result = c.Generate(0);

21:

22: Assert.AreEqual(0, result.Count);

23: }

24:

25: [Test]

26: public void Generate_One_ReturnEmptyList()

27: {

28: List<int> result = c.Generate(1);

29:

30: Assert.AreEqual(0, result.Count);

31: }

32:

33: [TestCase(2)]

34: [TestCase(3)]

35: [TestCase(5)]

36: public void Generate_PrimeNumber_ReturnPrimeNumber(int expected)

37: {

38: List<int> result = c.Generate(expected);

39:

40: Assert.AreEqual(expected, result[0]);

41: }

42:

43: [TestCase(4,2,2)]

44: [TestCase(6,2,3)]

45: public void Generate_NonPrimeNumberDivisableByTwo_ReturnTwoProducts(int number, int firstPrime, int secondPrime)

46: {

47: List<int> result = c.Generate(number);

48:

49: Assert.AreEqual(firstPrime, result[0]);

50: Assert.AreEqual(secondPrime, result[1]);

51: }

52:

53: [Test]

54: public void Generate_NonPrimeNumberWithThreeProducts_ReturnThreeProducts()

55: {

56: List<int> result = c.Generate(8);

57:

58: Assert.AreEqual(2, result[0]);

59: Assert.AreEqual(2, result[1]);

60: Assert.AreEqual(2, result[2]);

61: }

62:

63: [Test]

64: public void Generate_NonPrimeNumberNotDivisableByTwoWithTwoProducts_ReturnProducts()

65: {

66: List<int> result = c.Generate(9);

67:

68: Assert.AreEqual(3, result[0]);

69: Assert.AreEqual(3, result[0]);

70: }

71: }

72: }

The Production Code

1: using System.Collections.Generic;

2:

3: namespace Product

4: {

5: public class Calculator

6: {

7: private const int SMALLEST_PRIME = 2;

8:

9: public List<int> Generate(int i)

10: {

11: List<int> primes = new List<int>();

12:

13: int divider = SMALLEST_PRIME;

14: while (HasPrimes(i))

15: {

16: while (IsDivisable(i, divider))

17: {

18: i = AddPrimeToProductsAndReduce(i, primes, divider);

19: }

20: divider++;

21: }

22: return primes;

23: }

24:

25: private bool IsDivisable(int i, int divider)

26: {

27: return i%divider == 0;

28: }

29:

30: private bool HasPrimes(int i)

31: {

32: return i >= SMALLEST_PRIME;

33: }

34:

35: private int AddPrimeToProductsAndReduce(int i, List<int> primes, int prime)

36: {

37: primes.Add(prime);

38: i /= prime;

39: return i;

40: }

41: }

42: }

I understand that showing the final solution does not prove the value of TDD, I will be posting a web cast of this process to really show how TDD helps you code to evolve and also how I adhere to the Red, Green, Refactor steps to achieve readable, maintable and trustworthy code.

Where do I think mine is better?

The Tests

The tests all adopt the naming convention Method_Scenario_Behaviour

The Act and Assert are kept separate for readability.

I have not used any logic in my tests, unlike the list()method used in Uncle Bobs.

The Production Code

No magic numbers, I have extracted well named constants.

Refactored the logic in ther generate method into more readable methods. For example line 14.

1: while (HasPrimes(i))

Is much easier to understand than the line

1: for (int candidate = 2; n > 1; candidate++)

I don’t like the way the while loops were refactored into for loops to reduce the code. If you look closely at the for loop above you will see that it does not follow the normal convention where the evaluation part is related to the incremental part.  This I feel makes it easy to misread and hard to read if you don’t miss it.

Overall I think that the my code, although longer than Uncle Bobs is much easier to read and therefore easier to maintain.

I would love to hear feedback on the code, perhaps I have a couple of refactoring’s that could improve it still further.

This entry was posted in Development and tagged . Bookmark the permalink.
  • jdiamond

    Hi Andrew,

    I agree that your code is easier for me to read and understand.

    I do have a suggestion, though. Since you're using C#, you can use the yield keyword to turn your method into an iterator. That would avoid the overhead of creating the list which might remove a few lines. Iterators are still weird to a lot of people, though.

    Also, why did you make a method that does two things (AddPrimeToProductsAndReduce)?

  • http://www.21apps.com AndrewWoody

    Jason,

    Thanks, I will look at the yielf keyword – I have not used this before so this is definately something to learn (http://msdn.microsoft.com/en-us/library/9k7k7cf…)

    The method AddPrimeToProductsAndReduce is the bit I am most uncomfortable with, at first it was just named AddPrimeToProducts as part of the refatoring but I found that when reading the code it did not explain fully what it did. An area for improvement.

  • http://twitter.com/pauliehaha pauliehaha

    Love it! Now do a daily kata using the yield keyword!

  • jonathanadams

    Greate write up and also shows the power of TDD to simplfy things :)

    Excellent !

  • Pingback: Sitting down with Steve Ballmer; 2010, Worth the Upgrade?; Windows 7 tries to be cool - SharePoint Daily - Bamboo Nation

  • http://thedailyreviewer.com The Daily Reviewer

    Hi!

    Congratulations! Your readers have submitted and voted for your blog at The Daily Reviewer. We compiled an exclusive list of the Top 100 sharepoint Blogs, and we are glad to let you know that your blog was included! You can see it at http://thedailyreviewer.com/top/sharepoint

    You can claim your Top 100 Blogs Award here : http://thedailyreviewer.com/pages/badges/sharep

    P.S. This is a one-time notice to let you know your blog was included in one of our Top 100 Blog categories. You might get notices if you are listed in two or more categories.

    P.P.S. If for some reason you want your blog removed from our list, just send an email to angelina@thedailyreviewer.com with the subject line “REMOVE” and the link to your blog in the body of the message.

    Cheers!

    Angelina Mizaki
    Selection Committee President
    The Daily Reviewer
    http://thedailyreviewer.com

  • http://www.21apps.com AndrewWoody

    Looking at the Yield keyword in C# I am not sure this will help me in this situation. Using Yeild in a for loop, will pass out (yield) the result and continue to count up in the loop (most examples use a for loop).

    If in the example I want to find the Prime Products for 8 (which is 2,2,2) I cannot see how Yield would return this. I may be missing something, would be interested to see a code sample.

    The other problem I suspect with Yield is that it is not that well used, doesn't mean it's wrong, but adding this to any code would mean most developers would be saying WTH – how does this work?

  • http://www.21apps.com AndrewWoody

    Looking at the Yield keyword in C# I am not sure this will help me in this situation. Using Yeild in a for loop, will pass out (yield) the result and continue to count up in the loop (most examples use a for loop).

    If in the example I want to find the Prime Products for 8 (which is 2,2,2) I cannot see how Yield would return this. I may be missing something, would be interested to see a code sample.

    The other problem I suspect with Yield is that it is not that well used, doesn't mean it's wrong, but adding this to any code would mean most developers would be saying WTH – how does this work?

  • Pingback: Refactoring with Iterators: Prime Factors « Solutionizing .NET

  • http://blog.salamandersoft.co.uk/ Richard Willis

    Hi Andrew,

    Great post. The more we can do to persuade people using TDD or even just unit testing the better.

    There's a few things I'd do differently to make it more readable IMHO. All really minor though.

    1. I wouldn't use i as a parameter name for a method. It's confusing as it's normally limited to internal for loops an hence isn't descriptive.
    2. I'd consider throwing an ArgumentOutOfRangeException if a negative number was passed in to clarify the use. People might expect it to return the primes as though it was a positive number. I certainly did until I looked up getting the prime factors of a negative number.
    3. AddPrimeToProductsAndReduce. I found it easier to read moving the 2 lines back into the method it's called from. I'd agree on an extract method refactoring if there was more to AddPrime and Reduce, but as the method does 2 things in 2 lines of code it seems superfluous. IsDivisable on the other hand is clearer than the line of code it replaces.
    4. Depending on who will also be looking at the code I'd write out i /= prime longhand to i = i / prime. /= isn't often used, in fact I don't think I've ever seen it outside documentation before. I had to stop and think what it was doing rather than just glance at the line and know. Could just be my problem of course!
    5. The private methods don't use any instance variables so could/should be static. FxCop would tell you to do this.
    6. And this one really is minor. The problem said that the method should be static.
    7. There's a bug in your test Generate_NonPrimeNumberNotDivisableByTwoWithTwoProducts_ReturnProducts. It tests the first item in the output list twice.
    8. Generate_PrimeNumber_ReturnPrimeNumber. I'd call the parameter something like prime so it's clear it's the input as well as the expected result.

    Keep up the good work.
    Richard

  • Pingback: Refactoring with Iterators: Prime Factors - Solutionizing .NET (Keith Dahlby) - Los Techies : Blogs about software and anything tech!

  • http://www.21apps.com AndrewWoody

    Good suggestions Richard.

    In fact I went through the TDD Kata for this problem on the train yesterday and the solution was actually slightly different, in fact I think better.

    I did not add any exception handling as this was not a 'requirement' but I take your point that this should really be covered.

    i /= << I was the same as you at first, but this is one I think really should be learnt. we do += and -= all the time so it's not that challenging.

    And great to see you spotted a bug in my tests :)

    Going to post a web cast of doing the process so that people understand the technique and reason for some of the refactorings.

  • ElangoPrasiddhi

    Woo this is a nice blog, i would love to read more.

    respect
    kailis
    ______________________________________________
    Buy Aion Kinah | aion kina

  • http://www.computerhandymanmd.com Data Recovery Baltimore

    Man, you are on fire today. Great posts

    Thanks
    Justin Kemp
    ______________________________________________

  • http://www.bulktv.com/SE-Television.html Wholesale TV Services

    Man, you are on fire today. Great posts

    respect
    Justin Kemp
    ______________________________________________

  • http://www.computerhandymanmd.com/ Computer Services Baltimore

    I'm a software developer, building applications in FileMaker Pro and I'm trying to decide on the best strategy for tracking my work.

    Thanks
    raven conway
    ______________________________________________

  • http://www.dkairsystems.com/ Baltimore Heating

    I'm a software developer, building applications in FileMaker Pro and I'm trying to decide on the best strategy for tracking my work.

    Thanks
    andymock
    ______________________________________________

  • Jack Lewis

    Uncle Bob Martin kata is way better, u basically suck