# Checking to see whether a document can be deleted

My code looks like this:

public void checkIfCanDelete() throws BusinessException {

boolean canDelete = canDelete();

if (canDelete) {

checkIfLocked();

if(!editable) {
}

return;
}

if (newDoc) {
}

if (getSetting().isExcludedFromDeploy()) {
}

checkIfLocked();

// Default Message:
}


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?

-
@QuakeCore what do you mean by reliance/reliance – Salah Nov 12 '15 at 9:12

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;
} else if (newDoc) {
} else if (getSetting().isExcludedFromDeploy()) {
}
checkIfLocked();
}


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())) {
newDoc ?
getSetting().isExcludedFromDeploy() ?
);
}
}

-

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) {
}
return;
}

if (newDoc) {
} else if (getSetting().isExcludedFromDeploy()) {
}

// Default Message:
}

-
Updated the code by remove the 2 elses – Brian Apr 15 '14 at 15:21

right here:

boolean canDelete = canDelete();

if (canDelete) {

checkIfLocked();

if(!editable) {
}

return;
}


you can remove the Boolean variable and code it like this

if (canDelete()) {
checkIfLocked();
if(!editable) {
}
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?

-