开发者

Does this design make sense?

Does the below design look good to you? Is it a design pattern? How would you improve it if you think it needs refactoring?


public class FruitFetcher {
    public static void main(String[] args) {
        FruitFetcher fetcher = new FruitFetcher();
        Apple apple = (Apple) fetcher.fetch(new FetchAppleRequest());
    }


    public Fruit fetch(FetchFruitRequest request){
        Fruit fruit = null;

        if(request.getFruitName().equals(FetchAppleRequest.REQUEST_NAME)){
            fruit = new Apple();
        }else if (request.getFruitName().equals(FetchBananaRequest.REQUEST_NAME)){
            fruit =  new Banana();
        }
        return fruit;
    }

}

abstract class FetchFruitRequest{

    abstract public String getFruitName(); 

}

class FetchAppleRequest extends FetchFruitRequest{
    static String REQUEST_NAME = "Fetch_Apple";

    @Override
    public String getFruitName() {
        return REQUEST_NAME;
    }
}

class FetchBananaRequest extends FetchFruitRequest{
    static String REQUEST_NAME = "Fetch_Banana";

    @Override
    public String getFruitName() {
        return REQUEST_NAME;
    }
}

class Fruit {
}

class Apple extends Fruit{

}

class Banana extends Fruit{

}

In the code, clients of FruitFetcher need to upcast Fruit to a right type, do you think that is correct?


Edit: To answer The Elite Gentleman's question, I modified my code to show that the Reqeust needs a type other than a simple String.

Does get getResponse() in PaymentServer still look kind of 'ugly'? How do I re-fector it?



public class PaymentServer {


    public static void main(String[] args) {
        PaymentServer server = new PaymentServer();
        //set pin
        SetPinResponse setPinResponse = (SetPinResponse) server.getResponse(new SetPinRequest("aPin"));
        System.out.println(setPinResponse.isSuccess());

        //make payment
        MakePaymentResposne makePaymentResponse = (MakePaymentResposne) server.getResponse(new MakePaymentRequest(new Money("5.00)"),"aPin"));
        System.out.println(makePaymentResponse.isSuccess());
    }




   开发者_如何转开发 public Response getResponse(Request request){
        Response aResponse = null;

        if(request.getRequestName().equals(SetPinRequest.REQUEST_NAME)){
            aResponse = new SetPinResponse();
        }else if (request.getRequestName().equals(MakePaymentRequest.REQUEST_NAME)){
            aResponse =  new MakePaymentResposne();
        }
        return aResponse;
    }

}

abstract class Request{
    abstract public String getRequestName(); 
}

class SetPinRequest extends Request{
    static String REQUEST_NAME = "Set_Pin";
    private String pin;

    SetPinRequest(String pin){
    this.pin = pin;
    }

    @Override
    public String getRequestName() {
        return REQUEST_NAME;
    }

    boolean setPin(){
    //code to set pin
    return true;
    }
}

class MakePaymentRequest extends Request{
    static String REQUEST_NAME = "Make_Payment";
    private Money amount;
    private String pin;

    MakePayment(Money amount, String pin){
    this.amount = amount;
    this.pin = pin;
    }

    @Override
    public String getRequestName() {
        return REQUEST_NAME;
    }
}


abstract class Response {
    abstract protected boolean isSuccess();

}

class SetPinResponse extends Response{

    @Override
    protected boolean isSuccess() {
    return true;
    }

}

class MakePaymentResposne extends Response{
    @Override
    protected boolean isSuccess() {
    return false;
    }

}

Thanks,

Sarah


Your design is pretty close to a factory pattern. The "fetcher" is a fruit factory, you ask for a fruit of a special type and get that fruit.

The "request" part looks a bit complicated. Consider using enums:


public enum FruitType{
  APPLE, BANANA
}

public FruitFetcher { 
   public static Fruit fetch(FruitType type) {
     switch(type) {
       case APPLE:  return new Apple();
       case BANANA: return new Banana();
     }
   }
   return null;
}

Edit - It's still pretty complicated. It takes a while to understand, what you want to achieve and this is usually an indicator that there must be an easier design.

First - if your Request and Response classes offer nothing but abstract method declarations, then you should refactor your code an code both types as interfaces. One step towards improved readability.

Second - the getResponse methd can be greatly simplified. You don't need that (ugly) getName construct - just check the type of the request object:

public Response getResponse(Request request){
    Response aResponse = null;

    if(request instanceof SetPinResponse) {
        aResponse = new SetPinResponse((SetPinRequest) request);
    } else if (request instanceof MakePaymentResposne) {
        aResponse =  new MakePaymentResposne((MakePaymentRequest) request);
    }

    return aResponse;
}


Besides the design pattern, what you're looking for is Generics.

As for design pattern, What you're doing is called the Factory Method Pattern, where FruitFetcher is the Factory and Fruit (and its subclasses) are Products.


Your FruitFetcher.fetch is a bit "ambiguous" (for the lack of better word). I would suggest passing a type that would identify the Fruit that you are requesting.

the type can be an enum, integer, or string (it really depends on how you want to create a specific value type that can be recognised by FruitFetcher.fetch().

Example:

public class FruitFetcher {

    public Fruit fetch(String fruitName)  {
        if ("Apple".equals(fruitName)) {
            return new Apple();
        }

        if ("Banana".equals(fruitName)) {
            return new Banana();
        }

        return null;
    }
}

This is more eloquent than sending new FetchAppleRequest() or new FetchBananaRequest().


Hint: this is the "ugly" part of the code...

 if(request.getFruitName().equals(FetchAppleRequest.REQUEST_NAME)){
        fruit = new Apple();
    }else if (request.getFruitName().equals(FetchBananaRequest.REQUEST_NAME)){
        fruit =  new Banana();
0

上一篇:

下一篇:

精彩评论

暂无评论...
验证码 换一张
取 消

最新问答

问答排行榜