5
\$\begingroup\$

I created a custom responseModel to have a standard json responseModel for some of my different RestController.

Superclass TransactionResponseModel:

public class TransactionResponseModel {

    private HttpStatus httpStatus;
    private Double amount;
    private String transactionId;
    private String message;

    public SvsTransactionResponseModel() {
        super();
    }

    public SvsTransactionResponseModel(HttpStatus httpStatus, String message) {
        super();
        this.httpStatus = httpStatus;
        this.message = message;
    }
   //public getter/setter
}

Class BalanceInquiryResponseModel:

public class BalanceInquiryResponseModel extends SvsTransactionResponseModel {

    private final BalanceInquiryResponse balanceInquiryResponse;

    public BalanceInquiryResponseModel(BalanceInquiryResponse balanceInquiryResponse) {
        super();
        this.balanceInquiryResponse = balanceInquiryResponse;
        if (this.balanceInquiryResponse != null && this.balanceInquiryResponse.getBalanceInquiryReturn() != null) {
            setAmount(this.balanceInquiryResponse);
            setMessage(this.balanceInquiryResponse);
            setTransactionId(this.balanceInquiryResponse);
            setHttpStatus();
        }
    }

    private void setAmount(BalanceInquiryResponse balanceInquiryResponse) {
        if (balanceInquiryResponse.getBalanceInquiryReturn().getBalanceAmount() != null) {
            setAmount(balanceInquiryResponse.getBalanceInquiryReturn().getBalanceAmount().getAmount());
        }
    }

    private void setTransactionId(BalanceInquiryResponse balanceInquiryResponse) {
        setTransactionId(balanceInquiryResponse.getBalanceInquiryReturn().getTransactionID());
    }

    private void setMessage(BalanceInquiryResponse balanceInquiryResponse) {
        if (balanceInquiryResponse.getBalanceInquiryReturn().getReturnCode() != null) {
            setMessage(balanceInquiryResponse.getBalanceInquiryReturn().getReturnCode().getReturnDescription());
        }
    }

    private void setHttpStatus() {
        setHttpStatus(HttpStatus.OK);
    }
}

The BalanceInquiryResponseModel expects a BalanceInquiryResponse Object for setting different attributes. Nearly all attributes for my BalanceInquiryResponseModel are coming from a thiry-party Object BalanceInquiryResponse. This third-party object has to many information and because of that I dont want to response this object to the caller but set instead all the necessary attributes manually.

In my Controller I just return a ResponseEntity<? extends SvsTransactionResponseModel>.

@PostMapping(value = "/balanceInquiry")
public ResponseEntity<TransactionResponseModel> getBalanceInquiry(
        @Valid @RequestBody BalanceInquiryModel balanceInquiry, Errors errors) {
    if (errors.hasErrors()) {
        return new ResponseEntity<>(
                new TransactionResponseModel(HttpStatus.BAD_REQUEST, errors.getAllErrors().toString()),
                HttpStatus.BAD_REQUEST);
    }
    BalanceInquiryResponse balanceInquiryResponse = null;
    //logic to get the balanceInquiryResponse.

    return new ResponseEntity<>(new BalanceInquiryResponseModel(balanceInquiryResponse), HttpStatus.OK);
}

For other controllers I will have the other responseModels extending TransactionResponseModel.

Is this a good approach for a custom responseModel?

Tech-Stack:

spring-boot

\$\endgroup\$
  • \$\begingroup\$ Offtopic. But I would replace every private with protected. If you or someone else later wants to extend TransactionResponseModel or BalanceInquiryResponseModel then private can really start to hurt. The amount of times I wished for something to be protected instead of private is beyond counting. \$\endgroup\$ – clankill3r Oct 21 '17 at 18:53
  • \$\begingroup\$ @clankill3r why do you think its offtopic? \$\endgroup\$ – Patrick Oct 23 '17 at 8:08
  • \$\begingroup\$ Cause you asked if your code is a good approach for a custom responseModel. \$\endgroup\$ – clankill3r Nov 8 '17 at 14:27
1
+50
\$\begingroup\$

To start, I think what you have there looks pretty straight forward. It also looks very neatly written. From just what I can see here, it would appear the largest gain from your "base" model is that it removes extraneous data from traveling out to the user. The largest loss (potentially), is that it utilizes inheritance and theoretically forces those 4 properties onto every inherited result. That being said, here's some reasoning...

Disclaimer :) Inheritance may be the perfect solution for what you've got going here in the long run. But from the limited view here, it feels heavy handed. It should also be noted that I haven't written Java an about 9 years and I'm currently a C/F# programmer.

Two things stand out to me. First, the use of properties and private fields. This contract appears to be immutable. Could the fields just become public and final and lose the setters? It doesn't appear to do much except to protect inherited members from accessing those values directly. It would also reduce the amount of code in the class (#1 reason). I can't see from here what the benefit of them being private is. The notion that an inherited class would have no real access to it's base class raises a red flag.

This leads to the second thing. Consider removing the inheritance. Inheritance is always great until it's not. You contract is simple enough that it wouldn't suffer this problem today, but several inherited types later it might. The challenge with inheritance (especially with data), is that it forces everything to be the same shape. The upside, is that inheritance is never really required to solve a problem. It's just something we do... just because, usually.

You could consider a setup like this (pseudo code):

public class TransactionResponseModel { ... }

public class BalanceInquiryResponseModel {
    public BalanceInquiryResponseModel(TransactionResponseModel transactionResponseModel, BalanceInquiryResponse balanceInquiryResponse) {
    ...
    }

    public final TransactionalResponseModel TransactionalResponse;
    public final BalanceInquiryReponse InquiryResponse;
}

Inheriting doesn't really force anything, because the next developer might simply choose not to inherit. Here you get all the same shapes and data without the class coupling and hierarchy. Which is, in my experience, totally worth it. When the response model comes that doesn't fit the same shape as your base classes, you won't have to setup a new inherited structure, modify your current one, or even care for that matter. New data, new shape, no problem.

There are lot's of articles available on the un-benefits of inheritance:

Gooooooooogle

Hope this information is helpful.

\$\endgroup\$
  • \$\begingroup\$ Thanks for your answer. Not quite sure if its suitable for me. Do I need the `TransactionResponseModel´ first to create a ´BalanceInquiryResponseModel´? Maybe I need more details from your pseudo code. \$\endgroup\$ – Patrick Oct 23 '17 at 7:24
  • \$\begingroup\$ @Patrick not necessarily. you could new up a TransactionalResponseModel in the constructor. if you want to set it up that way. For myself, I would require it. But there's more than one way to get things done. The main point being, the more your inherited class is dependent on your base class, the harder it is to undo it when you need to. and... inheritance never really required. \$\endgroup\$ – TBone Oct 23 '17 at 13:04

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.