Business logic in service method

Question

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<Transfer, Long> repository;
 private Repository<Asset, Long> assetRepository;
 @Override
 public void informTransferTimeIsUp(long transferId) {
 Transfer transfer = repository.get(transferId);
 transfer.informTransferTimeIsUp();
 if (transfer.isRealized()) {
 Set<Asset> 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);
 }
 }
 repository.update(transfer);
 }
}
class Transfer {
 long receiver;
 Date transferTime;
 TransferType transferType;
 Set<Long> transferredAssets = new HashSet<Long>();
 boolean accepted;
 boolean timeIsUp;
 boolean realized;
 boolean cancelled;
 public void informTransferTimeIsUp() {
 this.timeIsUp = true;
 if (accepted) {
 this.realized = true;
 } else {
 this.cancelled = true;
 }
 }
 public long getReceiver() {
 return this.receiver;
 }
 public Set<Long> getTransferredAssets() {
 return Collections.unmodifiableSet(transferredAssets);
 }
 public TransferType getTransferType() {
 return transferType;
 }
 public boolean isRealized() {
 return realized;
 }
}
class Asset {
 Long actualOwnerId;
 Long currentOwnerId;
 boolean blocked;
 public void transferTemporarily(long receiverId) {
 this.currentOwnerId = receiverId;
 this.unblockIfBlocked();
 }
 public void transferPermanently(long receiverId) {
 this.actualOwnerId = receiverId;
 this.currentOwnerId = receiverId;
 this.unblockIfBlocked();
 }
 private void unblockIfBlocked() {
 this.blocked = false;
 }
}

Dependencies to get the above code to compile:

interface TransferService {
 void informTransferTimeIsUp(long transferId);
}
interface Repository<TEntity, TKey> {
 TEntity get(TKey key);
 Set<TEntity> getAll(Set<TKey> keys);
 void update(TEntity asset);
}
enum TransferType {TEMPORARY, PERMANENT}

Is there too much business logic in the service layer?

Answer 1

Set<Asset> 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:

1. It's safe to compare enums by ==.
2. 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...

• abuzittin gillifirca: Since the use of realized and cancelled are just opposites of each other, you may want to consider having just a single boolean field. I can't just do that, because, before the transfer time they are both false, but I think you're right. There may be a tri-state enum {PENDING, REALIZED, CANCELLED} there.
• h.j.k.: @abuzittingillifirca sorry for that wrong assumption, as there wasn't enough context to realize that in the first place... I thought you are just interested to know if the transfer is 'accepted' or not.