Making a web service call based on a type of request

Question

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?

• Simsons: @t3chb0t, Yes , I just typed it in Notepad++ . Will refactor them, Thanks
• t3chb0t: Now I like it. Thank you for your cooperation ;-)
• JeffC: 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.
• Greg Burghardt: (sigh) This is such a nitpick, but the correct spelling of "enrollment" has two "l"s.

Answer 1

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.

• peval27: couple of nits: you're missing 'string' in the property Name, and it's requestHandlers is of type IRequest[] not IRequest
• Danko Durbić: 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.
• Nazar554: In C# 6.0 you can also do public string Name { get; } = "ReEnrol";
• JanDotNet: @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.

Answer 2

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.

• Stephen Byrne: 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.
• 404: @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.

Answer 3

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.

Answer 4

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