2
\$\begingroup\$

Q: I have a Bank class containing multiple loan accounts (LoanAccount class). I've create a LoanAccountService that have the CRUD functionalities. My concerns are about how I implemented the update functionality.

Bank

public class Bank {
    private List<LoanAccount> loanAccounts;
}

Loan account

public class LoanAccount {
    private String id;
    private Integer numberOfInstallments;
    private LoanAccountType type;
    private Date creationDate;
    private BigDecimal loanAmount;
}

Service

public class LoanAccountService{

    private Bank bank;

    public LoanAccountService(Bank bank) {
        this.bank = bank;
    }

    public LoanAccount update(LoanAccount loanAccount) {
        Optional<LoanAccount> account = bank.getLoanAccounts()
                .stream()
                .filter(la -> la.getId().equals(loanAccount.getId()))
                .findAny();
        if (account.isPresent()) {
            account.get().setCreationDate(loanAccount.getCreationDate());
            account.get().setLoanAmount(loanAccount.getLoanAmount());

        account.get().setNumberOfInstallments(loanAccount.getNumberOfInstallments());
            account.get().setType(loanAccount.getType());
        } else {
            throw new IllegalArgumentException("The object does not exist.");
        }
        return loanAccount;
    }
}

When the method update is called with a LoanAccount containing an id that already exists in loanAccounts list, I want to update the existing object with the object loanAccount given as parameter.

Above is my implementation, but I feel like there should be better ways to do it.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Hello, from your implementation it seems me that you can create a loanaccount instance with the same id of an existing one and after use the newer to update the older (you are also updating the creation date field). I expect instead to have all loanaccounts have at least different ids, is this the expected behaviour ? \$\endgroup\$ Commented Sep 30, 2019 at 11:19
  • 1
    \$\begingroup\$ Why would you ever want to copy a LoanAccount? Please provide some context. \$\endgroup\$ Commented Sep 30, 2019 at 14:09

3 Answers 3

1
\$\begingroup\$

Feature Envy

The LoanAccountService needs to know all fields from LoanAccount that gets updated:

account.get().setCreationDate(loanAccount.getCreationDate());
account.get().setLoanAmount(loanAccount.getLoanAmount());
account.get().setNumberOfInstallments(loanAccount.getNumberOfInstallments());
account.get().setType(loanAccount.getType());

Now imagine LoanAccount would get a few more fields and they are updateable too but you forgot to change LoanAccountService.. This means LoanAccountService depends on LoanAccount..

A solution would be to add a new method to LoanAccount:

public LoanAccount updateBy(LoanAccount other) {
  this.creationDate = other.creationDate();
  this.loanAmount = other.loanAmount();
  this.numberOfInstallments = other.numberOfInstallments();
  this.type = other.type();
  return this;
}

Optional#map

The if-statement

if (account.isPresent()) {

can be replaced by the method map on Optional. With adding the new updateBy method:

public LoanAccount updateBy(LoanAccount other) {
  bank.getLoanAccounts()
      .stream()
      .filter(la -> la.getId().equals(other.getId()))
      .findAny()
      .map(loanAccount -> loanAccount.updateBy(other))
      .orElseThrow(() -> new IllegalArgumentException("The object does not exist."));
  return other;
}

Further Improvement

A second Feature Envy is in the following snipped:

bank.getLoanAccounts()
   .stream()
   .filter(la -> la.getId().equals(loanAccount.getId()))

Not the LoanAccountService should filter the data but the Bank itself should filter it:

// in Bank.java

public Optinal<List<LoanAccount> findBy(int id) {
   return loanAccounts.stream()
                      .filter(la -> la.getId().equals(id))
                      .collect(Collectors.toList())
}

All together

// LoanAccountService.java

public LoanAccount updateBy(LoanAccount other) {
  bank.findBy(other.getId())
      .findAny()
      .map(loanAccount -> loanAccount.updateBy(other))
      .orElseThrow(() -> new IllegalArgumentException("The object does not exist."));
  return other;
}
\$\endgroup\$
1
  • \$\begingroup\$ In Bank::findBy you have the wrong return type in the signature. Also LoadAccountService::updateBy is assuming Bank::findBy is returning a Stream. I'd also like to suggest to have Bank::findBy directly return a single (optional) LoanAccount by moving .findAny() there instead of building an unneeded List. \$\endgroup\$
    – RoToRa
    Commented Oct 1, 2019 at 12:31
1
\$\begingroup\$

I would suggest following:

  1. Place operations to the objects that contain data for this operation. It will improve encapsulation and allow more flexible code reuse. Also it will be easy to test such implementation because of small methods.
  2. Use more convenient structure. In this case Map<String, LoanAccount> (map id to loanAccount) instead of List<LoanAccount>

As you can see in this case service contains only required logic (how to react on loan absence). It is easy to read, test and understand.

LoanAccountService.java

public class LoanAccountService{

    private Bank bank;

    public LoanAccountService(Bank bank) {
        this.bank = bank;
    }

    public LoanAccount update(LoanAccount loanAccount) {
        if (!bank.updateLoanAccount(loanAccount)) {
            throw new IllegalArgumentException("The object does not exist.");
        }
        return loanAccount;
    }
}

Bank.java

@Getter
@Setter
public class Bank {
    private Map<String, LoanAccount> loanAccounts;

    public boolean updateLoanAccount(LoanAccount loanAccount) {
        LoanAccount loan = loanAccounts.get(loanAccount.getId());
        if (loan != null) {
            loan.update(loanAccount);
            return true;
        }
        return false;
    }
}

LoanAccount.java

@Getter
@Setter
public class LoanAccount {
    private String id;
    private Integer numberOfInstallments;
    private LoanAccountType type;
    private Date creationDate;
    private BigDecimal loanAmount;

    public void update(LoanAccount loanAccount) {
        this.setCreationDate(loanAccount.getCreationDate());
        this.setLoanAmount(loanAccount.getLoanAmount());
        this.setNumberOfInstallments(loanAccount.getNumberOfInstallments());
        this.setType(loanAccount.getType());
    }
}

If you can't use this approach, you could made a small refactoring of the service (less code and "if conditions" are good things):

public LoanAccount update(LoanAccount loanAccount) {
    LoanAccount account = bank.getLoanAccounts()
            .stream()
            .filter(la -> la.getId().equals(loanAccount.getId()))
            .findAny()
            .orElseThrow(() ->  new IllegalArgumentException("The object does not exist."));
    account.setCreationDate(loanAccount.getCreationDate());
    account.setLoanAmount(loanAccount.getLoanAmount());
    account.setNumberOfInstallments(loanAccount.getNumberOfInstallments());
    account.setType(loanAccount.getType());
    return account;
}

Also I think you should return updated loan account.

\$\endgroup\$
2
  • \$\begingroup\$ In your last example it should be acount.set... not account.get().set (account is not a Optional). \$\endgroup\$
    – RoToRa
    Commented Oct 1, 2019 at 12:22
  • \$\begingroup\$ Fixed. Thank you. Pasted not final version. \$\endgroup\$ Commented Oct 1, 2019 at 12:37
1
\$\begingroup\$

I'd like to focus on a different aspect:

I find it dangerous using the same class LoanAccount both as a storage class inside Bank and as a data transfer class in the API, because if by accident the code on the outside gets an storage instance then it could change it's data bypassing the service.

I'd suggest to make LoanAccount an interface containing only getters and use this in the API methods. Then Bank and the caller would have their own implementations that can't be modified by the other side.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Nice point! Alternatively it is possible to introduce LoanAccountDTO and use it on Service API and outside (it has clear name). Also mapstruct can be used for transformation. \$\endgroup\$ Commented Oct 1, 2019 at 13:02

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.