14
\$\begingroup\$

I have my code which makes a web service call based on a type of request:

public class Client
{
    IRequest request;


    public Client(string requestType)
    {
        request = new EnrolmentRequest();
        if (requestType == "Enrol")
        {
            request.DoEnrolment();
        }
        else if (requestType == "ReEnrol")
        {
            request.DoReEnrolment();
        }
        else if (requestType == "DeleteEnrolment")
        {
            request.DeleteEnrolment();
        }
        else if (requestType == "UpdateEnrolment")
        {
            request.UpdateEnrolment();
        }
    }

}

So as per open close principle, I can subclass like:

Class EnrolmentRequest:IRequest
{
    CallService();
}
Class ReEnrolmentRequest:IRequest
{
    CallService();
}
Class UpdateEnrolmentRequest:IRequest
{
    CallService();
}

Now my client class will look something like this:

public class Client
{
    public Client(string requestType)
    {
        IRequest request;

        if (requestType == "Enrol")
        {
            request = new EnrolmentRequest();
            request.CallService();
        }
        else if (requestType == "ReEnrol")
        {
            request = new REnrolmentRequest();
            request.CallService();
        }
        else if (requestType == "DeleteEnrolment")
        {
            request = new UpdateEnrolmentRequest();
            request.CallService();
        }
        else if (requestType == "UpdateEnrolment")
        {
            request = new UpdateEnrolmentRequest();
            request.CallService();
        }
    }

}

Now, I still have to use if and else, and will have to change my code if there are any new request type. So, it's definitely, not closed to modification.

Am I missing anything with respect to SOLID?

\$\endgroup\$
  • \$\begingroup\$ @t3chb0t, Yes , I just typed it in Notepad++ . Will refactor them, Thanks \$\endgroup\$ – Simsons Feb 15 '17 at 11:09
  • 1
    \$\begingroup\$ Now I like it. Thank you for your cooperation ;-) \$\endgroup\$ – t3chb0t Feb 15 '17 at 11:32
  • 1
    \$\begingroup\$ Minor nits not related to SOLID principles... I would create an enum for requestType. I would also turn those if-elseif chains into switches. The repeated request.CallService() in each if-elseif chain can be pulled out to the bottom of the Client constructor so that it's not repeated. \$\endgroup\$ – JeffC Feb 15 '17 at 16:22
  • \$\begingroup\$ (sigh) This is such a nitpick, but the correct spelling of "enrollment" has two "l"s. \$\endgroup\$ – Greg Burghardt Feb 16 '17 at 13:28
9
\$\begingroup\$

It would be better to give the Irequest interface an additional "Name" property that returns the value of requestType. Than you can iterate over all Irequest implementations and get the first with the corresponding name ;).

Example:

interface IRequest
{
    void CallService();
    string Name { get; }
}
class EnrolmentRequest : IRequest
{
    public void CallService() { /* impl. */ }
    public string Name => "Enrol";

}
class ReEnrolmentRequest : IRequest
{
    public void CallService() { /* impl. */ }
    public string Name => "ReEnrol";
}

...

public class Client
{
    private static IRequest[] requestHandlers = new IRequest[] 
    {
        new EnrolmentRequest(),
        new ReEnrolmentRequest(),
        ...
    }

    public Client(string requestType)
    {
        IRequest request = requestHandlers.SingleOrDefault(x => x.Name == requestType);
        if (request == null) { /* Handle null case */ }

        request.CallService();
    }    
}

That code fulfills the open/closed principle because you can add new request handlers without modified existing logic.

\$\endgroup\$
  • \$\begingroup\$ couple of nits: you're missing 'string' in the property Name, and it's requestHandlers is of type IRequest[] not IRequest \$\endgroup\$ – peval27 Feb 15 '17 at 13:16
  • \$\begingroup\$ Instead of FirstOrDefaultI would consider using SingleOrDefault because each implementation of IRequest should have a unique name. SingleOrDefault throws an exception if more than one element satisfies the condition. \$\endgroup\$ – Danko Durbić Feb 15 '17 at 16:17
  • \$\begingroup\$ In C# 6.0 you can also do public string Name { get; } = "ReEnrol"; \$\endgroup\$ – Nazar554 Feb 15 '17 at 17:25
  • \$\begingroup\$ @Nazar554: Yes, that is another solution which uses a backing field internally, but i prefer the public string Name => "ReEnrol"; notation which just creates a property that returns the string. \$\endgroup\$ – JanDotNet Feb 16 '17 at 15:22
6
\$\begingroup\$

As pointed out, you still need to have some sort of if/else, switch, map, etc to decide which class to create for the given requestType. However this doesn't need to be Client's responsibility. You can have a factory that encapsulates that logic, e.g.

class RequestFactory
{
    public IRequest CreateRequest(string requestType)
    {
        switch (requestType)
        {
            case "Enrol":
                return new EnrolmentRequest();
            case "ReEnrol":
                return new REnrolmentRequest();
            //... etc
        }
    }
}

Then Client can be restructured like so:

class Client
{       
    public Client(Requestfactory factory, string requestType)
    {
        if (factory == null)
        {
            throw new ArgumentNullException(nameof(factory))
        }

        var request = factory.CreateRequest(requestType);
        request.CallService();
    }
}

And now every time you need to add a new request type, you just change the factory method and create a new class for that type, and the rest remains the same.

\$\endgroup\$
  • 1
    \$\begingroup\$ great answer - the only thing I'd change is not do the work in the constructor - supply only the factory, and then pass requestType as a parameter to an "Execute" method or something which invokes the factory and calls the service. \$\endgroup\$ – Stephen Byrne Feb 15 '17 at 21:42
  • \$\begingroup\$ @StephenByrne I agree about not doing the work in the constructor, I simply did/left it there as that's where it was in the original code. \$\endgroup\$ – 404 Feb 16 '17 at 0:28
3
\$\begingroup\$

Am I missing anything with respect to SOLID?

Let's tiptoe through the tulips so I don't step in anything solid, if you know what I mean.

Thoughts in light of previously given answers


So as per open close principle, I can subclass like:

Class EnrolmentRequest:IRequest

That is not inheritance, just for the record.

But if you do inherit from a RequestBase then there's no point in having the IRequest interface. This is especially true if you want to have some default or common behavior. Off hand I'm thinking about handling errors from the web service call.


Request object instantiation

public class Client
{
   private static IRequest[] requestHandlers = new IRequest[] 
   {
      new EnrolmentRequest(),
      new ReEnrolmentRequest(),
      ...
    }
}

When I saw another answer that extracts this to a factory class, the above immediately struck me as a SRP violation. However if a request is the exclusive domain of the Client class (it is private after all), then perhaps not an SRP violation. Even so I think about a client using the request functionality, not being the request functionality.


Request triggered only in the Client constructor

Both answers require that every request must be a new object creation. If that is intentional make sure it's documented. Otherwise I'd say it is a single responsibility violation in the constructor method.

public class Client
{
    public Client(string requestType)
    {
       IRequest request = requestHandlers.FirstOrDefault(x => x.Name == requestType);
        if (request == null) { /* Handle null case */ }

        request.CallService();
     }
 }    

Least Knowledge Violation

requestHandlers.FirstOrDefault(x => x.Name == requestType);

Client in a sense is implementing Equals for another class. Client should only have to ask "give me this requestType". Something like this:

 factory.CreateRequest(requestType);

or

RequestDictionary[requestType];

or

IRequest myRequest;
RequestDictionary.TryGetValue(requestType, myRequest);

[Above are not syntactically complete statements. Just getting the idea across.]

Yes, IRequest declares a public Name property. But a well written class exposes behavior and hides state.


Formally Define the Request Types

Use an enum as suggested in a couple of comments.

The enum is a complete, type safe list of all request types. In contrast "Enrol" is merely a string; subject to typos, overloaded meaning, and introducing bugs.

Given the code as currently written, you'll never know what the complete set of requests is without reading ALL the code


SRP Revisited

Given the concept of "request types" as a distinct thing, then I'd say that makes the IRequest[] inside of Class a SRP violation.

\$\endgroup\$
1
\$\begingroup\$

First of all the inheritance you introduced is appropriate and will serve you as the Open-Closed-Principle will forecast. However, it will NOT help you with the following:

Now, I still have to use if and else, and will have to change my code if there are any new request type.

This problem does not affect the OCP. The if-statements are neccessary to map external information to internal data structures. So the only thing you can do is to minimize the amount of code to change but the underlying "structure" will remain the same: a mapping.

Some possibilities to reduce if-then-else handling:

  1. change your if-statements into a Dictionary with a name-class tupel
  2. externalize mapping information into a file with a name-class tupel
\$\endgroup\$

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.