7 ms·
Yep, glad I read this thread. We were making the same simple mistake.
by milesokeefe 8y ago
Yep, glad I read this thread. We were making the same simple mistake.
- bigiain 8y agoWe aren't. Now. (We caught ourselves doing it 4-5 months back, and went through _everything_ checking... Only random accident that brought it to the attention of anyone who bothered to question it too... Two separate instances by different devs of 'if (DEBUG_LEVEL = 3){ }' instead of == 3 - both missed by code reviews too...)
- jcoffland 8y agoThis is why you should turn on compiler warnings and heed them. It would have caught this.
- bch 8y agoAnd consider “Yoda Notation”[0], which some people find annoying, but I found an easy hurdle to clear: if ( 3 = DEBUGLEVEL ) wouldn’t pass the the parser because you can’t assign to an rvalue. [0] https://en.wikipedia.org/wiki/Yoda_conditions https://en.wikipedia.org/wiki/Yoda_conditions
- bigiain 8y agoYep - I pointed out that I used to do this in Perl back in '95 or so. At least one of the devs wasn't born then, none of them had ever used Perl. (I'm not even sure how they'd ended up with a Grails configuration that'd let them do this anyway...)
- extralego 8y agoThanks for sharing. I am a less experienced programmer and have never seen this before. The name is so wonderful.
- marssaxman 8y agoYes! I've been doing this in C for years. It's a little weird to read at first, but every now and then it really saves you.
- lawl 8y agoIn this specific case DEBUGLEVEL should be a constant anyways, and thus assignment should fail, no? Also kind of denoted by being all caps.
- stan_rogers 8y agoConventions cause assumptions.
- anyzen 8y agoThere are always assumptions being made, no matter what you do. But "uppercase -> constant" is such a generic and cross-platform convention that it should always be followed. This code should never have passed code review for this glitch alone.
- onion2k 8y agoWhich language would stop/warn you assigning the value of a constant to a variable? Doesn't "var = const" just work in most languages?
- eloff 8y agoIt's yoda condition, so const = var would fail.
- mkagenius 8y agoApt day for discussing Yoda condition :)
- foo101 8y agoI don't think "Yoda notation" is good advice. How do you prevent mistakes like the following with Yoda notation? if ( level = DEBUGLEVEL ) When both sides of the equality sign are variables, the assignment will succeed. Following Yoda notation provides a false sense of security in this case. As an experienced programmer I have written if-statements so many times in life that I never ever, even by mistake, type: if (a = b) I always type: if (a == b) by muscle memory. It has become a second nature. Unless of course where I really mean it, like: if ((a = b) == c)
- bigiain 8y agoFWIW I'm pretty sure both the devs who did this and both the other devs who code reviewed it would claim the same thing... Like other people are saying - the toolchain should have caught this. And it should have, I don't remember how it'd been disabled...
- bch 8y agoOne way to not write any bugs is to not write any code. If you must write code, errors follow, and “defence in depth” is applicable. Use an editor that serves you well, use compiler flags, use your linter, and consider Yoda Notation, which catches classes of errors, but yes, not every error.
- tigershark 8y agoAnd if you write f# or Java code?
- baud147258 8y agoFor java code, use final so that you have constants.
- _asummers 8y agoNote these are only constant pointers. Your data is still mutable if the underlying data structure is mutable, (e.g. HashMap). Haven't used Java in a few years, but I made religious use of final, even in locals and params.
- vgy7ujm 8y agoYoda makes code more confusing to read at a glance so I would recommend against it.
- laumars 8y agoI don't really see how unless you've never actually read imperative code before; either way you need to read both sides of the comparison to gauge what is being compared. I'm dyslexic and don't write my comparisons that way and still found it easy enough to read those examples at a glance. But ultimately, even if you do find it harder to parse (for whatever reason(s)) that would only be a training thing. After a few days / weeks of writing your comparisons like that I'm sure you'll find is more jarring to read it the other way around. Like all arguments regarding coding styles, what makes the most difference is simply what you're used to reading and writing rather than actual code layout. (I say this as someone who's programmed in well over a dozen different languages over something like 30 years - you just get used to reading different coding styles after a few weeks of using it)
- vgy7ujm 8y agoConsistency is king. Often when I glance over code to understand what it is doing I don't really care about values. When scanning from left to right it is easier when the left side contains the variable names. Also I just find it unnatural if I read it out loud. It is called Yoda for a reason.
- laumars 8y agoBut again, not of those problems you've described are unteachable. Source code itself doesn't read like how one would structure a paragraph for human consumption. But us programmers learn to parse source code because we read and write it frequently enough to learn to parse it. Just like how one might learn a human language by living and speaking in countries that speak that language. If you've ever spent more than 5 minutes listening to arguments and counterarguments regarding Python whitespace vs C-style braces - or whether the C-style brace should append your statement for sit on its own line - then you'd quickly see that all these arguments about coding styles are really just personal preference based on what that particular developer is most used to (or pure aesthetics on what looks prettiest - that that's just a different angle of the same debate). Ultimately you were trained to read if (variable == value) and thus equally you can train yourself to read if (value == variable) All the reasons in the world you can't or shouldn't are just excuses to avoid retraining yourself. That's not to say I think everyone should write Yoda-style code - that's purely a matter of personal preference. But my point is arguing your preference as some tangible issue about legibility is dishonest to yourself and every other programmer.
- simias 8y agoI know it's irrational but I really dislike Yoda notation. Every time I encounter one while reading code I have to take a small pause to understand them, I don't know why. My brain just doesn't like them. I don't think I'm the only one either, I've seen a few coding styles in the wild that explicitly disallow them. Furthermore any decent modern compiler will warn you and ask to add an extra set of parens around assignments in conditions so I don't really think it's worth it anymore. And of course it won't save you if you're comparing two variables (while the warning will).
- astura 8y agoYeah, exactly. This error shouldn't ever happen, period. All modern development tools give big fat warnings when you do this.
- mkagenius 8y agoPeople (atleast me) ignore warnings quite often, they aren’t safe haven if you ask me.
- _sh 8y agoHey no problem, just add -Werror to your compiler flags (C/C++/Java) or '<TreatWarningsAsErrors>true</TreatWarningsAsErrors>' to your csproj (C#).
- kiallmacinnes 8y agoThis! Treat every warning as a failure, ideally in your CI system so people can't forget, and this problem (ignoring warnings..) goes away. You will have a better, more reliable, and safer codebase once you clean up the legacy mess and turn this on..
- acomjean 8y agoI agree. Having worked in a project with warnings as errors on (c++) I found it annoying at first but it made me a better coder in the long run. Plus you get out of the habit of not reading output from the compiler because there are so many warnings...
- maljx 8y agoUnless you follow a zero warning policy they are almost useless. If you have a warning that should be ignored add a pragma disable to that file. Or disable that type of warning if it's too spammy for your project.
- FPGAhacker 8y ago“Should” is a bad word. If you are basing a conclusion off of a “should,” you are skating on thin ice.
- bognition 8y agoseems like a bug in your platform
- inferiorhuman 8y agoI'm curious, how often do you actually need to print out the password in a development context?
- zimpenfish 8y agoI've been working on authentication stuff for the last two weeks and the answer is "more than you'd like". But luckily it's something we cover in code reviews and the logging mechanism has a "sensitive data filter" that defaults to on (and has an alert on it being off in production.)
- deleted 8y ago[deleted]