Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Me and my colleagues do a small TDD-Kata practice everyday for 30 minutes. For reference this is the link for the excercise http://osherove.com/tdd-kata-1/

The objective is to write better code using TDD. This is my code which I've written


public class Calculator
{
    public int Add( string numbers )
    {
        const string commaSeparator = ",";
        int result = 0;
        if ( !String.IsNullOrEmpty( numbers ) )
            result = numbers.Contains( commaSeparator ) ? AddMultipleNumbers( GetNumbers( commaSeparator, numbers ) ) : ConvertToNumber( numbers );

        return result;
    }

    private int AddMultipleNumbers( IEnumerable getNumbers )
    {
        return getNumbers.Sum();
    }

    private IEnumerable GetNumbers( string separator, string numbers )
    {
        var allNumbers = numbers
            .Replace( "\n", separator )
            .Split( new string[]
        {
            separator
        }, StringSplitOptions.RemoveEmptyEntries );

        return allNumbers.Select( ConvertToNumber );
    }

    private int ConvertToNumber( string number )
    {
        return Convert.ToInt32( number );
    }
}

and the tests for this class are


[TestFixture]
    public class CalculatorTests
    {

        private int ArrangeAct( string numbers )
        {
            var calculator = new Calculator();
            return calculator.Add( numbers );
        }

        [Test]
        public void Add_WhenEmptyString_Returns0()
        {
            Assert.AreEqual( 0, ArrangeAct( String.Empty ) );
        }

        [Test]
        [Sequential]
        public void Add_When1Number_ReturnNumber(
            [Values( "1", "56" )] string number,
            [Values( 1, 56 )] int expected )
        {
            Assert.AreEqual( expected, ArrangeAct( number ) );
        }

        [Test]
        public void Add_When2Numbers_AddThem()
        {
            Assert.AreEqual( 3, ArrangeAct( "1,2" ) );
        }

        [Test]
        public void Add_WhenMoreThan2Numbers_AddThemAll()
        {
            Assert.AreEqual( 6, ArrangeAct( "1,2,3" ) );
        }

        [Test]
        public void Add_SeparatorIsNewLine_AddThem()
        {
            Assert.AreEqual( 6, ArrangeAct( @"1
2,3" ) );
        }
    }

Now I'll paste code which they have written


public class StringCalculator
    {
        private const char Separator = ',';

        public int Add( string numbers )
        {
            const int defaultValue = 0;
            if ( ShouldReturnDefaultValue( numbers ) )
                return defaultValue;

            return ConvertNumbers( numbers );
        }

        private int ConvertNumbers( string numbers )
        {
            var numberParts = GetNumberParts( numbers );
            return numberParts.Select( ConvertSingleNumber ).Sum();
        }

        private string[] GetNumberParts( string numbers )
        {
            return numbers.Split( Separator );
        }

        private int ConvertSingleNumber( string numbers )
        {
            return Convert.ToInt32( numbers );
        }

        private bool ShouldReturnDefaultValue( string numbers )
        {
            return String.IsNullOrEmpty( numbers );
        }

    }

and the tests


    [TestFixture]
    public class StringCalculatorTests
    {
        [Test]
        public void Add_EmptyString_Returns0()
        {
            ArrangeActAndAssert( String.Empty, 0 );
        }


        [Test]
        [TestCase( "1", 1 )]
        [TestCase( "2", 2 )]
        public void Add_WithOneNumber_ReturnsThatNumber( string numberText, int expected )
        {
            ArrangeActAndAssert( numberText, expected );
        }

        [Test]
        [TestCase( "1,2", 3 )]
        [TestCase( "3,4", 7 )]
        public void Add_WithTwoNumbers_ReturnsSum( string numbers, int expected )
        {
            ArrangeActAndAssert( numbers, expected );
        }


        [Test]
        public void Add_WithThreeNumbers_ReturnsSum()
        {
            ArrangeActAndAssert( "1,2,3", 6 );
        }


        private void ArrangeActAndAssert( string numbers, int expected )
        {
            var calculator = new StringCalculator();
            var result = calculator.Add( numbers );
            Assert.AreEqual( expected, result );
        }
    }

Now the question is which one is better?

My point here is that we do not need so many small methods initially because StringCalculator has no sub classes and secondly the code itself is so simple that we don't need to break it up too much that it gets confusing after having so many small methods.

Their point is that code should read like english and also its better if they can break it up earlier than doing refactoring later and third when they will do refactoring it would be much easier to move these methods quite easily into separate classes.

My point of view against is that we never made a decision that code is difficult to understand so why we are breaking it up so early.

So the question is when is the good time to break up a code into smaller methods?

  • Initially when you are writing the code
  • Or when you look at the code and see it is difficult to read

The reason I hesitate to choose option 1 because it will create so many methods initially which confuses me and I prefer to write it first, see if it is harder to read, then break it up. If it is a big block then I'll shift it into a separate class and break it up there.

So I need a third person's opinion to understand which option is much better.

share|improve this question

migrated from programmers.stackexchange.com Nov 1 '13 at 13:35

This question came from our site for professional programmers interested in conceptual questions about software development.

1 Answer 1

up vote 3 down vote accepted

I think the code should be easily readable from the start. My goal is to avoid the need for comments. Your colleagues make great points about the advantages of such verbose code as well. Additionally, tools such as Resharper make it incredibly easily to restructure code so it's easily readable like this.

The code that you wrote is definitely harder to read in my opinion.

share|improve this answer
    
I agree with you here that the ?: operator which I've used in Add method is making it a little harder to read. But this is my point here, after I've realized that this block is difficult to understand then I'll shift it to a separate method but not in the first place. The reason is, if we take their approach which is to create a separate method for almost everything then we get a lot of smaller methods which creates confusion. Wouldn't it better to extract that block to another class first and then refactor it there to create smaller methods. –  adeel41 Nov 1 '13 at 14:48
1  
Do you know that they created the separate method first, or if they performed an "extract method" refactoring (refactoring.com/catalog/extractMethod.html) after the fact? I tend to use both techniques depending on my knowledge of the code I'm writing. –  Alex Dresko Nov 1 '13 at 15:17
    
they performed "extract method" after writing the code. But still isn't it too early to extract a method just to make it read like english? because they do it for alomst each line because of Single Responsibility Principle. I am not saying they are doing wrong, I am asking isn't it too early? plus it will add so many small methods which will be difficult to handle. Why not after realizing that code is harder to read then shift the block into a class and then refactor it there. –  adeel41 Nov 1 '13 at 15:21
    
I don't think it's too early. The quicker you can make the code easier to read, the better. There's an extremely high likelihood that no one will have time to make it easier to read later. Plus, it's easier to perform an "extract class" refactoring if you've got a lot of methods to work with. Also, if necessary, you can always perform an "inline method" refactoring. –  Alex Dresko Nov 1 '13 at 15:38
    
I Agree with @AlexDresko on this. It is better to write clean code early. I'd also like to add that it is much easier to refactor / clean up the code while the original meaning or idea is still fresh in your mind. –  Jeff Vanzella Nov 1 '13 at 16:13

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.