3
\$\begingroup\$

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?

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$
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...

\$\endgroup\$
2
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ Commented May 6, 2015 at 6:29
  • \$\begingroup\$ @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. \$\endgroup\$
    – h.j.k.
    Commented May 6, 2015 at 7:57

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.