0
\$\begingroup\$

Need to perform such actions:

  • receive JSON from REST
  • extract nexted JSON document
  • calculate hash of the nested document
  • add some fields to extracted document and save it in database
  • return response CONTAINING some fields of the source DOCUMENT

I wrote this service:

@Service
public class InvoiceService {

    private static final Logger logger = LoggerFactory.getLogger(InvoiceService.class);

    @Autowired
    private InvoiceRepository repository;


    public Object processInvoice(InvoiceRequest invoice) {
        if (!validateInvoice(invoice)) {
            logger.error("Error: empty object UID. JSON dumped:");
            logger.error(invoice.toString());
            return new ResponseError(invoice.getRequestUID(), invoice.getObjectUID(),
                    invoice.getSenderDateTime(), "Empty object UID");
        }

        return saveInvoice(invoice);
    }

    private boolean validateInvoice(InvoiceRequest invoice) {
        return (invoice.getObjectUID() != null) && (!invoice.getObjectUID().equals(""));
    }

    private Object saveInvoice(InvoiceRequest invoice) {
        FlatInvoice flat = convertToFlat(invoice);

        if (repository.existsByUID(invoice.getObjectUID())) {
            if (tryToUpdate(invoice)) {
                ResponseSuccess success = new ResponseSuccess(invoice.getRequestUID(), invoice.getObjectUID(),
                        Time.now());
                return new ResponseEntity<>(success, HttpStatus.OK);
            }
            else {
                // return status OK
                return new ResponseEntity<>("", HttpStatus.OK);
            }
        }

        if (!repository.insert(flat)) {
            respondWithError(invoice, "DB insertion failed", HttpStatus.INTERNAL_SERVER_ERROR);
        }

        return respondWithSuccess(invoice);
    }

    private ResponseEntity<?> respondWithError(InvoiceRequest invoice, String msg, HttpStatus status) {
        ResponseError ret =  new ResponseError(invoice.getRequestUID(), invoice.getObjectUID(), Time.now(),
                msg);
        return new ResponseEntity<>(ret, status);
    }

    private ResponseEntity<?> respondWithSuccess(InvoiceRequest invoice) {
        ResponseSuccess ret =  new ResponseSuccess(invoice.getRequestUID(), invoice.getObjectUID(), Time.now());
        return new ResponseEntity<>(ret, HttpStatus.OK);
    }

    private FlatInvoice convertToFlat(InvoiceRequest invoice) {
        FlatInvoice flat = InvoiceFlattener.flatten(invoice);
        flat.setSenderDateTime(invoice.getSenderDateTime());
        flat.setCreateDateTime(Time.now());
        flat.setHash(Crypto.sha256(invoice.getInvoice().toString().getBytes()));
        return flat;
    }

    public Object findByUID(String uid) {
        return repository.findByObjectUID(uid);
    }

    public boolean tryToUpdate(InvoiceRequest invoice) {
        Object object = repository.findByObjectUID(invoice.getObjectUID());
        HashMap map = (HashMap) object;
        String hashStored = (String)map.get("hash");
        String hashActual = Crypto.sha256(invoice.getInvoice().toString().getBytes());

        if (compareSenderDates(invoice, map)) {
            if (!hashActual.equals(hashStored)) {
                return repository.update(invoice.getObjectUID(), convertToFlat(invoice));
            }
        }
        else {
            System.out.println("ignore doc, reason: actual senderDateTime is less or equal");
        }
        return false;
    }

    public boolean compareSenderDates(InvoiceRequest invoice, HashMap storedDoc) {
        long timestampReceived = Time.timestamp(invoice.getSenderDateTime());
        long timestampStored = Time.timestamp((String)storedDoc.get("senderDateTime"));
        return timestampReceived > timestampStored;
    }

}

It is overcomplicated because of a huge amount of nested logic and if statements. All methods do more than one logical action. Constructing of error response is mixed with business logic. How to refactor all of this?

public class ResponseError {

    private String requestUID;
    private String objectUID;
    private String receivedDateTime;
    private String errorMessage;

    public ResponseError(String requestUID, String objectUID, String receivedDateTime, String errorMessage) {
        this.requestUID = requestUID;
        this.objectUID = objectUID;
        this.receivedDateTime = receivedDateTime;
        this.errorMessage = errorMessage;
    }

    public String getRequestUID() {
        return requestUID;
    }

    public void setRequestUID(String requestUID) {
        this.requestUID = requestUID;
    }

    public String getObjectUID() {
        return objectUID;
    }

    public void setObjectUID(String objectUID) {
        this.objectUID = objectUID;
    }

    public String getReceivedDateTime() {
        return receivedDateTime;
    }

    public void setReceivedDateTime(String receivedDateTime) {
        this.receivedDateTime = receivedDateTime;
    }

    public String getErrorMessage() {
        return errorMessage;
    }

    public void setErrorMessage(String errorMessage) {
        this.errorMessage = errorMessage;
    }
}



public class ResponseSuccess {
    private String requestUID;
    private String objectUID;
    private String receivedDateTime;

    public ResponseSuccess(String requestUID, String objectUID, String receivedDateTime) {
        this.requestUID = requestUID;
        this.objectUID = objectUID;
        this.receivedDateTime = receivedDateTime;
    }

    public String getRequestUID() {
        return requestUID;
    }

    public void setRequestUID(String requestUID) {
        this.requestUID = requestUID;
    }

    public String getObjectUID() {
        return objectUID;
    }

    public void setObjectUID(String objectUID) {
        this.objectUID = objectUID;
    }

    public String getReceivedDateTime() {
        return receivedDateTime;
    }

    public void setReceivedDateTime(String receivedDateTime) {
        this.receivedDateTime = receivedDateTime;
    }
}


public class ResponseBuilder {

    public static ResponseError error(HashMap request, String msg) {
        return new ResponseError(FieldExtractor.requestUID(request),
                FieldExtractor.objectUID(request),
                Time.now(),
                msg);
    }

    public static ResponseSuccess success(HashMap request) {
        return new ResponseSuccess(FieldExtractor.requestUID(request),
                FieldExtractor.objectUID(request),
                Time.now());
    }

    private static class FieldExtractor {
        public static String requestUID(HashMap request) {
            return (String) getBusData(request).get("requestUID");
        }

        public static String objectUID(HashMap request) {
            return (String) getBusData(request).get("objectUID");
        }

        private static HashMap getBusData(HashMap request) {
            HashMap receiveData = (HashMap)request.get("receiveData");
            return (HashMap)receiveData.get("iBusData");
        }
    }
}


public class InvoiceRequest {

    private ReceiveData receiveData;

    public ReceiveData getReceiveData() {
        return receiveData;
    }

    public void setReceiveData(ReceiveData receiveData) {
        this.receiveData = receiveData;
    }

    public String getRequestUID() {
        return getReceiveData().getiBusData().getRequestUID();
    }

    public String getObjectUID() {
        return getReceiveData().getiBusData().getObjectUID();
    }

    public String getSenderDateTime() {
        return getReceiveData()
                .getiBusData()
                .getSenderDateTime();
    }

    public Invoice getInvoice() {
        return getReceiveData()
                .getiBusData()
                .getData()
                .getInvoice();
    }

    public String getUID() {
        return getInvoice().getUid();
    }

    public String getDate() {
        return getInvoice().getDate();
    }

    public String getNumber() {
        return getInvoice().getNumber();
    }

    public Boolean getMarked() {
        return getInvoice().getMarked();
    }

    public Boolean getPosted() {
        return getInvoice().getPosted();
    }

    public String getSenderCityUID() {
        return getInvoice().getSenderCityUID();
    }

    public String getReceiverTerminalUID() {
        return getInvoice().getReceiverTerminalUID();
    }

    public String getReceiverCityUID() {
        return getInvoice().getReceiverCityUID();
    }

    public String getCargoUID() {
        return getInvoice().getCargoUID();
    }

    public Double getAmount() {
        return getInvoice().getAmount();
    }

    public Double getAmountExtraLarge() {
        return getInvoice().getAmountExtraLarge();
    }

    public Double getNetWeight() {
        return getInvoice().getNetWeight();
    }

    public Double getNetWeightExtraLarge() {
        return getInvoice().getNetWeightExtraLarge();
    }

    public Double getGrossWeight() {
        return getInvoice().getGrossWeight();
    }

    public Double getNetVolume() {
        return getInvoice().getNetVolume();
    }

    public Double getNetVolumeExtraLarge() {
        return getInvoice().getNetVolumeExtraLarge();
    }

    public Double getGrossVolume() {
        return getInvoice().getGrossVolume();
    }

    public Double getDeclaredValue() {
        return getInvoice().getDeclaredValue();
    }

    public String getContractorSenderUID() {
        return getInvoice().getContractorSenderUID();
    }

    public String getContractorReceiverUID() {
        return getInvoice().getContractorReceiverUID();
    }

    public String getContractorPayerUID() {
        return getInvoice().getContractorPayerUID();
    }

    public String getContractorSenderIssueUID() {
        return getInvoice().getContractorSenderIssueUID();
    }

    public String getContractorReceiverIssueUID() {
        return getInvoice().getContractorReceiverIssueUID();
    }

    public String getActualReceiver() {
        return getInvoice().getActualReceiver();
    }

    public Double getFreightInKops() {
        return getInvoice().getFreightInKops();
    }

    public Double getDeliveryTimeInsuranceSum() {
        return getInvoice().getDeliveryTimeInsuranceSum();
    }

    public String getTerminalUID() {
        return getInvoice().getTerminalUID();
    }

    public Double getCargoInsuranceSum() {
        return getInvoice().getCargoInsuranceSum();
    }

    public String getSenderCityName() {
        return getInvoice().getSenderCityName();
    }

    public String getSenderCityKLADR() {
        return getInvoice().getSenderCityKLADR();
    }

    public String getReceiverCityName() {
        return getInvoice().getReceiverCityName();
    }

    public String getReceiverCityKLADR() {
        return getInvoice().getReceiverCityKLADR();
    }

    public Double getTransportationCostOnReceiving() {
        return getInvoice().getTransportationCostOnReceiving();
    }

    public Double getTransportationCostOnIssuing() {
        return getInvoice().getTransportationCostOnIssuing();
    }

    public String getBaseDocumentUID() {
        return getInvoice().getInvoiceUID();
    }

    public String getPackagingStateFlags() {
        return getInvoice().getPackagingStateFlags();
    }

    public String getCargoName() {
        return getInvoice().getCargoName();
    }

    public String getOperation() {
        return getInvoice().getOperation();
    }

    public Person getActualReceiverPersonalID() {
        return getInvoice().getActualReceiverPersonalID();
    }

    @Override
    public String toString() {
        return "InvoiceRequest{" +
                "receiveData=" + receiveData +
                '}';
    }
}

@Service
public class InvoiceFlattener {
    public static FlatInvoice flatten(InvoiceRequest invoice) {
        FlatInvoice result = new FlatInvoice(
                invoice.getUID(),
                invoice.getDate(),
                invoice.getNumber(),
                invoice.getMarked(),
                invoice.getPosted(),
                invoice.getSenderCityUID(),
                invoice.getReceiverTerminalUID(),
                invoice.getReceiverCityUID(),
                invoice.getCargoUID(),
                invoice.getAmount(),
                invoice.getAmountExtraLarge(),
                invoice.getNetWeight(),
                invoice.getNetWeightExtraLarge(),
                invoice.getGrossWeight(),
                invoice.getNetVolume(),
                invoice.getNetVolumeExtraLarge(),
                invoice.getGrossVolume(),
                invoice.getDeclaredValue(),
                invoice.getContractorSenderUID(),
                invoice.getContractorReceiverUID(),
                invoice.getContractorPayerUID(),
                invoice.getContractorSenderIssueUID(),
                invoice.getContractorReceiverIssueUID(),
                invoice.getActualReceiver(),
                invoice.getFreightInKops(),
                invoice.getDeliveryTimeInsuranceSum(),
                invoice.getTerminalUID(),
                invoice.getCargoInsuranceSum(),
                invoice.getSenderCityName(),
                invoice.getSenderCityKLADR(),
                invoice.getReceiverCityName(),
                invoice.getReceiverCityKLADR(),
                invoice.getTransportationCostOnReceiving(),
                invoice.getTransportationCostOnIssuing(),
                invoice.getBaseDocumentUID(),
                invoice.getPackagingStateFlags(),
                invoice.getCargoName(),
                invoice.getOperation(),
                invoice.getActualReceiverPersonalID());

        return result;
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ Could you also provide your InvoiceRequest, ResponseError, FlatInvoice and InvoiceFlattener ? \$\endgroup\$
    – gervais.b
    Nov 23, 2018 at 10:25
  • \$\begingroup\$ @gervais.b, InvoiceRequest is a hierarchy of objects representing JSON structure. \$\endgroup\$
    – typemoon
    Nov 23, 2018 at 10:33

1 Answer 1

1
\$\begingroup\$

Thanks for the classes. However I did not wait for them and just created some "stubs". Here is my answer:

When you have "a huge amount of nested logic" you should try to extract and distribute it somewhere else. I like to do DDD and place the business logic in my domain objects.

Validation

Let's start from the first lines of code: if ( !validateInvoice(invoice) ):

You can easily extract the validation to a dedicated class. Spring provide a good support for validation. You can also annotate your methods parameters with @Valid to have them validated automatically by the framework.

To separate the construction of the error from your business logic you can return a structure of errors instead of a boolean. Then you verify if thie structure has errors and construct the error response or let it continue. This structure already exists into the spring framework.

You can also hide the construction of the ErrorResponse in the class itself or into a factory method.

Map<String, Set<String>> errors = validator.validate(invoice);
if ( errors.isEmpty() ) {
  return saveInvoice(invoice);
} else {
  logger.error("Invalid request : {}. Json: {}.", errors, invoice);
  return new ResponseEntity<>(ResponseError.of(invoice, errors), HttpStatus.BAD_REQUEST);
}

Note that I have replaced the two calls to logger.error with one. Nothing guarantee that the two messages will be next to each other so it is better to log everything in one statement. If you are using Slf4j you can also use the templates expansion to format your message. And, please, do not use toString to have a JSON representation of your object.

Transformation

Many use a mapping framework. But you can also create your own mapper like you did with the InvoiceFlattener. However, it is sad that you still have to set some properties to the result object. Ideally the result will be "complete".

FlatInvoice flat = InvoiceFlattener.flatten(invoice);
// Nothing more

Persistence

It seems that saveInvoice methods contains your business logic but also a couple of ifs.

You can already remove the if around repository.insert(flat) because usually a repository trows an exception when he cannot persist the entity. You can use a Spring exception handler to convert the exception to a ResponseError. Apply this to all the methods in your repository.

The tryToUpdate method is confusing because it start with try and thus we expect an exception. It is also annoying to have the effective update as a side effect of this test. You should better keep the test in a method but update in the body of yourif.

if ( repository.exists(invoice) ) {
  if ( isChanged(invoice) ) {
    repository.update(invoice.getObjectUID(), InvoiceFlattener.flatten(invoice));
    ResponseSuccess body = new ResponseSuccess(invoice.getRequestUID(), invoice.getObjectUID(),
        LocalTime.now());
    return new ResponseEntity<>(body, HttpStatus.OK);
  } else {
    logger.debug("Ignoring unchanged invoice {}.", invoice);
    return new ResponseEntity<>("", HttpStatus.OK);
  }
} else {
  repository.insert(flat);
  ResponseSuccess ret =  new ResponseSuccess(invoice.getRequestUID(), invoice.getObjectUID(), LocalTime.now());
  return new ResponseEntity<>(ret, HttpStatus.OK);
}

Domain logic

Another improvement that you can do is to replace the Map that you receive from your repository by a class. So that you should be able to move the isChanged logic into this class. You can also map the InvoiceRequest to that new class so that you don't have to deal with InvoiceRequest, FlatInvoice and Map<String, ?> that are all representing the same model (from what I understand). If you do that you have something that looks like a domain object.

You can also continue the separation between the domain logic and the protocol by introducing a controller aside of your service. The controller will map the InvoiceRequest to your Invoice entity and convert the result and exceptions to HttpEntity.

You should end up with something like:

@Service
public class InvoiceService {
    private static final Logger LOG = LoggerFactory.getLogger(InvoiceService.class);

    private final InvoiceRepository repository;

    public InvoiceService(InvoiceRepository repository) {
        this.repository = repository;
    }

    public Invoice process(Invoice invoice) throws PersistenceException {
        return repository.find(invoice.getUuid())
                .map(existing -> updateOrIgnore(existing, invoice))
                .orElseGet(() -> repository.insert(invoice));
    }

    private Invoice updateOrIgnore(Invoice existing, Invoice updated) {
        if (existing.isAfter(updated) && existing.isDifferent(updated)) {
            return repository.update(existing.getUuid(), updated);
        } else {
            LOG.debug("Ignoring unchanged invoice {}.", updated);
            return existing;
        }
    }

}


// ~ ----------------------------------------------------------------------

@RestController
public class InvoiceController {

    private final InvoiceRequestValidator validator = new InvoiceRequestValidator();
    private final InvoiceRequestMapper mapper = new InvoiceRequestMapper();
    private final InvoiceService service;

    public InvoiceController(InvoiceService service) {
        this.service = service;
    }

    @PostMapping("/invoices")
    public ResponseEntity<?> receive(@Valid InvoiceRequest request) throws
            PersistenceException, ValidationException {
        Map<String, Set<String>> errors = validator.validate(request);
        if ( errors.isEmpty() ) {
            Invoice invoice = mapper.map(request);
            Invoice result = service.process(invoice);
            return new ResponseEntity<>(
                    ResponseSuccess.of(request, result),
                    HttpStatus.OK);
        } else {
            return new ResponseEntity<>(
                    ResponseError.of(request, errors),
                    HttpStatus.BAD_REQUEST);
        }
    }

    @ExceptionHandler(PersistenceException.class)
    ResponseEntity<ResponseError> on(PersistenceException pe) {
        ResponseError body = new ResponseError(pe.getRequestUID(), pe.getObjectUID(),
                pe.getSenderDateTime(), pe.getMessage());
        return new ResponseEntity<>(body, HttpStatus.INTERNAL_SERVER_ERROR);
    }

}

I hope that you can find some inspiration in this answer.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.