5 ms·
> You may have seen code which misunderstands how expressions work: > if isDelivered and isNotified: isDone = True else: isDone = false; > Instead of: > isDo
by cgriswald 7y ago
> You may have seen code which misunderstands how expressions work:
> if isDelivered and isNotified: isDone = True else: isDone = false;
> Instead of:
> isDone = isDelivered and isNotified
Are people actually finding code like this in professional work or is this just an example? I'm self-taught and know I've got some gaps, but this example is so fundamental I find it shocking.
- thecleaner 7y agoCan confirm. Made this mistake myself actually. However to make a contradictory point apart from being better style what are the other advantages of not using if statements and use boolean algebra instead ?
- munchbunny 7y agoVerbosity tends to hurt readability, but trying to cram too much into one line tends to create something where nobody remembers how it works either. So it's really a judgement call on which approach reads more naturally. My personal rule of thumb on this is that single statement conditions aka "a = {condition}" should be using only "and" operators or only "or" operators and should generally avoid nesting unless there's a clear explanatory comment. So "a and b and c and d" is ok but "a and b or c and d" is sketchy. The reason is that "all" and "any" are idiomatic, but "some" takes more mental work to parse. Also it's always useful to keep in the back of your mind: !(a || b) == !a && !b Whenever I spot either side of the equation in a condition, I stop and consider whether the alternate formulation would be more readable.
- dymk 7y agoOh, yes, I saw stuff like this in Big Blue Company's source control. At a different company, I found stuff like: ZERO_INDEX = 0 ONE_INDEX = 1 return arr[ZERO_INDEX]
- humanrebar 7y agoThat's usually a sign of overly aggressive linters and static analyzers. They probably got a "no magic numbers" diagnostic. The solution to this problem is to add some sort of `NOLINT` comment to that line or, if possible, the comment that turns off that particular check. Then you link to the design document describing what "arr" is and how its interface involves accessing indices 0 and 1. Or you could wrap "arr" up in a class or function of some sort so the code is theoretically more self documenting.
- dymk 7y agoWe had no linters enforcing the "no magic constants" convention. If we did, I'd question the sanity of the engineer who added that linter. The reason we got code like this was because of outsourced contractors cargo-culting something they saw elsewhere.
- humanrebar 7y agoWell, maybe it's cargo culted from a place with such a linter. Or ornery peer reviewers who think their job is to be a human linter. Correcting this sort of stuff is the job of a senior engineer, though. If it's extremely frustrating, I'd become independently wealthy or stay away from technical leadership positions.
- BurningFrog 7y agoYeah, linters that make "you should usually do X" into "you MUST ALWAYS do X" cause a lot of terrible code.
- torgard 7y agoIt took me a while to understand the whole "no magic numbers" thing. I was basically just told that most (if not all) numbers should be stored in constants. So I just added int twentyFive and stuff, and it annoyed me, but my teacher was happy. Much later, I realized the missing part: Name the constant not what it is, but rather what it does. So int rightBoundary instead of twentyFive, and the code suddenly becomes much more readable.
- leetrout 7y agoYes. And a coworker wrote his own string join method in Python (and was mad he wasn’t a senior engineer).
- itronitron 7y agoduring his job interview or after he was hired? (jk)
- transreal 7y agoI see this in professional work. The former reads more like English, and so there feels like there is an argument to be made that it is more readable, and also an argument that "the compiler will optimize it for you anyway".
- wisienkas 7y agoand you question yourself if it was better to pack that readability into a function or not. Also why would anyone store a boolean named isDone rather than just returning it as the contract of the function
- Biganon 7y agoAre they wrong? To me, the long version is valid precisely because it is explicit (more to me than the short version), and will be optimized
- 010101010101 7y agoMany, many times.
- munchbunny 7y agoI've seen it in professional work. If it's actually a single variable conditional, that's irritating but by itself not worth more than a comment in the PR. However, in practice it's rarely this cut and dry. I usually see conditions expanded like this for clarity reasons. For example, I consider this kosher if it's in some business logic... instead of writing: > return a && !(b || c) || d; I've seen (pardon the formatting, I'm typing on my phone): > if (d) {return true} > else if (a) {return !(b || c);} > else {return false} It's usually a choice to be verbose for clarity.
- kingludite 7y ago> return d || a && !(b || c);
- kingludite 7y ago// a b c d // * * * 1 true // * 1 * 0 false // * * 1 0 false // 1 0 0 * true // 0 * * 0 false if (d) {return true } if (b||c) {return false} if (a) {return true } return false
- userbinator 7y agoI think that would be clearer as "return a && !b && !c || d". Also, your second example doesn't directly correspond with the first, because condition 'a' is evaluated first. The major problem with code that's "overly branchy", as in containing a lot of if/else, is that you're forced to go through each case when trying to understand how it works, and it often proliferates into even more branchy code as someone makes a bugfix (with another if/else) in one of the cases, but neglects to see that a similar if not identical change must be made to some of the others. In other words, if/else cases like your second example optimise for micro-readability when what's often important in debugging and understanding is macro-readability.
- munchbunny 7y agoWhile I get your point, several thoughts: 1. I'm talking about acceptable code, as opposed to "the best choice". There are good reasons to go for verbosity. Depending on the problem domain I might agree with your expansion of the parentheses, but when we get to this type of discussion, usually my overwhelming reaction is "this is bikeshedding," because... 2. If it's "overly branchy" code and you're worried about causing macro-readability issues, the answer is to refactor, not to compress. Modifying inline conditions runs just as much of a risk of becoming inscrutable. You choose between following branches and enumerating binary tables. If it's showing up too many places, you're likely extracting either expression into its own function. When overly branch code happens, my experience is that the root cause is underthinking/overthinking the method, not taking time to design the right high level abstractions, or as you mentioned a case of too much repeating yourself. Generally, the fixes for those issues don't have that much to do with your choice of boolean expression vs conditional.
- onion2k 7y agoIn my 20something years of writing software for a living I've seen both. And I find it very hard to care which one someone on my team uses. It's not something fundamental. Both lines work perfectly well. They do the same thing. The second is more concise and 'better', but if the only improvement you can suggest in a code review is something like shortening a line then the code is basically fine. A difference between good programmer and a bad programmer is that the good one writes working, readable, human understandable code that does the job it's supposed to do. That's it. Someone who writes code that's shorter without being more understandable, or faster, or significantly more efficient, isn't better.
- agumonkey 7y agoYou have a point but the issue with redundant boolean code like above is that it shows a lack of metalevel thinking. kinda like this python exagerated code: def sum(a,b): return operators.add(a,b) It can lead to bloat.
- injb 7y agoThat's not really a fair comparison. The first example is a fairly natural way that a reasonable person could model the logic in their head. Knowing that your programming language provides a more concise way is an optimization. A basic one admittedly, but a mere optimization nonetheless. In contrast, your example is kind of the opposite situation - it's an example of forgoing the most natural, human representation in favour of a highly Python-specific one. In other words, the first one happens when you know the abstract logic, but don't know your tools very well; your example happens when you know lots of useless details about the tool but you're struggling with the abstract logic. Actually, the example from the article suggests to me that the author misunderstands what I regard as the most fundamental programming skill: knowing the precedence of correctness over all other concerns, including efficiency. It's better to attach the right parts together badly than to attach the wrong parts like a pro.
- nicoburns 7y agoI've worked on codebases where people consistently did things in 3-6 lines that could have been done in one (without it being an overly complex line). It was massively detrimental to readability, because it meant what should have been a 5 or 6 line function suddenly became 20 or 30 lines. A 20 line function is suddenly 100 lines. And you had to wade through each part to work out what it was doing rather than just glancing at it and it being obvious...
- ilovepeppapig 7y agoThe worst production code I've seen (Java) included its own Object interface and a corresponding ObjectImpl (IIRC it was an abstract class).
- yawboakye 7y agoI've seen variations of it. Usually along the lines of let isBoolean = false if (booleanComparisonOrComputation) { isBoolean = true } ...
- drchewbacca 7y agoIs anyone willing to explain the second line? I get the first means "if isDelivered and isNotified are both true then set isDone to True, if not set it to false". However I've never seen a variable being set "isDone = isDelivered" as part of a logical test. Personally I think programmers who are "too clever" are cancer inside a codebase. Sure you saved a bunch of keystrokes but your code golf has locked the code forever unless someone is willing to rewrite it. This is not a bad example but in general I think people who strive to do things in less characters are highly problematic.
- whiddershins 7y agoIt feels clever if you aren’t used to it but it is super common. isDelivered && isNotified always resolves to a Boolean, either TRUE or FALSE So you are just saying isDone = TRUE or isDone = FALSE that’s all
- LaserToy 7y agoI tend to agree that, given compiler doesn’t care most of time, saving characters can’t justify decrease in readability.
- edflsafoiewq 7y agoIt isn't (isDone = isDelivered) and isNotified, it's isDone = (isDelivered and isNotified). It's just an expression being assigned to a variable.
- drchewbacca 7y agoAh ok, great, thanks.
- ravenstine 7y agoYes, absolutely. Even the biggest Fortune 500 companies hire interns and juniors to code in relatively complicated apps and systems. Sooner or later they hire more mid-to-senior level engineers to clean up the mess. That example isn't even bad, either. At least you can understand it by reading it. If you've never had to spend days deciphering an ouroboros of several nested loops and if-statements in 2000+ line functions in classes with multiple layers of inheritance written by someone(who has since left the company) with a lack of experience or talent, you are one lucky individual IMO.
- whiddershins 7y agoI have that all throughout a codebase. I felt momentarily bad about it, and myself, and then realized I have a reason: The codebase I am maintaining is intended to be readable by people who have extremely varying levels of skill, and who have no intention of ever being professional programmers. When possible I write stuff so you don’t have to “think like a programmer” to understand it. I’m pleased I use the conditional version more often than not, because I believe it is far more intuitive.
- itronitron 7y agoyes, and that's a pretty tame example. Don't assume that any code you see is correct. A lot of misunderstandings about how a language works can be carried on for decades within a team.
- CodeWriter23 7y agoCame here to comment on that too...and was surprised how many replies you had where the fundamental whooshed right over all these skilled programmer’s heads. The shortcoming is failure to recognize your object has more than 2 states and the use of flags should be abandoned in favor of a state variable (or status bit vector, aka a state variable).
- userbinator 7y agoI've seen plenty of if(!(var != true) == false) and the like --- some people just seem to not really understand the concept of booleans.
- Izkata 7y agoIt often goes much further than that. I was a teacher's assistant in the intro-to-programming courses in college for 3 years, and eventually realized what students were seeing. Now, with some new hires, I'm seeing the same thing: They think the expression is "if (.. == ..) { .. }" Not "if (..) { .. }" The simple explanation of what boolean expressions are, and that any boolean expression can go in those parens - and that it's the same thing as storing the result to a variable and then using that variable - is like a lightbulb suddenly blinks in their head.
- kyberias 7y agoThis is extremely common.
- lasereyes136 7y agoAll the time. There are a lot of reasons people write really bad code and being a bad programmer is only one of those reasons.