1

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?

5
  • Bailing the happy path early is exactly what exceptions were invented for. Have you considered using those, instead of returning error-like objects like InvalidCustomerGroupResponse?
    – Alexander
    Mar 27 at 11:32
  • Good point. I thought about this, but was taught that exception-driven development is bad because if performance drops in the exception handler in .NET. Though that might not be a thing any more. It's definitely worth looking into.
    – Peter
    Mar 27 at 13:08
  • 1
    Stack-unwinding is expensive, yes, but that's only really a concern if it's happening all the time, particularly in real-time or high throughput systems. That's overwhelmingly not the case. Realistically, how often is your your system creating new Customers? Even if it ran that a million times a second, you'd exhaust the entire human population in under 3 hours lol
    – Alexander
    Mar 27 at 14:09
  • I miss the code which initializes customerGroup and segment in your final code snippet. Forget something?
    – Doc Brown
    Mar 28 at 10:55
  • @DocBrown true, but it's not super relevant here. The idea was to show how my code is currently structured. In the end, I went with the exception approach you proposed. My code is now a lot cleaner.
    – Peter
    Apr 6 at 7:46

2 Answers 2

2

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?

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

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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