tags:

views:

128

answers:

5

I have this question about best practices in following examples:

interface Request;
interface Service {
  void process(Request request)
}

class MyService implements Service;
class YourService implements Service;

class MyRequest implements Request;
class YourRequest implements Request;

But how to ensure that MyService will always receive MyRequest and YourService will get YourRequest only, and not in the opposite way? Obvious answer "if-instance-of-check" in MyService.process(...) seems ugly and somehow against SOLID principles. Maybe there are better ways around?

Maybe generics would be good solution? (But then, how to use them in code that has to run under Java 1.4?)

+6  A: 

Put simply, you are establishing an interface that you then don't want to adhere to, so it's not really an ideal design.

What I mean is, if MyService implements Service, then it must be able to take any kind of request. Otherwise it isn't following the defined contract.

I would question why you have the Service interface at all in this instance, and if you do need it (for other methods) whether it's appropriate for the process(Request request) method to be on there if subclasses are not going to honour it.

The logic behind my design was that I have lots of code common for both services. I want to reuse this code in each service - most notable is: class LoadBalancer implements Service { void process(Request r) { r.doSomeCommonStuff(); getNextService().process(r); } }Thanks for your comment anyway, I'll rethink the design.
Jakub
A: 

Anything implementing Service should expect to implements its methods as they are. If MyService and YourService require different method prototypes, then they are different interfaces.

Think of it from the other direction. Without knowing the implementation behind a Service interface, any caller should able to call Service.process(request) with any implementation of Request, and expect to receive a valid response.

Cogsy
+2  A: 

In my opinion generics would suit better here. Your interfaces pretend that a service can handle any type of Request. But in fact the implementations of each seem to be tightly coupled.

rudolfson
A: 

try introducing another level of indirection:

interface Module {
   Service createService();
   Request createRequest();
}

class MyModule implements Module {
   Service createService() { return new MyService(); } 
   Request createRequest() { return new MyRequest(); }
}

class YourModule implements Module {
   Service createService() { return new YourService(); } 
   Request createRequest() { return new YourRequest(); }
}
dfa
this doesnt help because it is possible that client code passes in a YourRequest instance into a MyService instance. The original design is broken at the core, and cant be fixed no matter how many factories are created.
Chii
+3  A: 

If the design of the contract is that each Service can process any kind of Request, then your implementation of MyService , which only takes MyRequest (and breaks if other kinds of Requests are passed in), is wrong.

If the design of the contract is that Service and Request subclasses maps to each other, e.g., MyService can (and should) only process a MyRequest, then you will need to change the interface of Service. Otherwise, the current interface as written in the question does not do what the question describes it to do. One way to fix is to parameterize the Service interface:

interface Service<R> {
   void process(R request);
}

then your concrete MyService will be

public class MyService implements Service<MyRequest> {
   public void process (MyRequest r) {/*blah*/}
}

You can see an example of this in action in the JDK - the Comparator interface does exactly this, for exactly the same reason. http://java.sun.com/javase/6/docs/api/java/util/Comparator.html

I cant see why you would, but if you still want to restrict the hierachy of MyRequest to be a request, then you can swap Service<R> with Service<R extends Request>

edit: this obviously doesnt run in 1.4, so to do the same thing[1] , you will need to use a visitor pattern. Its uglier, but 1.4 is ugly =)

interface Service {
   void process(Request visitor);
}
interface RequestVisitor {
   void visitMyRequest(MyService service);
   void visitYourRequest(YourService service);
   void visitTheOtherRequest(TheOtherService  service);
}
interface Request extends RequestVisitor { /* and any extra methods required for request*/ }
public class MyService implements Service {
   public process(Request r) {r.visitMyRequest(this);}
   public void doSpecialMyProcessing(MyRequest request) { /* your code using MyRequest*/ }
}
public class YourService implements Service {
   public process(Request r) {r.visitYourRequest(this);}
   public void doSpecialYourProcessing(YourRequest request) { /* your code using YourRequest */ }
}
public class MyRequest implements Request {
   void visitMyRequest(MyService service) {
      service.doSpecialMyProcessing(this);
   }
   void visitYourRequest(YourService service) {
      throw new UnsupportedOperation("Cannot call visitYourRequest in MyRequest!");
   }
   void visitTheOtherRequest(TheOtherService  service) {
      throw new UnsupportedOperation("Cannot call visitTheOtherRequest in MyRequest!");
   }
}
public class YourRequest implements Request {
   void visitMyRequest(MyService service) {
      throw new UnsupportedOperation("Cannot call visitMyRequest in YourRequest !");
   }
   void visitYourRequest(YourService service) {
      service. doSpecialYourProcessing(this);
   }
   void visitTheOtherRequest(TheOtherService  service) {
      throw new UnsupportedOperation("Cannot call visitTheOtherRequest in YourRequest !");
   }
}

[1] actually its not the same, because now you will need to write a method for each request subtype. In 1.4, you would have to cast and do instanceof etc, to achieve what 1.5 can do with generics.

Chii
Actually your second code (with `RequestVisitor` for 1.4) makes `MyService` depending on `YourService` and vice versa. Because `MyService` uses `MyRequest` that implements `Request` that extends `RequestVisitor` that directly depends on all the services. Not cool.
Jakub
I have to say, the way i've designed it isnt the way you'd want it given your comment about the LoadBalancer in one of the other answers.
Chii