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.

I had a small technical C# task using SOILD principle and TDD. But I failed to prove my coding skill through the test.

Can anyone offer any small advice for me? I am really eager to learn what my faults are and how I can improve.

The interview question is here, and my answer is here.

I just reference here some pieces of my source code.

Program.cs

class Program
{
    static void Main(string[] args)
    {

        ////////////////////////////////////////////////////////////////////////////////
        // Story 1. As a donor
        Console.WriteLine("-------------------------------------- Story1\r\n");

        //1.1. Craete Tax Calculator
        decimal taxRate = 20;
        ITaxCalculator taxCalculator = new TaxCalculator(taxRate);

        //1.2. Display GiftAid
        Console.WriteLine("Please enter donation amount:");
        decimal Amount = decimal.Parse(Console.ReadLine());
        Console.WriteLine("=> Your Gift Aid is: {0} \r\n\r\n", taxCalculator.GetGiftAid(Amount));

        ////////////////////////////////////////////////////////////////////////////////
        // Story 2. As a site administrator
        Console.WriteLine("-------------------------------------- Story2\r\n");

        //2.1. Create using factory pattern
        taxCalculator = new TaxCalculatorFactory().GetObject(new TaxRepository());

        Console.WriteLine("Please enter donation amount:");
        Amount = decimal.Parse(Console.ReadLine());

        //2.2. Display GiftAid
        Console.WriteLine("=> Your Gift Aid is: {0} \r\n\r\n", taxCalculator.GetGiftAid(Amount));


        Console.WriteLine("Press any key to exit.");
        Console.ReadLine();
    }
}

Calculator Classes

interface ITaxCalculatorFactory
{
    ITaxCalculator GetObject(decimal taxRate);
    ITaxCalculator GetObject(ITaxRepository taxRepo);
}

public class TaxCalculatorFactory
{
    public TaxCalculatorFactory()
    {
    }
    public ITaxCalculator GetObject(decimal dAmount)
    {
        return new TaxCalculator(dAmount);
    }
    public ITaxCalculator GetObject(ITaxRepository taxRepo)
    {
        return new TaxCalculatorByData(taxRepo);
    }
}


public interface ITaxCalculator
{
    decimal GetGiftAid(decimal dAmount);
}

public class TaxCalculator : ITaxCalculator
{
    private readonly decimal _taxRate;
    public TaxCalculator(decimal taxRate)
    {
        _taxRate = taxRate;
    }

    public decimal GetGiftAid(decimal dAmount)
    {
        var gaRatio = _taxRate / (100 - _taxRate);
        return dAmount * gaRatio;
    }

}

public class TaxCalculatorByData : ITaxCalculator
{
    private readonly ITaxRepository _taxRepo;
    public TaxCalculatorByData(ITaxRepository taxRepo)
    {
        _taxRepo = taxRepo;
    }

    public decimal GetGiftAid(decimal dAmount)
    {
        var gaRatio = _taxRepo.GetTaxRate() / (100 - _taxRepo.GetTaxRate());
        return dAmount * gaRatio;
    }

}

Test Codes

[TestFixture]
public class GetAidTest
{
    ////////////////////////////////////////////////////////////////////////////////
    // Story 1. As a donor
    [Test]
    public void Story1_Should_Return_TwentyFive_From_GiftAid()
    {
        /////////////////////////////////////////////////////////////
        // Setup
        decimal taxRate = 20;
        decimal donationAmount = 100;
        decimal valueExpected = 25;
        var taxCalculator = new TaxCalculator(taxRate);

        /////////////////////////////////////////////////////////////
        // Action
        var result = taxCalculator.GetGiftAid(donationAmount);

        /////////////////////////////////////////////////////////////
        // Verify the result
        Assert.AreEqual(valueExpected, result, "Should return 25");
    }

    ////////////////////////////////////////////////////////////////////////////////
    // Story 2. As a administrator
    [Test]
    public void Story2_Should_Store_TaxRate_Fourty_And_Return_GiftAid()
    {

        /////////////////////////////////////////////////////////////
        // Setup
        decimal taxRate = 40;
        decimal donationAmount = 100;
        decimal valueExpected = 66.67m;

        var repository = new Mock<ITaxRepository>();

        repository.Setup(r => r.GetTaxRate()).Returns(taxRate);
        repository.Setup(r => r.Save(It.IsAny<decimal>())).Verifiable();

        ITaxCalculator taxCalculator = new TaxCalculatorFactory().GetObject(repository.Object);

        /////////////////////////////////////////////////////////////
        // Action
        var result = decimal.Round(taxCalculator.GetGiftAid(donationAmount), 2, MidpointRounding.AwayFromZero);

        /////////////////////////////////////////////////////////////
        // Verify the result
        Assert.AreEqual(valueExpected, result, "Should return 25");
    }
}
share|improve this question
1  
Did you submit a pull request from your public repo? "If you have a paid GitHub account, feel free to create a private repo . . . and send us a pull request." [their emphasis] –  David Harkness Feb 17 at 13:39
1  
"This assignment takes an average of about 30 minutes." 4 user stories in 30 mins. They must have really good programmers. –  abuzittin gillifirca Feb 18 at 7:28

3 Answers 3

  1. TaxCalculator and TaxCalculatorByDB contains the same logic:

    var gaRatio = _taxRepo.GetTaxRate() / (100 - _taxRepo.GetTaxRate());
    return dAmount * gaRatio;
    

    Furthermore, it seems that TaxCalculator and the GetObject(decimal taxRate) method in the factory only used for test. It's a test smell: Test Logic in Production. I would use a mock instead of it.

  2. The assertion message does not say anything more than which is already obvious from the code itself:

    Assert.AreEqual(valueExpected, result, "Should return 25"); 
    

    Try to explain the whys if it's possible. Why should it return 25? Otherwise it's just noise.

  3. You could write more than one test per story. What happens or what should happen on invalid inputs, for example?

  4. I think the Program.cs should contain only one thing: get donation amount and print the answer. Requirements of Story 1-4 should be combined.

  5. I think implementing Story 4 means that the program

    1. should ask for the event type, or
    2. print the result for every event type.
  6. I can't find any test for Story 3 and Story 4.

  7. I can't test now (I don't have a C# compiler) but I think values of Story 4 should also be printed with rounding.

  8. The ITaxRepository.Save(decimal taxRate) method seems unnecessary. Story 2 isn't crystal clear about that but since there isn't any detail about how an admin can change the tax rate I guess they meant it with SQL commands (for example) and not through the program.

share|improve this answer
  1. The interview defines 4 user stories but you have only really implemented 2 as far as I can see.

  2. Story 3 says that the amount should be rounded to 2 decimal places. You have done that by applying the rounding to the result as part of a unit test which misses the point of the story (imho). The story says something about "should be displayed rounded to two decimal places" so it might need a tax calculator view which displays the results as per requirements.

  3. You should avoid having two ways of creating your TaxCalculator objects. If you have a factory then you usually don't want the user to instantiate new objects by new. If I were to use your class I would not know which way to use and if there is a difference or not.

  4. dAmount smells like hungarian notation of which I'm not a fan of and which is generally frowned upon.

  5. I would prefer Create over GetObject as method name in the factory.

  6. Remove the GetObject(decimal amount) factory method - you can pass a mock repository in unit tests which always returns a fixed amount.

share|improve this answer

In addition to the excellent previous posts and could be nit picking, but why the huge comment blocks?

////////////////////////////////////////////////////////////////////////////////

Seems unnecessary and it would certainly put me off if I was reviewing it!

There is a typo in the comments, and again, typo's could be nitpicking, but I'd certainly be thinking "has care and attention been paid whilst the code is being written" (I accept that an interview situation is not a normal working environment, so this may be a bit harsh!).

I'd also second the suggestion to avoid having two ways of creating your calculators. The factory doesn't seem like it's needed for this example.

I'd be thinking it's possibly disobeying the YAGNI principle by putting in a factory when you only have a single tax calculator to handle.

EDIT:

Further nitpicking!

You have a block level variable 'Amount' with a capital A, which I wouldn't expect, this is inconsistent with the rest of your variables.

share|improve this answer
1  
+1 In a do-at-home test, there's no excuse for formatting inconsistencies or typos. Polish it up when they give you the time. :) –  David Harkness Feb 17 at 13:35

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.