best way refactoring the ternary operator Java

93 Views Asked by At

isCredit is the common boolean value. Please pay attantion that we put the same values, (but crossing) on debet and credit side depends on boolean

 .setDebetSide(new DebetSideDto()
                        .setPayerINN(isCredit ? record.getInnContractor() : record.getClientInn())
                        .setPayerName(isCredit ? record.getNameContractor() : record.getClientName())  
.setCreditSide(new CreditSideDto()
                        .setReceiverINN(isCredit ? record.getClientInn() : record.getInnContractor())
                        .setReceiverName(isCredit ? record.getClientName() : record.getNameContractor())
1

There are 1 best solutions below

1
Maarten Bodewes On

Try and avoid double calls, so set additional variables in an if construct, then call both functions using these variables:

var debitSideDto = new DebetSideDto();
var creditSideDto = new CreditSideDto();
if (isCredit) {
    // contractor is payer, client is receiver
    debitSideDto.setPayerINN(record.getInnContractor());
    debitSideDto.setPayerName(record.getNameContractor());
    ...
} else {
    // client is payer, contractor is receiver
    ...
}

.setDebetSide(debitSideDto);
.setCreditSide(creditSideDto);

This has the following advantages:

  • code branches only once instead of 4 times (lower complexity)
  • only two calls per line maximum (a setter with the getter)

As always it sacrifices some conciseness for readability; usually ternary operations are removed to enhance readability.


One critique here is that debitSideDto was probably designed to contain an invalid state; it is required to call setters to get to a valid state. It's probably better to assign required values upon construction. In that case the constructor needs to be called within the if statement. The variable doesn't have to be assigned any value before the if; the compiler is fine as long as it is assigned a value - it may even be declared final:

final var debitSideDto;
final var creditSideDto;;
if (isCredit) {
    // contractor is payer, client is receiver
    debitSideDto = new DebetSideDto(
             record.getInnContractor(),
             record.getNameContractor());
    ...
} else {
    // client is payer, contractor is receiver
    ...
}

.setDebetSide(debitSideDto);
.setCreditSide(creditSideDto);

Alternatively you could create variables for payerName, payerInn, receiverName and receiverInn rather than variables for the Dto's of course. That would not remove the invalid states of the Dto's but it would localize that particular issue. On the other hand I'd prefer fewer variables, so I chose to use the Dto's to do the heavy lifting.

I've added some comments though as without these variables the roles are not very clear.


This is a localized change of course. In practice I would be wondering why such a reversal of roles is required at this late a stage. I'd be asking questions about the general design (it might be OK, but to me it is a bit of a red flag).