patternjavaMinor
Checking to see whether a document can be deleted
Viewed 0 times
cancheckingseedeleteddocumentwhether
Problem
My code looks like this:
In my case if
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
So is there any way to enhance this code block? Do I need to call the same method twice in the same method block?
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
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:
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:
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.