HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Checking to see whether a document can be deleted

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
cancheckingseedeleteddocumentwhether

Problem

My code looks like this:

public void checkIfCanDelete() throws BusinessException {

    boolean canDelete = canDelete();

    if (canDelete) {

        checkIfLocked();

        if(!editable) {
            throw new BusinessException(BusinessError.MSG_YOU_ARE_NOT_AUTHORIZED_TO_REMOVE_DOCUMENTS);
        }

        return;
    }

    if (newDoc) {
        throw new BusinessException(BusinessException.MSG_DOCUMENT_CANNOT_DELETE_A_NEW_DOCUMENT);
    }

    if (getSetting().isExcludedFromDeploy()) {
        throw new BusinessException(BusinessException.MSG_DOCUMENT_CANNOT_DELETE_A_READ_ONLY_DOCUMENT);
    }

    checkIfLocked();

    // Default Message:
    throw new BusinessException(BusinessError.MSG_YOU_ARE_NOT_AUTHORIZED_TO_REMOVE_DOCUMENTS);
}


In my case if canDelete is true, then I have to call checkIFLocked and throw exception if not editable.

Here I think that I have a duplicate code witch is throwing the same exception, or I did called the same method twice in the same method block, which is checkIfLocked.

So is there any way to enhance this code block? Do I need to call the same method twice in the same method block?

Solution

I assume that checkIfLocked() returns void, and throws a BusinessException if the document is locked.

There is one way to succeed, and many ways to fail. I think it would be beneficial to rearrange the code to make it obvious what the criteria for success are:

public void checkIfCanDelete() throws BusinessException {
    if (editable && canDelete()) {
        checkIfLocked();
        return;  // Can delete
    }

    // Can't delete.  We just have to choose a reason for the denial.
    if (canDelete()) {
        assert !editable;
        throw new BusinessException(BusinessError.MSG_YOU_ARE_NOT_AUTHORIZED_TO_REMOVE_DOCUMENTS);
    } else if (newDoc) {
        throw new BusinessException(BusinessException.MSG_DOCUMENT_CANNOT_DELETE_A_NEW_DOCUMENT);
    } else if (getSetting().isExcludedFromDeploy()) {
        throw new BusinessException(BusinessException.MSG_DOCUMENT_CANNOT_DELETE_A_READ_ONLY_DOCUMENT);
    }
    checkIfLocked();
    throw new BusinessException(BusinessError.MSG_YOU_ARE_NOT_AUTHORIZED_TO_REMOVE_DOCUMENTS);
}


The code above preserves the same logic as the original. However, if you're not picky about which reason you pick for the denial, you could simplify the code further:

public void checkIfCanDelete() throws BusinessException {
    checkIfLocked();

    if (!(editable && canDelete())) {
        throw new BusinessException(
            newDoc ?
                BusinessException.MSG_DOCUMENT_CANNOT_DELETE_A_NEW_DOCUMENT :
            getSetting().isExcludedFromDeploy() ?
                BusinessException.MSG_DOCUMENT_CANNOT_DELETE_A_READ_ONLY_DOCUMENT :
                BusinessError.MSG_YOU_ARE_NOT_AUTHORIZED_TO_REMOVE_DOCUMENTS;
        );
    }
}

Code Snippets

public void checkIfCanDelete() throws BusinessException {
    if (editable && canDelete()) {
        checkIfLocked();
        return;  // Can delete
    }

    // Can't delete.  We just have to choose a reason for the denial.
    if (canDelete()) {
        assert !editable;
        throw new BusinessException(BusinessError.MSG_YOU_ARE_NOT_AUTHORIZED_TO_REMOVE_DOCUMENTS);
    } else if (newDoc) {
        throw new BusinessException(BusinessException.MSG_DOCUMENT_CANNOT_DELETE_A_NEW_DOCUMENT);
    } else if (getSetting().isExcludedFromDeploy()) {
        throw new BusinessException(BusinessException.MSG_DOCUMENT_CANNOT_DELETE_A_READ_ONLY_DOCUMENT);
    }
    checkIfLocked();
    throw new BusinessException(BusinessError.MSG_YOU_ARE_NOT_AUTHORIZED_TO_REMOVE_DOCUMENTS);
}
public void checkIfCanDelete() throws BusinessException {
    checkIfLocked();

    if (!(editable && canDelete())) {
        throw new BusinessException(
            newDoc ?
                BusinessException.MSG_DOCUMENT_CANNOT_DELETE_A_NEW_DOCUMENT :
            getSetting().isExcludedFromDeploy() ?
                BusinessException.MSG_DOCUMENT_CANNOT_DELETE_A_READ_ONLY_DOCUMENT :
                BusinessError.MSG_YOU_ARE_NOT_AUTHORIZED_TO_REMOVE_DOCUMENTS;
        );
    }
}

Context

StackExchange Code Review Q#47239, answer score: 7

Revisions (0)

No revisions yet.