Software Design Blog

Part of my role is to review code and to coach developers.

Reviewing code provides insight into a range of techniques to solve problems. Like many developers, we often steal each other’s ideas. To become a good thief, you really need to be able to identify what is valuable so that you don’t steal someone else’s rubbish code.

Whilst coaching developers, I often take the code submitted for a code review and ask the person I’m coaching to review it. This technique helps to assess the fidelity of a developer to identify good and bad code. It also exposes new potential skills that can be coached or helps with confirming that a developer has mastered a skill.

The aim of this post is to show a working solution and discuss potential problems. A follow up post will provide an alternative that will solve these problems.

Setup

The problem we are trying to solve is to validate a credit card number based on the credit card type. The credit card context model / data transfer object (DTO) is shown below.

  
    public class CreditCard
    {
        public string Type { get; set; }
        public string Name { get; set; }
        public string Number { get; set; }
        public string Expiry { get; set; }
    }

    public interface ICreditCardValidator
    {
        void Validate(CreditCard creditCard);
    }

Code Review

The solution that was submitted for a code review is shown below.

  
    public class CreditCardValidator : ICreditCardValidator
    {
        public void Validate(CreditCard creditCard)
        {
            if (creditCard.Type.ToLower() == "visa")
            {
                var visaRegEx = new Regex("^4[0-9]{6,}$");
                if (!visaRegEx.IsMatch(creditCard.Number)) 
                      throw new Exception("Invalid card");
            }
            else if (creditCard.Type.ToLower() == "mastercard")
            {
                var masterCardRegEx = new Regex("^5[1-5][0-9]{5,}$");
                if (!masterCardRegEx.IsMatch(creditCard.Number)) 
                      throw new Exception("Invalid card");
            }
            else
            {
                throw new Exception(string.Format("Card type {0} is unsupported.", 
                                                  creditCard.Type));
            }
        }
    }

Code Smells

Let's run through a standard set of heuristics to identify code smells.

• Duplicate code – both If statements uses a fairly similar pattern but it doesn’t appear to justify refactoring
• Long method – the method length appears to be acceptable
• Large class – the class fits on one screen and it doesn’t appear to be too large
• Too many parameters – it only has one parameter so that's fine
• Overuse of inheritance – no inheritance at all
• Not testable – the class implements an interface which suggests that clients rely on a contract instead of a concrete implementation which promotes mocking and the class appears to be easily testable

Code Problems

Even though the code doesn't appear to have code smells, the code is not great. Here is a list of problems with the code:

1. Guard Exception – an ArgumentNullException should be thrown before line 5 when the client calls the validator with a null credit card instance
2. NullReferenceException – the client will receive an “object reference not set to an instance of an object” exception on line 5 when the card type is null
3. Throwing Exceptions – throwing custom exceptions such as InvalidCreditCardNumberException is better than throwing a generic Exception which is harder to catch and bubble up
4. Immutable String Comparison – at least the code is case insensitive using .ToLower() although strings are immutable so the .ToLower() comparison will create a new copy of the string for each if statement. We could just use the string comparison function such as creditCard.Type.Equals("visa", StringComparison.OrdinalIgnoreCase)
5. Constants – we should avoid using magic strings and numbers in code and use constants instead for names such as credit card names. A CreditCardType Enum could be an alternative if the credit card types are fixed.
6. Readability – as the number of supported credit cards grows, readability can be improved with a switch statement
7. Globalisation – error messages can be placed in resource files to provide multilingual support
8. Performance – the regular expression is created for every credit card number that is validated which can be changed to a singleton. We can use the RegEx compile parameter to improve the execution time.

  
    public class CreditCardValidator : ICreditCardValidator
    {
        private static readonly Regex VisaRegEx = 
                   new Regex("^4[0-9]{6,}$", RegexOptions.Compiled);
        private static readonly Regex MasterCardRegEx = 
                   new Regex("^5[1-5][0-9]{5,}$", RegexOptions.Compiled);

        public void Validate(CreditCard creditCard)
        {
            if (creditCard == null) throw new ArgumentNullException("creditCard");
            if (string.IsNullOrWhiteSpace(creditCard.Type)) 
                   throw new ArgumentException("The card type must be specified.");

            switch (creditCard.Type.ToLower())
            {
                case "visa":
                    if (!VisaRegEx.IsMatch(creditCard.Number)) 
                         throw new InvalidCardException("Invalid card");
                    break;
                case "mastercard":
                    if (!MasterCardRegEx.IsMatch(creditCard.Number)) 
                         throw new InvalidCardException("Invalid card");
                    break;
                default:
                    throw new InvalidCardException(
                         string.Format("Card type {0} is unsupported.", creditCard.Type));
            }
        }
    }

Even though the code was improved, the design is still poor and breaks SOLID principals.

9. Open/Closed Principal - The class might be small but is hard to extend by supporting additional credit cards without modifying the code
10. Single Responsibility Principle - The class knows too much about various credit card types and how to validate them

Summary

The post identified issues with a working solution and discussed fundamental coding practices.

If you can spot more issues, please let me know. If you know how to solve the problem then I’d love you hear from you too.

The next post will discuss how the design can be improved by applying a design pattern.