1. 5
  1. 15

    I love it when some “simple” static blog require JavaScript to load the page so I spend 2 more seconds waiting and can’t use reader mode to swap out that unreadable (for me) dark theme. But hey, it’s only 1.36M of JavaScript to display 18K of text. Of course, I understand that this is required because after all these years HTML5 still doesn’t have a native way to render text.

    Sorry, this is really off-topic, but good grief… What kind of performance art is this?


    Anyway:

    So in this example:

    if (checkValidity(card) || checkCredit(user, card) && isActive(user))  // Bad
    	doStuff(user, value);
    else
    	doOtherStuff(user);
    
    if (isEligibility(user))  // Better
    	doStuff(user, value);
    else
    	doOtherStuff(user);
    

    Splitting out the conditionals in a separate function isn’t necessarily better; you have a single action you want to do (“charge a user’s credit card”), and the more you can read that from top to bottom, the better. Putting conditionals in different functions isn’t necessarily all that helpful, especially not if they do three things and have a vague grammatically awkward name like isEligibility().

    I would sooner refactor that to something like:

    if validCard(card)
    	return givePrize()
    if isActive(user) && hasCredit(user, card)
    	return givePrize()
    
    noPrizeForYou()
    

    Not entire sure if that’s the intended logic as the example seems a bit strange and contrived (and I can never remember if it’s && or || that has higher precedence), but something along these lines keeps the entire logic readable from top to bottom, and also avoids the else. I don’t like else, and avoid them when possible. The more you can read something from top to bottom step-by-step, the better, and else often makes that harder. The “stack size” of my memory isn’t very large, and I like to put as few variables on it as possible. I have the feeling this applies to most people.

    I don’t think this particular conditional is all that hard in the first place by the way; other than the missing parens to clarify the precedence.

    Of course, if you want to call that in many places it might be better to have a isEligibility() function; it depends. But the idea that “complicated conditionals should be their own function” is not a good one IMO; if you have a single business logic that happens to take 60 lines of code then that’s okay, as long as you keep it easily readable: the more you split that up, the harder that piece of logic becomes to understand.


    In general, I find that I care less and less about “code style” and stuff like that. What makes code hard to work with isn’t that some variables are snake_case while others are camelCase. I mean, it’s not great, but it’s not that big of a deal either and it’s certainly not the kind of stuff that makes me go “wtf is this shit?! I can’t understand the hell this does?!”

    This kind of stuff isn’t “refactoring”, it’s “making your code look pretty”. And sure, it’s nice when code looks pretty, but it’s not really the most important thing, and I’ve seen some people introduce very real actual bugs in codebases which ended up costing a lot of lost customers/money with this kind of “code needs to look pretty” pseudo-refactoring. Don’t fix it if it ain’t broke, and especially not if “fixing” just means “fits my personal aesthetics”.

    If “bad code” works, then just let it be. Don’t worry about it until you need to. “Refactoring” a function that has been working with 0 problems in production and doesn’t require any changes (or just a tiny change) is just a waste of time and money, or worse, a liability. This is definitely a mistake I’ve made in the past myself because it certain can be tempting and satisfying to make an ugly function look pretty, but that doesn’t actually mean you’re doing something useful.

    Also, who cares about some leap year calculation function that last changed in 1582?

    1. 4

      The things that actually need to be refactored that I’ve seen cause the most problems aren’t really due to code style, but are due to poorly designed spaghetti code without tests written by people who didn’t understand what the actual constraints of the product were.

      The one that really stands out to me is an “event” system I had to deal with that didn’t make any distinctions between something that happened once and something that was ongoing. So things that were ongoing would just repeatedly notify the user every 30 seconds. Not fun to redesign.

      1. 1

        ‘Poorly defined spaghetti code’ is an element of style. It’s one of the more important elements of style.

        Nobody goes “ love Dostoevsky’s writing style: He indented his paragraphs so beautifully, and wrapping at 120 characters is so much better than Joyce’s 80 character line width.”.

        1. 3

          I was thinking that’s more structure, whereas style is something a linter helps with. If you structure your essay as one giant paragraph, no one’s going to read it, even if it has perfect grammar. :-)

          Either way, these are just different ways of describing technical debt when the article makes a big deal about aesthetics, which is the least important problem.

      2. 1

        If “bad code” works, then just let it be. Don’t worry about it until you need to.

        I agree, but some bad code is fine-ish until someone else needs to work with it. If that is the case for a large part of the code-base, something needs to be done. The question I guess is how to weigh the risk of breaking things by refactoring against the risk that something breaks if a new programmer tries to fix something in existing code.

        1. 1

          It depends on what you need to do with the code, and how often you need to do it. For code that has been working fine for years and needs a small addition or bugfix and it’s unlikely that you need to work on it frequently in the future: just do what you need to do and leave it be outside of that. I’ve seen people spend entire days or even weeks refactor modules that needed a very small change that could have been done easily without the refactor. At that point, you’re just rewritten working code to … working code, with little concrete benefit.

          If it’s code that you need to work with every week and you’re being hampered by the way it’s written: yeah, that should probably be cleaned up, if possible. A lot of refactoring I’ve seen (and also done) didn’t really reduce the risk of faults; it just made the code a bit nicer and easier to work with. Especially refactors like in this article are mainly a time-saving tool, rather than a fault-reduction tool.

          Let’s not forget that code isn’t a work of art and that the objective isn’t its elegance or beauty. The way some people talk about code and treat it might leave you with different impressions, but it’s just a tool to solve problems, and refactoring is a tool to make it easier to solve problems. If you’re spending a day on refactoring to perhaps maybe possibly save 30 minutes should you perhaps possibly maybe have to touch this in the future, then you’ve invested your time rather poorly IMO.

          On the other hand, if you can spend a day refactoring and this will save you an hour every week, then in 8 weeks you’ve recouped your time investment.

        2. 1

          and I can never remember if it’s && or || that has higher precedence

          It’s && that has higher precedence (and in terms of bitwise arithmetic in C-like languages, & has higher precedence than |). The way I like to remember it is that in terms of 0 and 1 boolean values, AND corresponds to multiplication, while OR is like saturating addition. Multiplication has higher precedence than addition.

        3. 1

          I like the suggestions to start with what you can’t break, such as comments and vertical spacing, but before applying reformatting (aka. destroying programmers’ signs of intent) and adding yet more abstractions (which have their own drawbacks), I would emphasize one more thing that has no drawback: See if the code can be simplified!

          Apply logical equivalences (such as De Morgan and deduplication) until you have micro-optimized the logic down to what it really means. The mentioned if/else reordering trick to avoid negation is a good one, but merely a tool in this box. If the code was bad enough, you get a cascade effect.

          Cascading simplification: This…

          if (isupper(a)) {
          	if (cond) {
          		cout << '_' << tolower(a);
          	} else {
          		cout << tolower(a);
          	}
          } else {
          	cout << a;
          }
          

          …simplifies to:

          if (isupper(a) && cond) {
          	cout << '_';
          }
          cout << tolower(a);