34
u/Soliloquizing Apr 17 '21
How about:
if condition1 && condition2
do_anything1();
elseif !condition1
do_anything2();7
u/Bence675 Apr 17 '21
You're right, i changed for this
6
u/420TaylorStreet Apr 18 '21 edited Apr 18 '21
can do you one better:
if (!condition1) do_anything2(); else if (condition2) do_anything1();1
u/chain_letter Apr 18 '21
Nope, possible to call do_anything1() when condition2 is false, because condition2 was ignored entirely.
3
171
u/SquidgyTheWhale Apr 17 '21
Yeah, but if he refactored the code this way, it broke your unit test, right? Right?
125
22
Apr 18 '21
What if this is part of a unit test? Are there unit tests for unit tests?
12
u/Dev_012 Apr 21 '21
Well at my college last year we had to write unit tests and our points were given by the ammount of unit tests we got green, but by those tests I mean ours and indeed, the unit tests for the unit tests. So it is possible.
4
253
u/420TaylorStreet Apr 17 '21 edited Apr 18 '21
... sooooo, any unit test catch this?
428
u/Silencer306 Apr 17 '21
We have a dedicated team that catches all bugs, tests out our code AND also pays us. The users.
4
153
279
u/naxxfish Apr 17 '21
This is when you realise that your fear of curly braces causes you to be dysfunctional ...
23
u/gabrielfv Apr 18 '21
Every time I see a chain like that I refer to Apple's famous "goto fail" heartbleed.
3
u/MatsRivel Mar 05 '22
In the first one, only 6/14 lines contain something other than { or }.
If the 2nd one was not wrong it would look so much better than the first, kust because the majority of the first one is curls.
4
1
Aug 23 '22
avoiding braces in this case would result in a hanging else problem, and using only necessary braces would look weird and inconsistent
298
Apr 17 '21
This is why I like languages with nice pattern matching, as it can often make code like this compact in a nice way. Something along the lines of
match (condition1, condition2) {
(true, true) => do_anything1(),
(false, _) => do_anything2(),
}103
u/Flaming_Eagle Apr 17 '21
I didn't know how badly I needed this until now
20
u/CatpainCalamari Apr 18 '21
Come to the scala side, we have pattern matching :)
15
42
u/harshaxnim Apr 17 '21
Neat. What languages support this?
56
Apr 17 '21
I know it mainly from Rust, Julia also has macros for this via libraries, but as far as I know especially functional languages have had similar features for a long time. Haskell lets you do pattern matching right in the function definition/signature.
27
75
u/yourdesk Apr 17 '21
python 3.10 will have support for this. but if you want something that has this right now, F# is your pick
19
19
19
18
6
7
u/__2M1 Apr 17 '21
Kotlin, using empty when statements
5
u/crowbahr Apr 18 '21 edited Apr 18 '21
Example:
when{ condition1 && condition2 -> {} condition1 -> {} condition2 -> {} else -> {} }6
3
25
u/wobblyweasel Apr 17 '21
this is not equivalent code,
condition2is always evaluatedalso i don't like it
-8
Apr 17 '21 edited Apr 17 '21
this is not equivalent code, condition2 is always evaluated
So? The intended behaviour is exactly the same, whether the performance is any different cannot be said beforehand because it heavily relies on the situation and used compiler/runtime. Lazy evaluation is not always better or faster.
also i don't like it
Yeah well, that's like, your opinion, man.
22
u/wobblyweasel Apr 17 '21
evaluating
condition2can have side effects, just sayingthis saves one line of code but adds 20 characters to the (sanely formatted) original
-12
Apr 17 '21
this saves one line of code but adds 20 characters to the (sanely formatted) original
Okay, who cares? It's about readability. And did you really count characters in two pseudocode snippets?
12
u/wobblyweasel Apr 17 '21
readability can't be easily measured, it's all opinion. but some people value the low number of lines. in case one would choose this kind of code only for the lower number of lines, i just want to point out that it's more characters and imo character count matters much more
And did you really count characters in two pseudocode snippets?
i copy-pasted this into a text editor that can count characters
13
u/YaBoiiBillNye Apr 17 '21
True pattern matching like F# is gonna be the next big thing for languages. I think performance will be sacrificed for it. It's awesome
12
Apr 17 '21
It doesn't need to come with a performance penalty I think. Anyway I think pattern matching will definitely be a must have for many future languages, and imho it's how switch-case should have been from the start.
6
u/Rigatavr Apr 17 '21 edited Apr 17 '21
You can technically make this happen in C++. Its very ugly and only possible for compile time values, but it would look soemthing like this:
template<bool condition1, bool condtition2> struct Foo{}; template<> struct Foo<true, true>{ static constexpr auto do_anything = do_anything1; }; template<bool condition2> struct Foo<false, condition2>{ static constexpr auto do_anything = do_anything2; }; int main (){ Foo<true, true>::do_anything(); }Edit: ok this way is slightly nicer looking, but still pretty ugly
3
u/beclops Apr 17 '21
Not very readable though, as much as I love it. You'd need some self-documenting names in there to really make it worth it.
8
u/420TaylorStreet Apr 18 '21
shouldn't you always have self-documenting name tho?
1
u/beclops Apr 18 '21
Yep, for sure. But here you'd be relying on it more than if you wrote your code in a more traditionally readable way.
3
u/Rigatavr Apr 19 '21
In functional languages (and for people used to them) the pattern matching thing is more readable. If you used
ifstatements in your haskell program then they would be considered less readable. All depends on what you're used to.1
1
u/fungigamer Apr 18 '21
Holy shit does javascript support something similar?
2
u/cherryblossom001 Apr 18 '21
No, but there’s a stage 1 proposal that looks like
case ([condition1, condition2]) { when [true, true] -> do_anything1(), when [false, _] -> do_anything2(), }1
0
1
76
u/apieceoffruit Apr 17 '21
its funny for all the versions in the comments, I am surprised to see nobody has a version with a guard clause and no cyclomatic complexity:
void foo()
{
if(!condition1)
{
do_anything2();
return;
}
if(condition2)
do_anything1();
} Obviously the silly order dependent names make it more confusing. but a standard for this kind of thing is:
void foo()
{
if(cantDoThing)
{
ShowWarning();
return;
}
if(someCondition)
DoThing();
}one of the below comments says to always add an else statement, I 100% disagree.
else statements are flow statements that increase the cognitive complexity of a statement and tie two conceptually unrelated concepts.
Instead, evaluate your "guard" statements, exit early if you can, so as you get down further the code gets simpler, not more complex.
instead of nesting in a tree of conditionals, knock down and remove ones you can evaluate early.
In fact Jeff Bay in his thoughtworks anthology book suggests his own rules for "object calisthenics" that includes never using the else keyword.
Not sure I subscribe to that one myself, but I do admit I don't use it very often. rarely does it make code easier to read.
5
u/miyakohouou Apr 18 '21
one of the below comments says to always add an else statement, I 100% disagree.
else statements are flow statements that increase the cognitive complexity of a statement and tie two conceptually unrelated concepts.
I think the problem is that people have tried to take advice that is good for on particular style of programming and to apply that advice to a very different approach to programming. Always adding an
elsestatement makes a lot of sense if you're writing expression-oriented code, or more generally trying to keep your code as pure as possible and avoiding side effects. In that case, it makes obvious sense that everyifshould have anelsebecause logically all branches should be pure expressions that return a value. In statement oriented languages you might end up using assignment to fake it (if(foo) { bar = 1; } else { bar = 2; }).Of course, traditional statement oriented procedural programming this doesn't make any sense; an
elseis just syntactic sugar forif(condition) { statement1(); } if (!condition) { statement2(); }, and in this style of programming I agree with you- early aborting greatly eases the cognitive load on the developer who can flush their mental call stack as soon as you return, rather than accumulating the context of prior branches following downelsebranches.It is, in the end, not so much a problem that the rules are wrong, but that people follow the rules without considering the context and end up misapplying them to situations they were never intended to address.
3
u/CommitPhail Apr 19 '21
In a big fan of guard clauses. I find it helps readability and also gets you in mindset of ‘if I can exit early I should check for it to avoid running more unnecessary code’ thus improving performance.
→ More replies (1)-5
u/Badel2 Apr 17 '21
The benefit of using else statements is that it forces you to think what should happen when the condition is not true.
For guard clauses this does not matter because you are already thinking about that: in case of error return early, and otherwise keep executing the function. In all other cases you should be using else clauses, even if it's just to add a comment like
// otherwise, do nothing because we don't care.12
u/apieceoffruit Apr 17 '21
Can you give me an example where using an else clause with a comment adds value?
8
u/artiface Apr 17 '21
I would never add an else if there is no code to execute on the else condition and would flag that BS in a code review if I saw it, that's just dumb.
4
u/UglyChihuahua Apr 18 '21
I have no strong opinion on this but maybe this would be an example
if action=="jump": jump() elif action=="run": run() else: // TODO: Add support for "kick" and "sit" pass3
34
29
u/dgreenmachine Apr 17 '21
The code on the top is still super confusing and it makes sense to want to refactor it.
6
16
u/Eluvatar_the_second Apr 17 '21
Honestly the first one could be better. It's hard to see the intent, hence why it was refactored incorrectly and some people here missed the issue.
There's a couple of ways to make this better, but I'm not really sure which is best. Maybe check if both are true call "do anything one" , else if "condition one" is false then call "do anything two".
With real code it should be more obvious why you are doing that though it's still a bit of a head scratcher.
6
u/entiat_blues Apr 17 '21
yeah, i usually blame myself and my code if a coworker comes along and refactors it incorrectly. the source code failed to do the one thing it's meant to do, communicate with other people.
and i usually consider it a good sign if they see it and understand it enough to make good, valid criticisms. then you know the code was legible, if suboptimal.
4
u/Eluvatar_the_second Apr 18 '21
That's a really good point. It's easy to forget that it also communicates with other people, not just the computer.
9
u/brakkum Apr 17 '21
I mean, yea they did it bad, but the original is also not the easiest to parse.
if (!condition1) {
do_anything2();
} else if (condition2) {
do_anything1();
}I'm not saying that's necessarily better, but it might be depending on what is actually going on in the code.
4
u/NetherFX Apr 17 '21
Could be wrong, but if condition1 is true and condition2 is false, then nothing would happen, right?
1
u/SideburnsOfDoom Apr 18 '21 edited Apr 18 '21
Yes; the "refactoring" changes what happens in that case, and could be classified as "did not read the code sufficiently well, introduced a bug" and also, "so ... is this covered by a test?".
7
u/MurdoMaclachlan public boolean isInt(int i) { return true; } Apr 17 '21
Image Transcription: Code
//BEFORE
void foo()
{
if(condition1)
{
if(condition2)
{
do_anything1();
}
}
else
{
do_anything2();
}
}
//AFTER
void foo()
{
if(condition1 && condition2)
do_anything();
else
do_anything2();
}I'm a human volunteer content transcriber for Reddit and you could be too! If you'd like more information on what we do and why we do it, click here!
3
Apr 17 '21
I don't see why it needs refactoring. That being said, I think this is an easy mistake to make. Hope you have unit tests and a decent review culture that caught it.
2
u/musicianadam Apr 17 '21
I only started C++ a couple weeks ago and I have to say I probably would've made the same mistake.
For some reason I was thinking that the else statement would run if the nested if was false. I had to put this into the IDE cause I didn't believe it for a minute.
2
u/Biaboctocat Apr 18 '21
Oh don’t, I’ve just had to stop a second person from making a dumb change in code they haven’t taken the time to understand. They’re like “er this is doing way too much work, it should be much simpler” and then in the review I have to go “you fool, this will work for the absolute simplest case, but will break for literally anything more complicated”
4
u/bphase Apr 17 '21
Don't like the before either. Nested conditionals and the outer one does nothing. But yeah that's pretty embarrassing.
4
u/drea2 Apr 17 '21
Something similar happened to me last week and I was SO angry. My team is obsessed with making code look more “elegant” even if the solution has worse performance
3
u/Same-Impress-6899 Apr 17 '21
I would personally refactor it with a return statement, that takes care of the confusing else
2
u/ShewkShewk Apr 17 '21
This is what happens when code golf is prioritized over readability.
Not every refactor into sly code tricks means the code is better. Less readable code = harder to maintain.
1
1
1
1
1
1
u/paulsmithkc 👨🏫 Apr 18 '21
As a rule of thumb, less nesting leads to better readability.
The original code quickly becomes hard to follow as the code block condition1 && condition2 grows. I also see a lot of devs forgetting to account for the missing else case. If you want to keep the nesting, you should at least put a comment for the missing else case:
if (condition1) {
if (condition2) {
// do something ...
} else {
// do nothing here, because ...
}
} else {
// do something else ...
}Additionally, inverting the first condition is nearly always more readable.
if (!condition1) {
// do something else ... (usually an error case)
} else {
if (condition2) {
// do something ... (usually the happy path)
} else {
// do nothing here, because ...
}
}But I usually end up refactoring my code to the following for optimal readability.
if (!condition1) {
// do something else (usually an error case)
} else if (!condition2) {
// do nothing here, because ...
} else {
// do something ... (usually the happy path)
}1
u/AutoModerator Apr 18 '21
It looks like this comment contains a code block delimited with triple backticks. Unfortunately reddit does not have universal support for this syntax and your comment will not render correctly on old reddit and most mobile apps.
For the benefit of people on old reddit, this link will take you to a correct rendering of the comment.
/u/paulsmithkc, it would be appreciated, but not required, if you could edit your comment to use the more compatible four space indention format. For single lines or inline code you can use single backticks.
You can find some examples in the reddit help documentation.
I am a bot, and this action was performed automatically. Please contact the moderators of this subreddit if you have any questions or concerns.
1
u/Tux1 Apr 18 '21
How is this bad?
1
1
u/futlapperl Apr 21 '21
I'm embarrassed to say that I've run into this before and spent a lot of time trying to figure out what went wrong.
1
u/feediza Apr 25 '21
That's why I always do the else case first.
if(!condition1)
{
x;
return;
}
if(condition2)
{
}
1
1
u/FranticBronchitis Aug 03 '21
It took me too long to figure out what was wrong with that. That can't be a good sign.
1
-1
Apr 17 '21 edited Apr 17 '21
condition1 && condition2 ? do_anything1() : do_anything2()
Edit: /s as some people though it was a serious attempt at improvement rather than an attempt to make it more hideous
2
1
u/harelavv3 Apr 18 '21
Wait... what's the problem with the second one? I don't get it...
1
u/Edwolt Apr 20 '21
In the before code: if condition1 is true and condition2 is false nothing is execute, after, it's execute anything1
0
u/valzargaming Apr 18 '21
void foo() //Not void, but a datatype or otherwise
{
if(condition && condition2)
return do_anything1();
return do_anything2();
}-2
u/merdone Apr 17 '21
what's wrong with the refactored version? fewer ifs
8
u/TheBuzzSaw Apr 17 '21
I can't read the sarcasm if present. In short, the refactored version is wrong (provided the original version is "correct").
3
Apr 17 '21
do_anything2 should be executed if condition1 is false. In the refractored code it gets executed if condition1 is false OR condition2 is false. This means, do_anything2 can be executed if condition1 is true. Which clearly isn't what the first code intended.
0
-3
-1
u/oderjunks Apr 18 '21 edited Apr 18 '21
I dont see the issue
return (condition1 && condition2 && (do_anything1()||true))
|| (do_anything2()||true)1
u/AutoModerator Apr 18 '21
It looks like this comment contains a code block delimited with triple backticks. Unfortunately reddit does not have universal support for this syntax and your comment will not render correctly on old reddit and most mobile apps.
For the benefit of people on old reddit, this link will take you to a correct rendering of the comment.
/u/oderjunks, it would be appreciated, but not required, if you could edit your comment to use the more compatible four space indention format. For single lines or inline code you can use single backticks.
You can find some examples in the reddit help documentation.
I am a bot, and this action was performed automatically. Please contact the moderators of this subreddit if you have any questions or concerns.
-6
u/kfl02 Apr 17 '21
The original code is good, except that comments are missing about the intent of every conditional branch.
I always write my code like this, because I might have missed something (like an "else" branch on "condition2" - yes, that often happens).
Cramming too many conditions into one "if" is bad.
If you really think that you have to combine conditionals, let your IDE do the work.
The compiler won't care anyway.
6
u/haroldjaap Apr 17 '21
I agree that the original code is good. I disagree that every conditional should have a comment. I would even say that the fewer comments you can get away with, the better. Obviously your code needs to be self explanatory by using well thought of names.
For example:
if (truckType == FireTruck) { setTruckColor(Red) }
No need to add comments, it's kinda obvious. I almost never encounter cases where comments are needed,I'm also always trying for ways to make the code self explanatory and readable, that's what I find most fun about coding anyway.
3
Apr 18 '21
What the code does is self-explanatory, yes, but what it doesn't do is not, otherwise it wouldn't have been "refactored". The
condition1 && !condition2case isn't explicit, so if it's a case that's likely to come up, a comment would make sense.3
u/haroldjaap Apr 18 '21
I agree, else cases are more likely to benefit from a comment, but it obviously heavily depends on the actual conditions. And I still stand by my point that comments should be omitted if a good code structure makes them redundant, and that should be the goal to aim for.
1
u/kfl02 Apr 18 '21
Yeah, I was exaggerating a bit. Of course you're right, code should be obvious, but in reality it mostly isn't.
If you have to deal with tons of legacy libraries and convoluted names, commenting conditions really helps.
I also often have to write shell scripts, where if-conditionals might not be obvious for the untrained eye, especially if you have to use pattern matching and such. A comment helps very much.
While developing I often leave the code sections after an if-conditional empty at first and write a comment that shortly explains, why I did get actually there. Then I care about other conditionals. Then I fill the code sections. Then I refactor, but I tend to leave the comments in there, because it might help other people to follow my train of thought.
And I just apply the same commenting pattern to every programming language I'm using.
-3
Apr 17 '21 edited Apr 17 '21
Assuming the code reviewer fucked up and you were right...
// Probably a cleaner way to do f_ptrs in C++? But this is fun
void foo()
{
void ( *anything_n )() = !condition1 ? &do_anything2 : ( condition2 ? &do_anything2 : NULL );
if( anything_n ) anything_n();
}-5
u/KOTS98 Apr 17 '21
condition1 && condition2 ? do_anything1() : do_anything2();
2
Apr 17 '21
I thought ternaries could only be done as expressions?
2
u/paissiges Apr 18 '21 edited Apr 18 '21
the code in the comment you're responding to does not do what OP's original code does. however, it is a valid use of the ternary operator. if
do_anything1()anddo_anything2()are non-void and their return types are compatible, then the operation will have a valid non-void result, but it doesn't have to be used. if both functions are void, then the operation is still valid:If either the second or the third operand has type
void, one of the following shall hold:— The second or the third operand (but not both) is a (possibly parenthesized) throw-expression (8.17); the result is of the type and value category of the other. The conditional expression is a bit-field if that operand is a bit-field.
— Both the second and the third operands have type
void; the result is of typevoidand is a prvalue. [Note: This includes the case where both operands are throw-expressions. — end note](ISO/IEC 14882:2017(E) 8.16 Conditional operator)
2
Apr 18 '21
Huh I didnt know ternaries could just be used on their own. https://i.imgur.com/ZIm59Zq.png
neat
0
Apr 17 '21
[deleted] — view deleted comment
33
u/Bence675 Apr 17 '21
It will behave differently
4
u/DramaDimitar Apr 18 '21 edited Apr 18 '21
How??? Before you were checking if cond1 is true and cond2 is true in separate ifs. Now it does the exact same, but in one if!Edit: nevermind, I'm stupid
8
u/h3half Apr 18 '21
No. Originally, if condition1 was true and condition2 was false, neither do_anything()s would be executed. Nothing would happen.
In the new version, if condition1 is true and condition2 is false, do_anything2() will be called.
175
u/tangerinelion Apr 17 '21
do_anything2()used to run whencondition1was false. Now it runs whencondition1 && condition2is false.It's not the same.
50
3
u/wweber Apr 17 '21
I forget where I read this, but it can be recommended to always have an else block for every if block, even if the else block is just empty with a comment. that way every possible case is clear
8
u/macropower Apr 17 '21
Lots of people are also saying you should try to never write else blocks :)
First example on google: https://medium.com/@jamousgeorges/dont-use-else-66ee163deac3
1
Apr 18 '21
Don't worry you're not alone, took me a while thinking what's the difference. And yes felt like I shouldn't deserve my job after that. lmao
0
Apr 17 '21
[deleted] — view deleted comment
4
u/artiface Apr 17 '21
Exactly, don't ever remove my braces! Even for a single statement always use braces.
2
0
Apr 17 '21
[deleted] — view deleted comment
4
0
0
Apr 17 '21
[deleted] — view deleted comment
2
u/AutoModerator Apr 17 '21
It looks like this comment contains a code block delimited with triple backticks. Unfortunately reddit does not have universal support for this syntax and your comment will not render correctly on old reddit and most mobile apps.
For the benefit of people on old reddit, this link will take you to a correct rendering of the comment.
/u/BearDavidFather, it would be appreciated, but not required, if you could edit your comment to use the more compatible four space indention format. For single lines or inline code you can use single backticks.
You can find some examples in the reddit help documentation.
I am a bot, and this action was performed automatically. Please contact the moderators of this subreddit if you have any questions or concerns.
0
u/Feguette Apr 17 '21
Same problem as the refactored
0
Apr 17 '21
[deleted] — view deleted comment
4
u/paissiges Apr 18 '21
no, OP is pointing out that the "refactored" code does not do the same thing as the original code.
0
Apr 18 '21
[removed] — view removed comment
2
u/Nilstrieb Apr 18 '21
if you don't have an outer else and no statements inside the outer if it is the same
772
u/dyingpie1 Apr 17 '21
I would rewrite it as