5

I'm having major troubles trying to get rid of this switch statement. To put a little of context first, I'm working with asynchronous batch operations. These operations can be applied to any entity in the system. You just have to send a different namespace.

Example:

"jobs": [
{
    namespace: "client",
    request: {"id": 10, "name": "john"}
}, 
{
    namespace: "invoice",
    request: {"id": 99, "number": "F20170101"}
}]

After I send those jobs, the service returns data similar, but with a response key like the following:

results: [
{
    namespace: "client",
    request: {id: 10, name: "john"},
    response: {success: false}
}, 
{
    namespace: "invoice",
    request: {id: 99, number: "F20170101"},
    response: {success: true, s_id: 3310}
}]

Now here comes the funny part

I have to process the response, and based on the namespace, I have to select the appropriate repository to update our database records. So far I have this smelly thing:

if (namespace == 'client') {
    customerRepository.doSomething()
} elseif (namespace == 'invoice') {
    invoiceRepository.doSomethingTotallyDifferent(withDiffParams)
}

Now this clearly breaks Open/Close principle, and I can't figure out how to solve this by removing the switch (actually a if/elseif spaghetti thingy, but it's the same). Thanks in advance.

PS: this is not a specific language question, so please refrain to provide answers that applies only to a specific language.

6
  • Does the code currently work? If so, don't replace a couple of lines of if-elif-else with dozens of lines of inheritance boilerplate that expresses the same thing but with a ton of indirection.
    – Miles Rout
    Sep 13, 2017 at 6:28
  • @MilesRout problem is, if I do as you say, it'd be breaking Open/Closed. The consequence for breaking it is having to rewrite test cases for every "extended" namespace, even though the specification hasn't changed (The spec for this class is "process the response"). Sep 14, 2017 at 17:57
  • The 'open/closed principle' is just a meaningless buzzword. The 'consequence of breaking it' is that your code looks better, reads better and more direct.
    – Miles Rout
    Sep 14, 2017 at 23:35
  • I respect your point of view, but I disagree with it. Open/close principle makes your code extensible, without having to modify the already "closed" functionality. In other words, you get to call a piece of code "completed" and keep extending it's functionality (as long as the specification remains, of course). Sep 15, 2017 at 16:29
  • In theory, it does. In practice, it does sometimes. However often there are two other important factors to consider in practice: firstly, often you don't actually need extensibility, and can add it when you need it, and secondly, it's hard to predict how you will extend your code in future. Yes, maybe you need to extend it later, but in a totally different way.
    – Miles Rout
    Sep 17, 2017 at 0:41

4 Answers 4

5

Your repositories should be derived from a common base class, lets call it baseRepository. This class needs to provide two virtual methods

  • correspondingNamespace()

  • processReponse(responseKey)

Overide the methods in the derived classes, so correspondingNamespace should return the correct namespace for each repo, for example "client" for customerRepository. processReponse should delegate to the individual business logic like calling doSomething or calling doSomethingTotallyDifferent with different parameters.

Now you can easily initialize a dictionary with the available repositories beforehand, in pseudo code:

for each repo in [customerRepository, invoiceRepository]:
     repoDict[repo.correspondingNamespace()]=repo

And the response processing will look (roughly) like:

  repoDict[responseKey.namespace].processReponse(responseKey)

To your comment: if you do not want to add these methods to the repos directly, you can use a separate hierarchy of strategy classes and place the two methods there. Derive customerResponseStrategy and invoiceResponseStrategy from responseStrategy, and let each responseStrategy hold a reference to the corresponding repo. That will lead to something like

 for each respStrat in 
        [new customerResponseStrategy(customerRepository),
         new invoiceResponseStrategy(invoiceRepository)]:
         responseStrategyDict[respStrat .correspondingNamespace()]=respStrat 

and

  responseStrategyDict[responseKey.namespace].processReponse(responseKey)
3
  • 1
    My repositories have no business logic in common, so making them childs of a superclass just to proxy some calls feels smelly, and could be fixed with composition over inheritance. However, the use of a dictionary sounds feasible, maybe I could use one to map some interface's implementation, key being the namespace. Sep 5, 2017 at 20:17
  • @ChristopherFrancisco: see my edit. Note using strategy is a solution using inheritance (correctly), not a solution using composition only (though it avoids to use inheritance for your repos).
    – Doc Brown
    Sep 5, 2017 at 20:27
  • I'm marking this one as the correct answer because of the strategy pattern solution. I like EmersonCardoso solution, but since for this particular question I need to encapsulate the business logic of processing based on the namespace, a dictionary fits better. Sep 14, 2017 at 18:03
3

What I've done plenty of times in projects, and that helped a LOT with new feature requests was:

interface JobResponseWorker {
    bool canProcess(namespace);
    void process(response);
}

class ClientResponseWorker implements JobResponseWorker {

    bool canProcess(namespace) {
        return namespace == "client"
    }
    void process(response) {
        //... whatever you want to do here
    }
}

class InvoiceResponseWorker implements JobResponseWorker {
    bool canProcess(namespace) {
        return namespace == "invoice"
    }
    void process(response) {
        //... whatever you want to do here
    }
}

workers = [new ClientResponseWorker(), new InvoiceResponseWorker()]

//your main logic
void processResponse(namespace, request, response) {

    for (JobResponseWorker worker in workers) {
        if (worker.canProcess(namespace)){
            worker.process(response)
        }
    }
}

Like this, most of your main code is closed for modifications. For new workers, you Add new code (open for extension), and then only adds another object into an array (no additional logic in old code).

2
  • This looks exactly like what DocBrown already suggested in his answer
    – Hulk
    Sep 7, 2017 at 10:39
  • It's very similar, but in my approach you could use more than one Worker to process the same response. I needed this approach once, where I had different workers and within the response there were parts that could be processed by multiple workers. With the dictionary approach you bind one worker (that Doc Brown calls repository instead) to one response. That's the difference, and why I posted my answer. Sep 8, 2017 at 12:09
0

I think what you are looking for here is the kind of approach described by the Strategy pattern. In a nutshell you need to translate your namespaces to classes/objects. In your example you would have a Client class and a Invoice class. Each would have a doTheThing() method.

What remains is that you need to have some sort of switch or map that determines which type you should be using. This might seem somewhat pointless in a simple example like this but as the number of operations and classes increases, it will greatly simplify both the code and make it much more understandable. You will also be more able to add new types without touching any of the existing logic for other types.

1
  • This approach seems like it's the way to go. The dictionary might be the key. Sep 5, 2017 at 20:18
0

As long as you have only one place in your code where you have to do this discrimination you should stay with it. But if you have more places where you need to apply different behavior upon the namespace value you should go for the "polymorphic" approach.

The problem is that with a "polymorphic" approach you have to do the discrimination too because you have to select the implementation that belongs to your namespace value at some point.

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.