[FIXED] Is there a better way to write my if statement?

Issue

I have a function that checks if multiple conditions are fulfilled before performing an operation. The code below shows all conditions that must be fulfilled.

        if (!user.isPresent()) {
            throw new ResourceNotFoundException(MessageUtil.ERROR_USER_NOTFOUND);
        }

        if (!account.isPresent()) {
            throw new ResourceNotFoundException(MessageUtil.ERROR_ACCOUNT_NOT_FOUND);
        }

        if (account.get().getStatus() == AccountStatus.CLOSED) {
            throw new BadRequestException("Error. This account is closed");
        }

        if (!user.get().getGroups().contains(group)) {
            throw new BadRequestException("Error. You are not a member of this group");
        }

This code works perfectly as is. However, is there a better way of writing my if statements? Something about the look of it just feels wrong, feels verbose.

Solution

Perhaps the question is specifically about '4 ifs', in which case the answer is mostly no: This is fine - there is no particular commonality amongst the 4 conditions you check, nor is there any particula commonality amongst the 4 code blocks.

Given that you have Optional, you could rewrite it but the result isn't any "shorter" in terms of the amount of AST nodes (the 'complexity' of the code). Your formatting guidelines may dictate that you could write it in 'fewer lines', but that just means your style guidelines are silly, not that mapping off of optional is somehow 'better'. There's a reason that many style guides dictate that you must write braces after an if. Whatever that reason may be, it's hard to follow how it doesn't also apply to optional .map and .orElse. Hence, it's a style guide thing, not an objective improvement.

But, just for argument's sake:

if (!user.orElseThrow(x -> new ResourceNotFoundException(MessageUtil.ERROR_USER_NOTFOUND)).getGroups().contains(group)) {
  throw new BadRequestException("Error. You are not a member of this group");
}

if (account.orElseThrow(x -> new ResourceNotFoundException(MessageUtil.ERROR_ACCOUNT_NOT_FOUND)).getStatus() == AccountStatus.CLOSED) {
  throw new BadRequestException("Error. This account is closed");
}

This is certainly shorter. It's also harder to read and harder to modify in light of new feature requests, so it's almost certainly worse. Contrast it to this code:

 if (!user.isPresent()) throw new ResourceNotFoundException(MessageUtil.ERROR_USER_NOTFOUND);
 if (!account.isPresent()) throw new ResourceNotFoundException(MessageUtil.ERROR_ACCOUNT_NOT_FOUND);
 if (account.get().getStatus() == AccountStatus.CLOSED) throw new BadRequestException("Error. This account is closed");
 if (!user.get().getGroups().contains(group)) throw new BadRequestException("Error. You are not a member of this group");

You tell me - which one is easier to understand/change? I think the second, by miles. Which should mostly tell you that (assuming you hold to this), any style rule that if statements must have braces is misguided and should be perhaps be discarded, or at least adjusted to allow 'fast return' (i.e. if the body is throw, continue, break, or return, braces are optional. This is a nuance many more modern style guides do have). Point is: I don't think this qualifies as 'better', so not an actual answer to your question. Even if it is, under some style guides, 'shorter'.

However, I think this code can be improved in a significant way (significant: Results in fewer bugs, is considerably easier to understand/follow, and can more easily be modified in the face of changing requirements) but we need to enlarge the scope a little bit.

It appears you call getAccount() somewhere which apparently returns an Optional<Account>. Or, possibly, this method has a parameter of type Optional<Account>.

instead of having a method public Optional<Account> getAccount(), instead, have a method public Account getAccount() throws ResourceNotFoundException, BadRequestException. This method will return the account if it exists, and throws an exception right away if it does not. This way, you've isolated the code: Instead of writing if (!account.isPresent()) throw new ....(MessageUtil...) in 81905 different places in your code base, you now write it just the one time, in the getAccount() method. The assumption being that the vast majority of callers who ask for the account would want to throw that if that isn't a (valid) account. In passing this method can even check for CLOSED state and throw the appropriate error right away too. This avoids repetition and mistakes: What if some code by accident forgets to check for a closed account?

Then, for the rare cases where you want code that can fetch an account even if that account is closed, or which wants to continue even if there is no account (and possibly branch out someplace), make a separate method with a different name that sufficiently bells-and-whistles the requirements to follow when using it (needs to indicate to the caller that they need to pay some attention, as they'll have to do alllll the required validation of that account, including presence of it, themselves).

Same applies to a user (it's not clear what the semantic difference is between a 'user' and an 'account' here - in most systems there are synonyms. All advice above applies just the same to accounts as it does to users).

Now you can get rid of 3 ifs, and an optional unwrap in the rest of the code that follows the snippet you wrote (because surely after your 4 if clauses you actually do something with user.get()). And get rid of it not just here, but in many places. The one check that remains is the group check. You could even make a getUserIf() method that you pass not just whatever you need to fetch the user, but also an additional group param, and the getUserIf method will throw that BadRequestException if that user isn't a member of the group, but at some point you have to stop writing such 'utility-fetch' methods. The right ones are near universally applicable to a vast array of situations. If, in your system, 'group check' is 40%+ likely to happen when you have any reason to retrieve a user object, then make that method. If not, then don't.

Answered By - rzwitserloot
Answer Checked By - Clifford M. (JavaFixing Volunteer)