Refactor chunks of code where variables from one chunk are used in others

Question

What is a good pattern to refactor a chunk of code where you can identify smaller chunks, but the results of these smaller chunks are used in other chunks?

An example should make things more clear. Let's say you have something like this (reduced in size to make things easier):

public Response CreateCustomer(CreateCustomerDTO input)
{
    var customerGroup = GetCustomerGroup(input.GroupId);
    if (customerGroup == null)
    {
        return new InvalidCustomerGroupResponse();
    }

    if (customerGroup.HasReachedMaxAmountOfCustomers()
    {
        return new InvalidCustomerGroupResponse();
    }

    var segment = SegmentService.GetSegment(input.SegmentId);
    if (!segment.CanAcceptCustomer(input.Country))
    {
        return new CannotAddCustomerToSegmentResponse();
    }

    // More segment checks

    CustomerService.CreateCustomer(input.Name, input.Country, customerGroup, segment);
    return new SuccessResponse();
}

You might find all sorts of things can be improved in this code, but I totally made it up. The key thing is to demonstrate a structure. Plus, the real code is many times longer. We can identify separate chunks. Ideally, I'd want to refactor it into something like this:

public Response CreateCustomer(CreateCustomerDTO input)
{
    CheckGroup(input);
    CheckSegment(input);
    CustomerService.CreateCustomer(input.Name, input.Country, customerGroup, segment);
    return new SuccessResponse();
}

But of course, the chunk that does the checks also retrieves some data that the subsequent chunks need. So I end up with something like this:

public Response CreateCustomer(CreateCustomerDTO input)
{
    var customerCheckResponseCode = CheckCustomer(input.GroupId);
    if (customerCheckResponseCode != ResponseCode.OK)
    {
        return new InvalidCustomerGroupResponse();
    }

    var segmentCheckResponseCode = CheckSegment(input.SegmentId);
    if (segmentCheckResponseCode != ResponseCode.OK)
    {
        return new CannotAddCustomerToSegmentResponse();
    }

    CustomerService.CreateCustomer(input.Name, input.Country, customerGroup, segment);
    return new SuccessResponse();
}

This is only marginally better. It's readable here, but in my real code, there are many lines I could group into chunks. Things I can logically group, but I can't hide everything in separate methods because some variables I need in other logical groups as well. So I can't refactor out all those if checks for example. Unless someone knows a better pattern for this problem?

Answer 1

Use exceptions, they are exactly made for this purpose. It is quite unlikely you will perceive any measurable performance issues (and when, then it is early enough to think about resolving them):

public void CreateCustomer(CreateCustomerDTO input)
{
    // can throw InvalidCustomerGroupException
    var customerGroup = GetCustomerGroup(input.GroupId);

    // can throw CannotAddCustomerToSegmentException
    var segment = SegmentService.GetSegment(input.SegmentId);

    // might also throw some kind of CustomerCreateException 
    CustomerService.CreateCustomer(input.Name, input.Country, customerGroup, segment);
}

In case all possible exceptions are derived from CustomerCreateException, code will look like this.

try
{
      CreateCustomer(input);
}
catch(CustomerCreateException ex)  
{
      // do some error handling. If you really need a response object,
      // make it an attribute of CustomerCreateException and 
      //
      // return ex.Response
}

Your comment about exceptions shows you confused "don't use exceptions for standard flow of control, only for exceptional situations" with "don't use exceptions". This makes no sense, why should we have exceptions at all if this tool would not be useful for situations like this one?

Answer 2

Generics

With generic types you can create a standardized return type with "success" and "error" variables

var result = someBlock();
if (result.hasError)
  return result.error()

var something = result.success()
...

That would allow you to move all the detailed logic down, so the top level method becomes totally boiler plate. Your code is still going to be verbose, but its likely you won't make many mistakes with such rigid success/error syntax.

If your code is very linear:

var a = calcA()
var b = calcB(a)
var c = calcC(b)

You might be able to do something like building a list of function pointers then simply walk down the list calling each method and passing the result to the next one - you could combine this with the "result" type I started this post with, so you can break out early.

Alternatively if you add a "map" method (that only maps successful values) to your Result class, you can then do something like:

calcA().map(calcB).map(calcC)

I grabbed the "map" idea from Kotlin:

https://kotlinlang.org/api/latest/jvm/stdlib/kotlin/map.html

Syntactic Sugar

Doing much more than that is probably going to require syntactic sugar from your language, for example:

• Exception handling so that you can break out early without needing to do a return.
• Old school C with "goto error_hander;"
• "Truthy" support (Python/Ruby etc) where you can put the assignment in an if and then return.
• Kotlins "elvis" operator, where null can be used as a special value to break out of a block.
• Some kind of operator overloading
• Any language that can create a DSL