CODEWITHSUNDEEP
c#design-patterns
developer9969
developer9969

Reputation: 5236

Remove if statement and make it more readable

Hi wondering if there is a better way to remove this else if statement and use a strategy pattern. any ideas?

public async Task<TResponse> HandleResponseAsync<TRequest, TResponse>(TRequest item)
    where TResponse : class, new()
{
    TResponse result = default(TResponse);
    Type currentResponseType = typeof(TResponse);   

    if (currentResponseType == typeof(MyResponseA))
    {
        result = //dosomething
    }
    else if (currentResponseType == typeof(MyResponseB))
    { 
        result = //dosomething
    }
    else if (currentResponseType == typeof(MyResponseC))
    {
        result = //dosomething
    }

    return result;
}

Upvotes: 0

Views: 267

Answers (3)

Eric Lippert
Eric Lippert

Reputation: 660513

If you're switching on the type of a generic, you're probably doing something wrong. Generics should be generic. The code you've written is not generic, it's extremely fragile, and generics make it more, not less complicated.

Here, let me rewrite that for you:

if (currentResponseType == typeof(MyResponseA))
    return (TResponse) HandleResponseA(item);
else if (currentResponseType == typeof(MyResponseB))
    return (TResponse) HandleResponseB(item);
else if (currentResponseType == typeof(MyResponseC))
    return (TResponse) HandleResponseC(item);

If there are three possible code paths through the method, no more, no less, then simply write three methods HandleResponseA, HandleResponseB, HandleResponseC. That's what you did already anyways, you just put the body of each of those methods as the body of an if. There's no savings here in making them all one method; you've just made a fragile, over-complicated method. Now that we've refactored your method, we see that we can delete the method entirely and simply call the appropriate handler directly.

Also, consider your poor callers! At the call site there's no compelling reason to prefer HandleResponseAsync<Giraffe, MyResponseB>(giraffe) to the much better HandleResponseB(giraffe); the call site is shorter, clearer, and safer if there are three methods instead of one.

Moreover, if the service provided here depends 100% on the type of the response object, then why isn't this a concern of the response object? Why isn't the call site actually MyResponseB.Handle(giraffe)? The methods could be static methods of the response type. If you're going to write three method bodies, one for each type, then put them on that type.

Upvotes: 5

Mahdi
Mahdi

Reputation: 1827

in c# 7 you can use switch

       var response = new TRsponse();
        switch (response)
        {
            case MYResponseB b:
                break;

            case MYResponse a:
                break;

         }

We’re generalizing the switch statement so that: 1. You can switch on any type (not just primitive types) 2.Patterns can be used in case clauses 3.Case clauses can have additional conditions on them

here are new features in c# 7:

https://blogs.msdn.microsoft.com/dotnet/2016/08/24/whats-new-in-csharp-7-0/

Upvotes: 1

Sergey Kalinichenko
Sergey Kalinichenko

Reputation: 727067

Using specific subclasses inside a generic method is a very dangerous approach, because it limits future flexibility by locking you up into the list of classes inside the generic method. It also throws away most of the advantages that you could get from making it a generic method.

Since you already require a no-argument constructor, one approach is to give your TResponse an interface, and use that interface to call back into the instance to make proper result:

interface IResponse {
    void Configure(); // Use custom parameters if necessary
}

public async Task<TResponse> HandleResponseAsync<TRequest, TResponse>(TRequest item)  where TResponse : class, new(), IResponse {
    TResponse result = new TResponse();
    result.Configure(...);
}

Now the code from if-then-else blocks would go into Configure methods of the corresponding IResponse implementations:

class MyResponseA : IResponse {
    public MyResponseA() {}
    public void Configure() {
        // dosomething from the first IF
    }
}
class MyResponseB : IResponse {
    public MyResponseB() {}
    public void Configure() {
        // dosomething from the second IF
    }
}

Note: Things may get tricky if different instances require different information from the HandleResponseAsync to be passed to them. In this case you should also create an object that holds all information that any of the implementations may need in order to complete its Configure method.

Upvotes: 7

Related Questions