2
\$\begingroup\$

I recently did a programming exercise for a company which I didn't pass. I didn't get any good feedback from my test so I wanted some advice/opinions for what I did wrong and what I could have done better. I had to refactor this class:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;

namespace App
{
    public class CustomerService
    {
        public bool AddCustomer(string firname, string surname, string email, DateTime dateOfBirth, int companyId)
        {
            if (string.IsNullOrEmpty(firname) || string.IsNullOrEmpty(surname))
            {
                return false;
            }

            if (!email.Contains("@") && !email.Contains("."))
            {
                return false;
            }

            var now = DateTime.Now;
            int age = now.Year - dateOfBirth.Year;
            if (now.Month < dateOfBirth.Month || (now.Month == dateOfBirth.Month && now.Day < dateOfBirth.Day)) age--;

            if (age < 21)
            {
                return false;
            }

            var companyRepository = new CompanyRepository();
            var company = companyRepository.GetById(companyId);

            var customer = new Customer
                               {
                                   Company = company,
                                   DateOfBirth = dateOfBirth,
                                   EmailAddress = email,
                                   Firstname = firname,
                                   Surname = surname
                               };

            if (company.Name == "VeryImportantClient")
            {
                // Skip credit check
                customer.HasCreditLimit = false;
            }
            else if (company.Name == "ImportantClient")
            {
                // Do credit check and double credit limit
                customer.HasCreditLimit = true;
                using (var customerCreditService = new CustomerCreditServiceClient())
                {
                    var creditLimit = customerCreditService.GetCreditLimit(customer.Firstname, customer.Surname, customer.DateOfBirth);
                    creditLimit = creditLimit*2;
                    customer.CreditLimit = creditLimit;
                }
            }
            else
            {
                // Do credit check
                customer.HasCreditLimit = true;
                using (var customerCreditService = new CustomerCreditServiceClient())
                {
                    var creditLimit = customerCreditService.GetCreditLimit(customer.Firstname, customer.Surname, customer.DateOfBirth);
                    customer.CreditLimit = creditLimit;
                }
            }

            if (customer.HasCreditLimit && customer.CreditLimit < 500)
            {
                return false;
            }

            CustomerDataAccess.AddCustomer(customer);

            return true;
        }
    }
}

Also there is the caveat that you cannot change their test harness file:

using System;
using App;

namespace Harness
{
    public class Customer
    {
        public static void ProveAddCustomer(string[] args)
        {

          /*
           *  You MUST NOT change this code. This is an existing consumer of the CustomerService and you must maintain
           *  backwards compatibility.
          */

            var custService = new CustomerService();
            var addResult = custService.AddCustomer("Joe", "Bloggs", "joe.bloggs@adomain.com", new DateTime(1980, 3, 27), 4);
            Console.WriteLine("Adding Joe Bloggs was " + (addResult ? "successful" : "unsuccessful"));
        }
    }
}

This is what I did:

using System;
using System.Text.RegularExpressions;
using App.DataAccess;
using App.Entities;
using App.Repositories;

namespace App.Services
{
    public class CustomerService
    {
        public virtual bool AddCustomer(string firstName, string surname, string email, DateTime dateOfBirth, int companyId)
        {
            // Validate customer fields before going further
            if (!IsValidCustomerHelper(firstName, surname, email, dateOfBirth)) return false;

            var company = GetCustomerCompanyHelper(companyId);

            var customer = new Customer
                               {
                                   Company = company,
                                   DateOfBirth = dateOfBirth,
                                   EmailAddress = email,
                                   Firstname = firstName,
                                   Surname = surname
                               };

            SetCreditValuesHelper(company, customer);
            GetCreditLimitHelper(customer);

            if (customer.HasCreditLimit && customer.CreditLimit < 500)
            {
                return false;
            }

            CustomerDataAccess.AddCustomer(customer);

            return true;
        }

        protected virtual Company GetCustomerCompanyHelper(int companyId)
        {
            var companyRepository = new CompanyRepository();
            var company = companyRepository.GetById(companyId);
            return company;
        }

        protected virtual void GetCreditLimitHelper(Customer customer)
        {
            using (var customerCreditService = new CustomerCreditServiceClient())
            {
                var creditLimit =
                    customerCreditService.GetCreditLimit(customer.Firstname, customer.Surname, customer.DateOfBirth);
                customer.CreditLimit = creditLimit;
            }
        }

        /// <summary>
        /// Set 
        /// </summary>
        /// <param name="company"></param>
        /// <param name="customer"></param>
        protected virtual void SetCreditValuesHelper(Company company, Customer customer)
        {
            if (company.Name == "VeryImportantClient")
            {
                // Skip credit check
                customer.HasCreditLimit = false;
            }
            else if (company.Name == "ImportantClient")
            {
                // Do credit check and double credit limit
                customer.HasCreditLimit = true;
                using (var customerCreditService = new CustomerCreditServiceClient())
                {
                    var creditLimit =
                        customerCreditService.GetCreditLimit(customer.Firstname, customer.Surname, customer.DateOfBirth);
                    creditLimit = creditLimit * 2;
                    customer.CreditLimit = creditLimit;
                }
            }
            else
            {
                // Do credit check
                customer.HasCreditLimit = true;
            }
        }

        protected virtual bool IsValidCustomerHelper(string firstName, string surname, string email, DateTime dateOfBirth)
        {
            if (string.IsNullOrEmpty(firstName) || string.IsNullOrEmpty(surname))
            {
                return false;
            }

            var isValidEmail = Regex.IsMatch(email,
                @"^(?("")("".+?(?<!\\)""@)|(([0-9a-z]((\.(?!\.))|[-!#\$%&'\*\+/=\?\^`\{\}\|~\w])*)(?<=[0-9a-z])@))" +
                @"(?(\[)(\[(\d{1,3}\.){3}\d{1,3}\])|(([0-9a-z][-0-9a-z]*[0-9a-z]*\.)+[a-z0-9][\-a-z0-9]{0,22}[a-z0-9]))$",
                RegexOptions.IgnoreCase);

            // use regular expression because this can break easily
            if (!isValidEmail)
            {
                return false;
            }

            var now = DateTime.Now;
            var age = DateTime.Now.Year - dateOfBirth.Year;
            if (now.Month < dateOfBirth.Month || now.Month == dateOfBirth.Month && now.Day < dateOfBirth.Day) age--;

            return age >= 21;
        }
    }
}

I added a new class that inherits from this class so I could do dependency injection:

using System;
using App.Entities;
using App.Interfaces.Repositories;
using App.Interfaces.Services;
using App.Interfaces.Wrappers;

namespace App.Services
{
    /// <summary>
    /// Refactored CustomerService to CustomerCompanyService
    /// This version has de-coupled dependencies to make it easier to unit test
    /// and swap out implementations via interfaces
    /// </summary>
    public class CustomerCompanyService : CustomerService, ICustomerService
    {

        private readonly ICustomerCreditService _customerCreditService;
        private readonly ICompanyRepository _companyRepository;
        private readonly ICustomerDataAccessWrapper _customerDataAccessWrapper;

        public CustomerCompanyService(ICustomerCreditService customerCreditService, ICompanyRepository companyRepository, ICustomerDataAccessWrapper customerDataAccessWrapper)
        {
            _customerCreditService = customerCreditService;
            _companyRepository = companyRepository;
            _customerDataAccessWrapper = customerDataAccessWrapper;
        }

        public override bool AddCustomer(string firstName, string surname, string email, DateTime dateOfBirth, int companyId)
        {
            // Validate customer fields before going further
            if (!IsValidCustomerHelper(firstName, surname, email, dateOfBirth)) return false;

            var company = GetCustomerCompanyHelper(companyId);

            var customer = new Customer
            {
                Company = company,
                DateOfBirth = dateOfBirth,
                EmailAddress = email,
                Firstname = firstName,
                Surname = surname
            };

            SetCreditValuesHelper(company, customer);
            GetCreditLimitHelper(customer);

            if (customer.HasCreditLimit && customer.CreditLimit < 500)
            {
                return false;
            }

            _customerDataAccessWrapper.AddCustomer(customer);

            return true;
        }

        protected override Company GetCustomerCompanyHelper(int companyId)
        {
            var company = _companyRepository.GetById(companyId);
            return company;
        }

        protected override void GetCreditLimitHelper(Customer customer)
        {
            //TODO: NG - removed using (have to make sure IDisposable is still being used correctly
                var creditLimit =
                    _customerCreditService.GetCreditLimit(customer.Firstname, customer.Surname, customer.DateOfBirth);
                customer.CreditLimit = creditLimit;
        }

        /// <summary>
        /// Refactored SetCreditValuesHelper
        /// Reduced line count
        /// </summary>
        /// <param name="company"></param>
        /// <param name="customer"></param>
        protected override void SetCreditValuesHelper(Company company, Customer customer)
        {
            // Has limit if not VeryImportantClient.
            // Could re-factor further and remove magic string
            customer.HasCreditLimit = company.Name != "VeryImportantClient";

            // Do credit check and double credit limit
            if (!customer.HasCreditLimit) return;
            var creditLimit =
                _customerCreditService.GetCreditLimit(customer.Firstname, customer.Surname, customer.DateOfBirth);
            creditLimit = creditLimit * 2;
            customer.CreditLimit = creditLimit;
        }
    }
}

I also had to make this wrapper class so I could use their static class but also inject it as a dependency:

using App.DataAccess;
using App.Entities;
using App.Interfaces.Wrappers;

namespace App.Wrappers
{
    public class CustomerDataAccessWrapper : ICustomerDataAccessWrapper
    {
        /// <summary>
        /// This is a wrapper so we can mock adding customer without
        /// changing legacy CustomerDataAccess class.
        /// </summary>
        /// <param name="customer"></param>
        public void AddCustomer(Customer customer)
        {
            CustomerDataAccess.AddCustomer(customer);
        }
    }
}

Keep in mind, this test was about refactoring legacy code but also changing the code to where it can be unit testable. This is the full description of the task:

  1. The most common mistakes come from candidates not following the instructions so please make sure you read them clearly and do not divert away from what is being asked

  2. Make sure you don’t “over-engineer” the test and stick to the 2 hour limit, if the test is over engineered you will not be progressed

  3. Ensure that you maintain and apply basic engineering principles such as SOLID, DRY, YAGNI and KISS

The only conclusion I could reach is AddCustomer should return an object with the credit limit so we can write tests around the calculation being correct. I also believe that maybe I should of added a new constructor to CustomerService which would use the dependency injection. That way I wouldn't have to make a second class (CustomerCompanyService).

\$\endgroup\$
  • 1
    \$\begingroup\$ The current question title applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review - Asking Questions for guidance on writing good question titles. \$\endgroup\$ – BCdotWEB Oct 2 '18 at 14:36
  • \$\begingroup\$ When refactoring legacy code always write tests that assess the current behavior first. Otherwise your refactor could cause huge unnoticed damages. That may be a reason that make you fail the exercise. \$\endgroup\$ – Spotted Oct 2 '18 at 14:57
  • \$\begingroup\$ @Spotted Do you think I should of added a second constructor to CustomerService and do my DI there? For better or worse I don't usually write classes with multiple constructors. \$\endgroup\$ – nick gowdy Oct 2 '18 at 15:00
  • \$\begingroup\$ I think your interlocutor may have expected that you would write a bunch of tests assessing the current code before making any change to it. This practice is advised in the book "Working effectively with legacy code" and may be your interlocutor's judgment basis. \$\endgroup\$ – Spotted Oct 2 '18 at 15:12
  • \$\begingroup\$ Could you add the actual instructions mentioned in bullet 1.? \$\endgroup\$ – Henrik Hansen Oct 2 '18 at 17:32
2
\$\begingroup\$

As David has pointed out it's difficult to know what the company actual means by SOLID, DRY, YAGNI and KISS, so the below is just my thoughts about your code:

  public virtual bool AddCustomer(string firstName, string surname, string email, DateTime dateOfBirth, int companyId)
  {
    // Validate customer fields before going further
    if (!IsValidCustomerHelper(firstName, surname, email, dateOfBirth)) return false;

    var company = GetCustomerCompanyHelper(companyId);

    var customer = new Customer
    {
      Company = company,
      DateOfBirth = dateOfBirth,
      EmailAddress = email,
      Firstname = firstName,
      Surname = surname
    };

    SetCreditValuesHelper(company, customer);
    GetCreditLimitHelper(customer);

    if (customer.HasCreditLimit && customer.CreditLimit < 500)
    {
      return false;
    }

    CustomerDataAccess.AddCustomer(customer);

    return true;
  }

At first sight you manage to clean the code up to some extend. But you keep the creation of the Customer inside the main method - but make a nice method to create the company in. I would make a CreateCustomer(...) method in order to keep the main method clean (see below).

In the same way: you have the nice helper methods SetCreditValuesHelper and GetCreditLimitHelper but then you make the credit limit check in the main method. I would handle all that in one method.

Names: Why do you add "Helper" to all your private methods, for instance: SetCreditValuesHelper(). IMO it is redundant and disturbing.

So - again IMO - your refactoring of AddCustomer() still violates both SOLID and KISS

A gentle but yet efficient refactoring of the AddCustomer() could be:

public bool AddCustomer(string firname, string surname, string email, DateTime dateOfBirth, int companyId)
{
  try
  {
    if (!CheckName(firname) || !CheckName(surname) || !CheckEmail(email) || !CheckAge(dateOfBirth))
    {
      return false;
    }

    var company = GetCompany(companyId);
    if (company == null)
    {
      return false;
    }

    var customer = CreateCustomer(firname, surname, email, dateOfBirth, company);
    if (customer == null)
    {
      return false;
    }

    if (!HandleCredit(customer, company))
    {
      return false;
    }

    CustomerDataAccess.AddCustomer(customer);

    return true;
  }
  catch (Exception)
  {

  }

  return false;
}

Here I've encapsulated everything in a try..catch because we don't know what CompanyRepository or CustomerCreditServiceClient is up to, when it comes to exceptions, and the existing client doesn't handle exceptions.

And the helper functions :

private bool CheckName(string name)
{
  return !string.IsNullOrWhiteSpace(name);
}

private bool CheckEmail(string email)
{
  return !string.IsNullOrWhiteSpace(email) && email.Contains("@") && email.Contains(".");
}

private bool CheckAge(DateTime birthDate)
{
  var now = DateTime.Now;
  int age = now.Year - birthDate.Year;
  if (now.Month < birthDate.Month || (now.Month == birthDate.Month && now.Day < birthDate.Day))
  {
    age--;
  }

  return age >= 21;
}

private Company GetCompany(int companyId)
{
  var companyRepository = new CompanyRepository();
  return companyRepository.GetById(companyId);
}

private Customer CreateCustomer(string firname, string surname, string email, DateTime dateOfBirth, Company company)
{
  return new Customer
  {
    Company = company,
    DateOfBirth = dateOfBirth,
    EmailAddress = email,
    Firstname = firname,
    Surname = surname
  };
}

private bool HandleCredit(Customer customer, Company company)
{
  if (company.Name == "VeryImportantClient")
  {
    // Skip credit check
    customer.HasCreditLimit = false;
  }
  else
  {
    customer.HasCreditLimit = true;
    using (var customerCreditService = new CustomerCreditServiceClient())
    {
      customer.CreditLimit = customerCreditService.GetCreditLimit(customer.Firstname, customer.Surname, customer.DateOfBirth);
      customer.CreditLimit = company.Name == "ImportantClient" ? customer.CreditLimit * 2 : customer.CreditLimit;
    }
  }

  return !customer.HasCreditLimit || customer.HasCreditLimit && customer.CreditLimit >= 500;
}

You are right, it is difficult to meaningfully unit test a method just returning true/false, and it seems that the existing client doesn't even accept some well chosen exceptions either. Therefore your DI-idea seems to be a good idea - in order to be able to test each action separately.

The idea of derive from CustomerService is good because you then meet the O in SOLID.

But you actually make some bugs:

  public class CustomerCompanyService : CustomerService, ICustomerService
  {
    protected override void SetCreditValuesHelper(Company company, Customer customer)
    {
        // Has limit if not VeryImportantClient.
        // Could re-factor further and remove magic string
        customer.HasCreditLimit = company.Name != "VeryImportantClient";

        // Do credit check and double credit limit
        if (!customer.HasCreditLimit) return;
        var creditLimit =
            _customerCreditService.GetCreditLimit(customer.Firstname, customer.Surname, customer.DateOfBirth);
        creditLimit = creditLimit * 2;
        customer.CreditLimit = creditLimit;
    }
  }

What about company.Name != "ImportantClient"?

(By the way: having company names as "VeryImportantClient" and "ImportantClient" seems to be candidates for trouble, and you should maybe have suggested an enum property instead).

And by subclassing you violate the DRY principle to some extent, and maybe also YAGNI? And you won't actually be able to unit test the original code.

Instead of subclassing, I think I would stick to the extra constructor idea mentioned by David Arno in a way like this:

  public interface ICustomerProvider
  {
    // Should throw appropriate exceptions
    Customer GetCustomer(string firstName, string lastName, string email, DateTime birthDate, Company company);
  }

  public interface ICompanyProvider
  {
    // Should throw appropriate exceptions
    Company GetCompany(int companyId);
  }

  public interface ICreditHandler
  {
    // Should throw appropriate exceptions
    void SetCredit(Customer customer);
  }

  public class CustomerService
  {
    public CustomerDataAccess CustomerDataAccess { get; private set; }
    private ICustomerProvider _customerProvider;
    private ICompanyProvider _companyProvider;
    private ICreditHandler _creditHandler;

    public CustomerService()
    {
      // TODO instantiate some default implementations:
      //_customerProvider = new DefaultCustomerProvider();
      //_companyProvider = new DefaultCompanyProvider();
      //_creditHandler = new DefaultCreditHandler();
    }

    public CustomerService(ICustomerProvider customerProvider, ICompanyProvider companyProvider, ICreditHandler creditHandler)
    {
      _customerProvider = customerProvider;
      _companyProvider = companyProvider;
      _creditHandler = creditHandler;
    }

    public Customer CreateAndAddCustomer(string firname, string surname, string email, DateTime dateOfBirth, int companyId)
    {
      Company company = _companyProvider.GetCompany(companyId);
      if (company == null)
      {
        throw new InvalidOperationException($"Unable to find the company with Id: {companyId}");
      }

      Customer customer = _customerProvider.GetCustomer(firname, surname, email, dateOfBirth, company);
      if (customer == null)
      {
        throw new InvalidOperationException($"Unable to create the customer: {firname} {surname}");
      }

      _creditHandler.SetCredit(customer);

      CustomerDataAccess.AddCustomer(customer);

      return customer;
    }

    public bool AddCustomer(string firname, string surname, string email, DateTime dateOfBirth, int companyId)
    {
      try
      {
        //CreateAndAddCustomer(firname, surname, email, dateOfBirth, companyId);
        //return true;
        return CreateAndAddCustomer(firname, surname, email, dateOfBirth, companyId) != null;
      }
      catch (Exception)
      {

      }

      return false;
    }

Here the default providers/handler in the parameterless constructor can use the original existing code, and the DI-constructor makes it easy to customize the behavior. CreateAndAddCustomer(...) is well suited for a test environment. The interfaces make it easy to test each step or action independently.

Some would claim that adding the DI-constructor, one violates the open/close-principle, but I think it's doable because we extent but don't modify the existing interface.

If all that is "over-engineering" in the way the company interprets the term, we will probably never know?

\$\endgroup\$
  • 2
    \$\begingroup\$ They probably define over-engineering as everything we don't understand :-P \$\endgroup\$ – t3chb0t Oct 3 '18 at 6:00
1
\$\begingroup\$

Offering useful feedback is tricky. It's so dependent on what the company setting the test thinks is "over engineering" and what they mean by the casual statement "Ensure that you maintain and apply basic engineering principles such as SOLID, DRY, YAGNI and KISS". It's quite likely for example that they viewed your efforts to introduce DI as over-engineering. But maybe they wanted you to do that. Only they know.

Rather than using inheritance, I'd personally have added a default constructor to CustomerService that used the current services (to maintain backward compatibility) and another that injected such services, which is something you suggest too at the end of your question.

Also, I'd not have added the regex for the email checking. The only rule that applies 100% to all emails is that it must contain an @. There are no other rules that apply to every email in existence, so your regex is guaranteed to fail for a real email address at some stage.

But really, apart from those two points, there's nothing obviously wrong with your code. The company just has an expectation and you failed to "magically" anticipate that expectation. So don't be disheartened, they just weren't the company for you.

I would chase them for feedback though. Email them, phone them etc and ask directly what they thought was wrong with your submission. Unless they are incredibly rude and disinterested (in which case, did you even want to work there?), they should provide some feedback.

\$\endgroup\$
  • \$\begingroup\$ I am inclined to agree with you. After doing the test, I considered that the return type for AddCustomer shouldn't be a boolean. The reason being if we decided to test the method using something like BDD, our only success condition is "does it return true". Where are if we return an object with the credit limit expected from the business rule, we can say "does it return true", "is the credit limit correct based on the client type". Thanks for your feedback. \$\endgroup\$ – nick gowdy Oct 2 '18 at 14:57
  • \$\begingroup\$ @nickgowdy, I'd agree that changing the method from returning a bool to something that indicates what was in error makes sense. But they tied your hands there by insisting that their laughable attempt at a test harness not be modified, which requires a bool result. So I don't think you could have improved it in that area without breaking their rules. \$\endgroup\$ – David Arno Oct 2 '18 at 15:09
0
\$\begingroup\$

Summary of the original code

Let's go over the original code first. Here's a summary of what AddCustomer does:

  • It performs a few basic argument checks (names not being empty, email appearing valid).
  • It also performs some business-specific checks (age < 21, credit limit < 500).
  • It obtains a company object, which is used to determine the credit limit of the new customer.
  • It creates a customer object and adds it to a database.

Problems with the original code

In order of most important to least, here's what I think is troublesome:

  • CustomerDataAccess.AddCustomer looks like a property access, but it's actually a static method in a static class. Because globals are so 'conveniently' accessible, they tend to result in many more (hidden) dependencies between components. Ultimately, this makes a code-base increasingly more difficult to understand, maintain and test.
  • Creating a temporary CompanyRepository and CustomerCreditServiceClient object for a single operation looks very strange. Such dependencies should be injected via the CustomerService constructor. The test harness prevents the constructor from being changed, but it's still possible to provide an overload that uses injection (the original constructor can create its own dependencies, similar to the old behavior).
  • Those repositories, service-clients and data-access classes are concrete types, which makes it difficult to replace them with an alternate implementation (such as a mock object, for testing). Creating interfaces for them seems appropriate here.
  • There's a bit of code duplication in the credit-limit part, which shouldn't be too difficult to simplify. What's worse is those hard-coded company-specific rules. Solving that requires more knowledge about the rest of the system, but it looks like this information should either be stored in Company (company.UseCreditLimit, company.CreditLimitFactor) or CustomerCreditServiceClient should take the company into account.
  • It looks like the original code didn't have any documentation. That makes it difficult to determine whether the lack of exception handling is intentional or not. Those business-specific checks (age, credit limit) would also benefit from some documentation.
  • The code doesn't check whether companyId refers to an existing company.
  • Returning a boolean doesn't let the caller know why adding a customer failed, nor does it give the caller access to the newly created customer (via some kind of ID). Maybe that's because callers don't need to know, or maybe they're obtaining this information in some other way (which probably hints at system-wide code duplication).
  • Strangely, Customer has an Id property, but it's never used. Looks like an oversight in this exercise. Also, it could use a proper constructor.
  • Determining the age of the new customer takes up quite a bit of space, relatively speaking. An int GetCurrentAge(DateTime birthDate) helper method would be useful.

Analysis of your changes

Let's now take a look at your changes. Your CustomerCompanyService class is using dependency injection and a wrapper around that static database method, which solves the most important problems. AddCustomer is now much easier to test. However:

  • Using inheritance here is actually somewhat problematic:
    • The 'test harness' creates a CustomerService instance, so your child class won't be used there anyway.
    • Using virtual methods to inject dependencies is a good idea, but it's not executed properly: by mixing other logic into these methods, your child class has to duplicate that logic. So not only do you end up with more code that needs to be maintained, you're also not able to test the code that is actually being used (the base class).
  • Your child class duplicates the AddCustomer method of its parent, making the problem of code duplication worse. Another virtual method for adding the customer to the database could've prevented that.
  • Combining the creation of a credit service object with additional credit limit logic causes further code duplication. A virtual method for obtaining a credit limit service could've prevented some more duplication. Alternately you could've created a helper method to obtain the credit limit itself: int GetCreditLimit(Customer customer, Company company).
  • Splitting methods up into smaller ones is a useful technique, but:
    • It has to be done properly. Splitting the credit-limit logic up into two methods makes it more difficult to see that you introduced a bug (the Get method overrides the double credit limit that was set in Set). Fixing that will expose an additional bug in the child class: its Set method doubles the credit limit of every customer with a limit. I see no reason why you need two methods instead of one.
    • It must not be taken too far, otherwise it'll make code harder to understand. The various argument checks are fairly simple and don't take up a lot of space, so leaving them inside AddCustomer lets you immediately see what's going on. I would only move them to a separate method if the validation is more complicated.
  • That email regex is a bad idea. It's most likely going to fail for some exotic addresses, and perhaps more importantly: it's not doing what the original code did, so this might be introducing bugs as well. Not to mention the readability of the regex itself (and thus our ability to verify that it works as intended)... ;)
  • Try using more accurate names. GetCreditLimitHelper implies that it'll return a credit-limit helper object, but that's not what it's doing. SetCreditLimits would be a more descriptive name (that Helper suffix is more confusing than, well, helpful).
  • Regarding disposal, your class may need to implement IDisposable as well, depending on whether it should take ownership of the given dependency. That's difficult to determine without further context.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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