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&lt;Transfer, Long&gt; repository;
    private Repository&lt;Asset, Long&gt; assetRepository;

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

        if (transfer.isRealized()) {
            Set&lt;Asset&gt; 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&lt;Long&gt; transferredAssets = new HashSet&lt;Long&gt;();

    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&lt;Long&gt; 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&lt;TEntity, TKey&gt; {

    TEntity get(TKey key);

    Set&lt;TEntity&gt; getAll(Set&lt;TKey&gt; keys);

    void update(TEntity asset);

}

enum TransferType {TEMPORARY, PERMANENT}


Is there too much business logic in the service layer?
Set&lt;Asset&gt; 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...