Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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?

share|improve this question

3 Answers 3

up vote 7 down vote accepted

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;
        );
    }
}
share|improve this answer

I agree, with Malachi by moving the variable to the if statement, since its not being used anywhere else.

If they are both performing a checkIfLocked() on the file, wouldn't it be better to perform this first, then continue one with the other checks?

    public void checkIfCanDelete() throws BusinessException {

      checkIfLocked();

      if (canDelete()) {
        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);
      } else if (getSetting().isExcludedFromDeploy()) {
          throw new  BusinessException(BusinessException.MSG_DOCUMENT_CANNOT_DELETE_A_READ_ONLY_DOCUMENT);
      }

      // Default Message:
      throw new BusinessException(BusinessError.MSG_YOU_ARE_NOT_AUTHORIZED_TO_REMOVE_DOCUMENTS);
    }
share|improve this answer
    
Updated the code by remove the 2 elses –  Brian Apr 15 at 15:21

right here:

boolean canDelete = canDelete();

if (canDelete) {

    checkIfLocked();

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

    return;
}

you can remove the Boolean variable and code it like this

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

if the return value of the function is a boolean you should be able to just call the function inside the expression of the if statement.

that is the only thing that I could see.

I don't know what checkIfLocked() does, it would be nice to see that as well.


I don't know what all checkIfLocked() and canDelete() do, but I would imagine that you should just have checkIfLocked() inside of your canDelete() function and eliminate the call to it from this checkIfCanDelete() Method.

If it's locked you can't delete it right?

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.