16

The add operator of the set class returns a boolean which is true if the element (which is to be added) wasn't already there, and false otherwise. Is writing

if (set.add(entry)) {
    //do some more stuff
}

considered good style in terms of writing clean code? I am wondering since you do two things at once. 1) adding the element and 2) checking whether the element existed.

4
  • 6
    You're talking about the standard java.util.Set, which returns true on add when the element wasn't already there, right? Apr 7, 2017 at 16:38
  • 1
    I would usually consider the opposite test: if (!set.add(entry)) {// entry already present, possibly a case you want to handle}
    – njzk2
    Apr 7, 2017 at 18:57
  • Is there something wrong with doing two things at once?
    – user207421
    Apr 8, 2017 at 9:57
  • @user2357112 yes, that's right. Apr 11, 2017 at 7:27

5 Answers 5

20

Yes, it is.

The usual point of having an operation return a Boolean value is that you can use it to make decisions, i.e. within an if construct. The only other way you could realize this potential would be to store the return value in a variable and then immediately reuse it in an if, which is just silly and certainly not in any way preferable to writing if(operation()) { ... }. So just go ahead and do that, we won't judge you (for that).

13
  • Thanks for your answer. As @Niels vanReijmersdal points out in his answer doing this breaks the command and query separation, doesn't it? Don't you think that that matters? Apr 7, 2017 at 13:55
  • 4
    @AndreasBraun personally I think command query separation is dumb. It is often logical for one method to both perform an action and returning a value related to that action. Enforcing an artificial separation between the two leads to unnecessarily verbose code.
    – user82096
    Apr 7, 2017 at 15:08
  • 1
    @dan1111: indeed. Further, “command and query separation” leads to the “check then act” and similar anti patterns, which can break in multi-threaded contexts. It’s rather strange to advertise such a separation, when the rest of the world is trying to build atomic fused operations for robust and efficient SMP solutions at the same time…
    – Holger
    Apr 7, 2017 at 17:25
  • 2
    @Jörg W Mittag: page 759 of what? Since an atomic operation is intrinsically immune against the “check then act” anti-pattern, it doesn’t look beneficial to split it it up just to read an “entire chapter” of whatever to find out, how to make the construct thread safe again. Since you didn’t name any actual arguments, I have no “specific objections to those arguments”.
    – Holger
    Apr 7, 2017 at 18:27
  • 1
    @Jörg W Mittag: Well, I’m talking about the answer on this page, discouraging using Set.add as single operation without naming an alternative. If the intended operation is to add a single element and find out whether it has been added, there is no need for a more powerful alternative that still complicates the operation without benefit. I hope you are aware that Set.add is a builtin JRE method that can be used without studying a >700 page book first.
    – Holger
    Apr 7, 2017 at 19:25
13

I would say it isn't as clean as possible, because it forces the maintainer to either already know or look up what the return value signifies. Does it mean the value already existed, didn't already exist, was successfully inserted? If you don't use it a lot, you won't know, and even if you do, it's that much more mental load.

I would prefer the following:

boolean added = set.add(entry);

if (added) {
    //do some more stuff
}

Yes, a tad more verbose, but the compiler should generate pretty much the exact same bytecode, and even people who haven't used Java sets in years can follow the logic without looking anything up.

15
  • 2
    add returns true if the element wasn't already there, not if it was. (The question's wording is misleading.) Apr 7, 2017 at 16:36
  • 9
    Looking up a method is less of a burden than trying to understand the original developer's intention for a redundant variable declared outside the scope where it's used. In all modern Java IDEs, a developer can hover their mouse pointer over a method and instantly access its documentation. Good developers do this constantly when working with any unfamiliar API. Apr 7, 2017 at 17:05
  • 9
    I second @Holger. If you don't know Set.add, you're inexperienced and you should be looking these methods up to learn them and become more experienced. Apr 7, 2017 at 18:04
  • 7
    notAlreadyPresent is not the best wording. I would use added, and expect reader to know why a value would not be added to a Set.
    – njzk2
    Apr 7, 2017 at 18:59
  • 3
    @JustinTime: Using comments to describe what code does is widely considered bad practice. Your code should be self-describing. In a code review, I would flag your example as unacceptable. Apr 7, 2017 at 19:38
9

If true means success, then it's good, clear code.

There is a widespread convention that a function or method returns true (or something that evaluates to true) on success. As long as your code follows that, I think putting the method in the conditional is fine.

Code like this is unnecessarily cluttered in my view:

boolean frobulate_succeeded = thing.frobulate();

if (frobulate_succeeded) {
    ...
}

It feels like you are repeating yourself.

However, the question is ambiguous on the meaning of the return value. You say "a boolean which indicates whether the added element already existed", which might imply that true means the element existed (and add did not occur). If that is the case, I would ideally change the method's return behavior to be more conventional. If that isn't possible, I would add an extra intermediate variable that allows you to clearly label the return result in your code (as suggested by others).

7
  • What does success mean in the context of adding an item to a set? Not throwing an exception means success; true vs. false means something more specific in this context. Apr 7, 2017 at 18:04
  • @Solomonoff'sSecret In this case, an exception means the set refused to add the element, false means that the element wasn't added because it was already contained, and true means the element wasn't already contained. As add() adds the element to the set if the set didn't already contain it, true therefore means that the element was successfully added to the set. Apr 7, 2017 at 18:32
  • @JustinTime You are adding your own value judgements to the two cases. Another interpretation is that success is that the item is in the set when the method returns. If the item was already in the set, add succeeds without having to do anything. If the item wasn't already in the set, add succeeds by adding the item to the set. Which interpretation is correct is arbitrary. The less arbitrary definition of success is the one from the language: the method succeeds if it returns normally. Apr 7, 2017 at 18:38
  • 1
    The least arbitrary definition of success is that the method successfully performed the task it set out to perform. If I have a method firstLessThanSecond(int l, int r), and it returns true if l > r or false if l <= r, then that method is not successful, even though it returns normally. Apr 7, 2017 at 18:51
  • 1
    @JustinTime add is a severe abbreviation of the contract. The documentation starts with "Adds the specified element to this set if it is not already present", which is satisfied whether or not the item was already in the set. You are free to interpret the return value however you want but ultimately it is just your interpretation. And in your second example the method evaluates whether a condition is true. If the condition is false the method certainly has not failed, as it has successfully evaluated the conditional. Apr 7, 2017 at 19:09
2

I'd say it's very C-like. Most of the time I would prefer to have a descriptively-named variable for a mutation result, and no mutation happening in an if condition.

A compiler will eliminate this variable if it's immediately reused. A human will have easier time reading the source; to me, it's more important.

Should somebody have to extend the condition by adding an and / or clause to the if condition, they may end up not calling .add() in certain cases due to short-circuit evaluation. Unless short-circuiting is specifically anticipated, this may end up as a bug.

5
  • If I write if (set.contains(entry)){set.add(entry); //do more stuff} is that also eliminated by the compiler? Apr 7, 2017 at 14:06
  • 1
    @AndreasBraun: I don't think so; the compiler has no idea about the semantic link between contains and add. Also, it does double work of looking up entry in the set; this may play a role for very large sets and very light loops.
    – 9000
    Apr 7, 2017 at 14:09
  • 1
    So I take it you would rather not write if (set.contains(entry)){set.add(entry); //do more stuff} but instead go with e.g. Karl Bielefeldt's answer? Apr 7, 2017 at 14:50
  • @AndreasBraun: exactly (upvoted that answer).
    – 9000
    Apr 7, 2017 at 15:10
  • 1
    a good point. Mutations in ifs are tricky, as a future dev may had other conditions with short-circuits.
    – njzk2
    Apr 7, 2017 at 19:03
0

Your code seems to break Command Query Separation. This is discussed in the Clean Code book and the Function Structure video. So from a Clean Code perspective I think it is not considered good style.

“Clean code is simple and direct. Clean code reads like well-written prose. Clean code never obscures the designer’s intent but rather is full of crisp abstractions and straightforward lines of control.

-- Grady Booch author of Object Oriented Analysis and Design with Applications”

For me the intent of your code is unclear. Is the if both executed when the entry is added succesful, or also when it already exists? What does add() return? the item? The error-code?

6
  • 2
    Well, yes that's what I thought as well. But I also think @Kilian Foth has a point. If I wanted to seperate query and command I would have to write if (set.contains(entry)){set.add(entry); //do more stuff} which seems kind of silly. What's your take on this? Apr 7, 2017 at 13:45
  • I don't know the domain and context of this code to suggest a good name, but I would wrap this in a function with clear intent. For example: doesEntryExistOrCreateEntry(entry), this could contain your contains() + add() logic and return a boolean. Similar question here: softwareengineering.stackexchange.com/questions/149708/… which discusses this practise outside of clean code. Apr 7, 2017 at 13:56
  • 6
    I think the command query separation rule is dumb, particularly when applied to a case like this, where a method both performs an action and returns the success state of the action. But also, you don't explain what the rule is or provide much in the way of justification for it. Downvoted for these reasons.
    – user82096
    Apr 7, 2017 at 16:23
  • 1
    "What does add() return?" I expect any java developer who does code review to know the Collection api.
    – njzk2
    Apr 7, 2017 at 19:04
  • 3
    Agreed with @dan1111. It's especially dumb because it is the sort of things that makes fertile ground for race conditions. Apr 7, 2017 at 19:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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