Timeline for Developer insists if statements shouldn't have negated conditions, and should always have an else block
Current License: CC BY-SA 3.0
37 events
| when toggle format | what | by | license | comment | |
|---|---|---|---|---|---|
| Oct 11, 2023 at 10:26 | comment | added | Stef | "The presence of the return statement makes things worse." Or, maybe, with the presence of the return statement it's easier to spot an error in the code with the empty else than without. int f(int x) { if (x == 3) { return 12; }} is wrong, but it's easy to miss it; whereas int f(int x) { if (x == 3) { return 12; } else { } } is wrong, and it's easy to spot the missing return. So I would say that's actually an argument in favour of the colleague's recommended empty else block. | |
| Feb 5, 2021 at 9:29 | comment | added | Piovezan | "It is not false that testing for a closed door is more intuitive than testing for a non-opened door." The irony ;) | |
| Jun 15, 2017 at 15:23 | audit | First posts | |||
| Jun 15, 2017 at 15:24 | |||||
| Jun 13, 2017 at 23:13 | audit | First posts | |||
| Jun 13, 2017 at 23:13 | |||||
| S Jun 13, 2017 at 16:26 | history | suggested | John Doucette | CC BY-SA 3.0 | fixed grammar. |
| Jun 13, 2017 at 15:15 | review | Suggested edits | |||
| S Jun 13, 2017 at 16:26 | |||||
| Jun 13, 2017 at 14:39 | comment | added | user3067860 | if (!this.IsMaster || (!this.Ready && !this.CanWrite)) -- you are not fixing anything, you're just moving it somewhere else. So you would have, isSlave = !isMaster; isSynchronizing = !ready, readOnly = !canWrite; if(isSlave || (synchronizing && readOnly)) ... But !master might not = slave, you can be master, slave, or something else, etc. So you wouldn't want to use slave = !isMaster, you would want notMaster = !isMaster, so now you have if(notMaster || (notReady && notCanWrite)) -- you have replaced 3 !s with 3 lines of assignment and the word "not" instead of an !... | |
| Jun 12, 2017 at 16:20 | comment | added | mbrig | @Nelson I'm very much inclined to agree, which makes me wonder what the MIRSA rules (which require explicit else, at least in versions I've seen) based themselves on. Was it just the writer's/committee's personal feelings, or was it based on some kind of studies? | |
| Jun 12, 2017 at 14:05 | comment | added | Baldrickk | @Nelson what about if(x) {do_stuff();} {/*This space intentionally left blank*/} ? | |
| Jun 12, 2017 at 0:09 | comment | added | Justin Time - Reinstate Monica | I'd just like to point out that an empty else block won't necessarily incur three wasted lines, depending on the indent style. It would be 0 or 1 with 1TBS or similar styles, for example, depending on whether the programmer puts the closing brace of empty blocks immediately after the opening brace or on a separate line. Still distracting either way, though. | |
| Jun 11, 2017 at 21:36 | comment | added | Arseni Mourzenko | @Brad: you're absolutely right (and this point was already mentioned in comments in the past). I edited my answer. | |
| Jun 11, 2017 at 21:33 | history | edited | Arseni Mourzenko | CC BY-SA 3.0 | added 12 characters in body |
| Jun 11, 2017 at 20:35 | comment | added | Brad | You want to create additional properties that are just in the reverse of ones that already exist? That's absolutely ridiculous, in my opinion. It's as if you want to add confusion (as if they had different core meaning) and failed to complete some sort of refactor. | |
| Jun 11, 2017 at 8:19 | comment | added | Kyslik | @Veedrac exactly, use De Morgan hammer and have single negation instead of three. | |
| Jun 10, 2017 at 18:50 | comment | added | Ant P | I think the rule shouldn't be "don't test for negations" but "express conditions in their simplest form," i.e. isFish is better than !isNotFish but there is absolutely nothing wrong with !isFish. | |
| Jun 10, 2017 at 12:12 | comment | added | leftaroundabout | I disagree that if x {doA(); return a;} doB(); return b; is better than if x {doA(); return a;} else {doB(); return B;}. In fact I'd argue this should probably be refactored to return x? presentA() : presentB(), which makes it completely clear that A and B are alternative options for the result. | |
| Jun 9, 2017 at 21:17 | comment | added | Panzercrisis | Most of this answer I agree with, but it seems like duplicating properties of classes to have positive and negative versions would violate DRY somewhat and increase complexity. | |
| Jun 9, 2017 at 14:35 | comment | added | David Hammen | @hobbs - At least in the limited the domain of high reliability computing, people are primed to almost always expect the else. That an if doesn't have an else is generally taken as a sign of a potential problem with the code. An empty else helps to alleviate those concerns. | |
| Jun 9, 2017 at 12:03 | comment | added | Veedrac | Personal practice is to pull negations up to top level. if (!(this.IsMaster && (this.Ready || this.CanWrite))). | |
| Jun 9, 2017 at 9:06 | comment | added | BlueRaja - Danny Pflughoeft | Your last example can be rewritten without creating new properties (which is often not an option): bool isReady = this.Ready || this.CanWrite; if(!this.IsMaster || !isReady) ... | |
| Jun 8, 2017 at 23:02 | comment | added | Paul | @Mark that's less clear, even with the comment, than saying if (!commonCase) { handle the uncommon case }, though. | |
| Jun 8, 2017 at 20:51 | comment | added | Mark | The other reason I'll do an empty "if" is to make it explicit that nothing is being done. It gives me someplace to hang a comment: if(common case) { /*do nothing*/ } else {handle the uncommon case} | |
| Jun 8, 2017 at 19:47 | comment | added | SoylentGray | So If I have tests that also test for is master should I create a property that returns !IsSlave? What if it is nullable and I need to test for that too should I create yet another property just so I can test is true and be more readable? | |
| Jun 8, 2017 at 19:29 | comment | added | hobbs | It's not about "three lines of code", it's about writing for the appropriate audience (someone with at least a basic grasp of programming). An if is already explicit about the fact that the condition could be false, or you wouldn't be testing for it. An empty block makes things less clear, not more, because no reader is primed to expect it, and now they have to stop and think about how it could have gotten there. | |
| S Jun 8, 2017 at 18:55 | history | suggested | Deduplicator | CC BY-SA 3.0 | added syntax-highlighting; minor fixups |
| Jun 8, 2017 at 18:19 | review | Suggested edits | |||
| S Jun 8, 2017 at 18:55 | |||||
| Jun 8, 2017 at 18:09 | comment | added | Casey | It might also make sense to swap out if(!cond) {doB();} else{doA();} for if(cond) {doA();} else{doB();}. | |
| Jun 8, 2017 at 17:15 | comment | added | Machado | One other thing I'd do in the name of code readability is to create another local boolean to handle the whole "this.IsSlave || (this.Synchronizing && this.ReadOnly)" expression. Something like "var isImmutable = this.IsSlave || (this.Synchronizing && this.ReadOnly);" and then test like "if (isImmutable)". That would make things clearer for the next unfortunate developer who have to read my code. | |
| Jun 8, 2017 at 17:09 | comment | added | Machado | "create additional properties with clear names": Unless you have deeper analysis of the domain, this may be changing the global scope (the original class) to solve a local problem (the application of a specific business rule). One thing that can be done here is to create local booleans with the desired state, such as "var isSlave = !this.IsMaster" and apply your if with the local booleans instead of creating several other properties. So, take the advice with a grain of salt and take some time to analyse the domain before creating new properties. | |
| Jun 8, 2017 at 17:00 | history | edited | Arseni Mourzenko | CC BY-SA 3.0 | added 998 characters in body |
| Jun 8, 2017 at 16:32 | comment | added | 8bittree | @Neil Negating a condition does not necessarily require any additional CPU time. C compiled to x86 could just swap the jump instruction from JZ (Jump if Zero) for if (foo) to JNZ (Jump if Not Zero) for if (!foo). | |
| Jun 8, 2017 at 15:23 | vote | accept | Patsuan | ||
| Jun 8, 2017 at 14:36 | comment | added | Neil | Negating a condition requires an operation, taking CPU time. However, the extra CPU time is hardly worth considering. Rather, performing a branch (jump) is expensive (at least relative to the negation), justifying perhaps to put the more likely to happen condition in the if block rather than the else block. | |
| Jun 8, 2017 at 14:24 | comment | added | Nelson | Empty blocks always look like mistakes to me. It'll immediately draw my attention and I'll ask 'Why is this here?' | |
| Jun 8, 2017 at 13:59 | history | edited | Arseni Mourzenko | CC BY-SA 3.0 | added 307 characters in body |
| Jun 8, 2017 at 13:53 | history | edited | Arseni Mourzenko | CC BY-SA 3.0 | added 307 characters in body |
| Jun 8, 2017 at 13:46 | history | answered | Arseni Mourzenko | CC BY-SA 3.0 |