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();
精彩评论