6
\$\begingroup\$

I have this very simple method:

private boolean isTheParticipantTalking(ParticipantID participantId){
    if(mCurrentParticipant != null) {
        return participantId.equals(mCurrentParticipant.getId());
    }
    return false; // no one is talking
}

I receive a warning stating that I can simplify the code in the following manner:

private boolean isTheParticipantTalking(ParticipantID participantId){
    return mCurrentParticipant != null && participantId.equals(mCurrentParticipant.getId());
}

In your opinion, is this change desirable? I understand that we go from 4 lines of code to just 1, but I find my way more understandable in one blick.

\$\endgroup\$
  • \$\begingroup\$ It is the same as combining two if statements. I probably would. \$\endgroup\$ – user34073 Feb 16 '15 at 16:27
8
\$\begingroup\$

I prefer your method, because it's a lot clearer. But I would turn the if around (first check the input, then the normal action):

private boolean isTheParticipantTalking(ParticipantID participantId){
    if(mCurrentParticipant == null) {
        return false; // no one is talking
    }
    return participantId.equals(mCurrentParticipant.getId());    
}

Another question is if this is actually the best approach.

Is a non-existing participant talking? That question really doesn't make so much sense.

You could use Objects.requireNonNull which seems to have been written exactly for cases like this, or throw your own NullPointerException if you want to.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I ended up going for your solution. I had to add //noinspection SimplifiableIfStatement so lint would not keep on complaining. Thanks! \$\endgroup\$ – AlvaroSantisteban Feb 16 '15 at 17:25
1
\$\begingroup\$

It's hard to give an objective answer, but for me this is the winner. Concise and readable.

private boolean isTheParticipantTalking(ParticipantID participantId) {
    return 
        mCurrentParticipant != null &&         
        participantId.equals(mCurrentParticipant.getId());
}
| improve this answer | |
\$\endgroup\$
-1
\$\begingroup\$

You must understand that return means true for compiler while it concider that this expression performs, so in that case it might be true only if mCurrentParticipant != null and participantId.equals (mCurrentParticipant.getId () is true. In other cases (when mCurrentParticipant == null or when participantId.equals (mCurrentParticipant.getId () is false) it returns false which contradicts the condition than any boolean expression expected to be true for compiler, so this Android Studio solution is more clear than another ones.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ This answer is unclear. Please explain what you mean by 'return means true ...' and 'it returns false which contradicts ...' \$\endgroup\$ – Daniel Jun 3 '18 at 8:46
  • \$\begingroup\$ I know that it's so hard to explain it, so the developers don't understand simplifications like Android Studio (included) do, but I'll try again. There's only one condition when the expression which is returns by any method - is true, it's means that this expression performs and it's true. And even it must returns false (for example return !bar) it means that it's true that this expression (must be) false, so in that case (!bar) it means that "It's true that bar has false value". \$\endgroup\$ – Acuna Jun 3 '18 at 15:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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