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.

Christopher Francisco
  • 2,392
  • 1
  • 12
  • 27
  • 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 '17 at 06: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"). – Christopher Francisco Sep 14 '17 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 '17 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). – Christopher Francisco Sep 15 '17 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 '17 at 00:41
  • In cases where you definitely will extend your code, in a predictable way, of course it's appropriate to design for that. For example, if you are writing a game, and you want to handle collision detection between units, it's pretty bloody likely you'll be adding new units in the future. But it's hard to know in most cases how you want extensibility. For example, most of the answers here seem to assume that repositories know which namespaces they can process, and they can each process 1 or more namespaces. What if it's the other way around ie namespaces know which repos they can be process'd by – Miles Rout Sep 17 '17 at 00:46

4 Answers4

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)
Doc Brown
  • 199,015
  • 33
  • 367
  • 565
  • 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. – Christopher Francisco Sep 05 '17 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 05 '17 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. – Christopher Francisco Sep 14 '17 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).

Emerson Cardoso
  • 2,050
  • 7
  • 14
  • This looks exactly like what DocBrown already suggested in [his answer](https://softwareengineering.stackexchange.com/a/356875/187318) – Hulk Sep 07 '17 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. – Emerson Cardoso Sep 08 '17 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.

JimmyJames
  • 24,682
  • 2
  • 50
  • 92
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.

Timothy Truckle
  • 2,336
  • 9
  • 12