Business logic in service method

I'm currently writing a small asset tracking system.

The implementation should meet the following requirements:


The system; scans the transfers every 15 minutes. If the transfer is
accepted by the receiver by the specified transfer time, realizes the
transfer. When a transfer is realized, assets are assigned to the
receiver as of transfer time; if the transfer is permanent, actual
owner is changed also.


The system; If the transfer operation is not accepted by the receiver
by the specified transfer time; cancels the transfer.

Above requirement is dependent on this straight-forward requirement (UI related parts omitted):


Transferring user; When a new transfer is started,
[...]
and specifies

• the time when the transfer will be realized

• whether the transfer is temporary or permanent

• assets to be transferred

• the receiver

• [... and saves ....]

The implementation:

```
import java.util.Collections;
import java.util.HashSet;
import java.util.Set;

import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;

@Transactional
@Service
public class TransferServiceImpl implements TransferService {

private Repository repository;
private Repository assetRepository;

@Override
public void informTransferTimeIsUp(long transferId) {
Transfer transfer = repository.get(transferId);
transfer.informTransferTimeIsUp();

if (transfer.isRealized()) {
Set transferredAssets = assetRepository.getAll(transfer.getTransferredAssets());
for (Asset asset : transferredAssets) {
if (TransferType.TEMPORARY.equals(transfer.getTransferType())) {
asset.transferTemporarily(transfer.getReceiver());
} else {
asset.transferPermanently(transfer.getReceiver());
}
assetRepository.update(asset);
}
}

reposi

Set transferredAssets = assetRepository.getAll(transfer.getTransferredAssets());
for (Asset asset : transferredAssets) { ... }

Since the transfer isn't done yet, and also to prevent ambiguity post-transfer, may I suggest renaming the method to getAssets()? You can also inline the call:

for (Asset asset : assetRepository.getAll(transfer.getTransferredAssets())) { ... }

Two minor improvements I can suggest for the snippet below:

• It's safe to compare enums by ==.

• Have a transfer(TransferType, long) method in Asset so that the toggling is done within Asset: callers only need to supply the TransferType value.

if (TransferType.TEMPORARY.equals(transfer.getTransferType())) {
    asset.transferTemporarily(transfer.getReceiver());
} else {
    asset.transferPermanently(transfer.getReceiver());
}

Since the use of realized and cancelled are just opposites of each other, you may want to consider having just a single boolean field, to keep the implementation simpler.

if (accepted) {
    this.realized = true;
} else {
    this.cancelled = true;
}

In Asset:

public void transferTemporarily(long receiverId) {
    this.currentOwnerId = receiverId;
    this.unblockIfBlocked();
}

public void transferPermanently(long receiverId) {
    this.actualOwnerId = receiverId;
    this.currentOwnerId = receiverId;
    this.unblockIfBlocked();
}

This can be slightly simplified as:

public void transferTemporarily(long receiverId) {
    this.currentOwnerId = receiverId;
    this.unblockIfBlocked();
}

public void transferPermanently(long receiverId) {
    transferTemporarily(receiverId);
    this.actualOwnerId = receiverId;
}

Alternatively, if reading temporarily inside a permanent method is slightly confusing, have a new method setCurrentOwnerAndUnblock() to do that instead...

StackExchange Code Review Q#88763, answer score: 2