15

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.

  • 6
    You're talking about the standard java.util.Set, which returns true on add when the element wasn't already there, right? – user2357112 supports Monica Apr 7 '17 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 '17 at 18:57
  • Is there something wrong with doing two things at once? – user207421 Apr 8 '17 at 9:57
  • @user2357112 yes, that's right. – Andreas Braun Apr 11 '17 at 7:27
19

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).

|improve this answer|||||
  • 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? – Andreas Braun Apr 7 '17 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 '17 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 '17 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 '17 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 '17 at 19:25
12

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.

|improve this answer|||||
  • 2
    add returns true if the element wasn't already there, not if it was. (The question's wording is misleading.) – user2357112 supports Monica Apr 7 '17 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. – Kevin Krumwiede Apr 7 '17 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. – Reinstate Monica Apr 7 '17 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 '17 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. – BlueRaja - Danny Pflughoeft Apr 7 '17 at 19:38
8

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).

|improve this answer|||||
  • 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. – Reinstate Monica Apr 7 '17 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. – Justin Time - Reinstate Monica Apr 7 '17 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. – Reinstate Monica Apr 7 '17 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. – Justin Time - Reinstate Monica Apr 7 '17 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. – Reinstate Monica Apr 7 '17 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.

|improve this answer|||||
  • If I write if (set.contains(entry)){set.add(entry); //do more stuff} is that also eliminated by the compiler? – Andreas Braun Apr 7 '17 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 '17 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? – Andreas Braun Apr 7 '17 at 14:50
  • @AndreasBraun: exactly (upvoted that answer). – 9000 Apr 7 '17 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 '17 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?

|improve this answer|||||
  • 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? – Andreas Braun Apr 7 '17 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. – Niels van Reijmersdal Apr 7 '17 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 '17 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 '17 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. – Paul Draper Apr 7 '17 at 19:05

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.