
How not to write shitty code: Intentional Conditions - ingve
https://tewha.net/2020/04/how-not-to-write-shitty-code-intentional-conditions/
======
UglyToad
This hits on an important point I don't see discussed much and certainly not
addressed as important. Debugging is (one of) the most important UX aspects of
your code.

Similar to code being read a lot more than written, you should prioritise
debugger user experience over other aspects. One thing I try to do is have a
temporary variable called result in many places and immediately return that on
the next line. Though in some versions of Visual Studio you can faff about and
add a watch with some magic name ($return Value?) to inspect the return value
its much easier to have an easy location to chuck a breakpoint. Similarly
chaining method calls or LINQ should be broken up with easy inspection
variables so you can easily verify the state.

It seems like needless verbosity but when prod is broken and you desperately
need to fix something you'll be glad of it.

~~~
lonelappde
Writing extra error prone code just to make easier debugging seems like a bad
trade-off.

Better to learn how to use a debugger, and check preconditions, and log state
changes, and write tests.

------
spacedcowboy
For me, one of the prime drivers for how easy something is to grok, is how the
formatting of the code lays out the logic within.

He does a fair job with the last incarnation, but I’d go further than that. As
he says, compilers are smart enough to do variable storage optimisation these
days, so break up that enormous Boolean into multiple bools (sameFirsName,
sameLastName, sameDepartment, sameCompany) and then use the condition at the
end (hasChanged) to be a simple combination of those other bools. It’s easy to
lose a bracket and add it in somewhere wrong in a complicated expression.
Simpler ones are easier to see and immediately understand without effort. When
debugging, this is the coding-goal.

Now onto formatting. Do all the tests fit onto one line ? Fine, go ahead and
do that, it’s an easy chunk to grok and the variable-names ought to read
pretty much like English anyway. If they won’t fit onto one line, then for the
love of all that’s holy, line up the conditions underneath each other, so it’s
clear what logic is actually being expressed at a glance. This minimal effort
contributes greatly to code clarity, IMHO.

You get simpler statements, and you get better formatting. For me, it’s an
easy win. It’s not rocket science, it’s basically agreeing with his premises,
but taking it a step or two further down the process.

~~~
jdashg
With our drive towards auto-formatting, we are losing degrees of expressivity
of formatting. Bad code looks better, but good code can't look quite as clear
as it used to.

------
necovek
Another tip is to not be too smart in optimising your conditions either — I've
once reviewed a branch with a complex if condition that made no sense, before
realising that it was the result of applying De Morgan's rules.

So, express the intent, but do not mathematically simplify or evaluate it (let
the compiler/interpreter do that, or if it is really a performance hotspot,
describe the intent even though comments are supposed to be about whys and not
whats).

------
m463
I like perl when writing complicated conditions like this because of if and
unless.

You can not only do stuff like:

    
    
      if(condition) {...}
    

but also:

    
    
      unless (<condition>) {...}
    

you can also do stuff like:

    
    
      do_something if <condition>;
      do_something unless <condition>;
    

in other languages, you have to say "if not <condition>" which adds not
clarity.

------
04091948
I personally, would've overridden the equal function of record (or made an
equality helper function).

so the code would become

if(!record.isBuiltIn() || !user.canChange(record) ||
!record.equals(oldRecord)){ // can't save / or no need too }

and if no one can change a builtin, then a user certainly can't change it.

so it becomes:

if( !user.canChange(record || !record.equals(oldRecord))){ // can't save / or
no need too }

