7
irene
4y

Code review today and the senior says "Avoid comments. Putting the procedure in a well named function where it can modify those class properties will work just as well."

ARE YOU KIDDING ME? YOU ARE GOING TO PREFER CODE OVERHEAD OVER COMMENTS? I AM SO TRIGGERED RIGHT NOW I CAN'T

Comments
  • 5
    Code overhead?
  • 12
    Comments can lie, code can't, I'm with your senior on this one... Even if they're accurate to begin with, there's no guarantee they'll be updated if something changes.
  • 0
    @spongessuck Two choices here.

    Make a method and name it in a descriptive way. Move the single-purpose procedural code into the new method. Call the new method from the other method. There is zero chance of possible method reuse.

    Or just add a single comment which gets ignored at compile time.

    Fuck everything about adding pointless code complexity because you cant handle a comment in code.
  • 0
    @CptFox that’s kind of dumb. You are less likely to change the name of a method randomly than a comment. The method name will definitely be more stale long term.
  • 0
    @CptFox literally this change

    foo(){
    // Descriptive stuff about next line
    Procedure.goes.here();
    Procedure.also.goes.here();
    More.procedure();
    }

    Versus this

    descriptiveStuff(){
    Procedure.goes.here();
    Procedure.also.goes.here();
    }
    foo(){
    descriptiveStuff();
    More.procedure();
    }
  • 8
    @irene If you have at all decent refactoring tools, changing a method name is just as trivial as updating a comment.
    Heck, that might help you find misuses of your newly updated method of the changes are breaking in any way. But if you're making breaking changes to the method's abstract behaviour, I'd say renaming it isn't what you should worry about (most of the time, at least).

    Still, by "code doesn't lie", I meant that using methods, you can abstract complex code from its context in order to make it more readable. Of course a method name can be misleading, but it's much easier to read a few short method to understand what they really do than a single big one.

    Code is already expressing ideas, why repeat yourself with comments that might become misleading in the future?

    But as always, there may be exceptions where comments are valuable, maybe this was such a case and your senior just needed to bitch about something. I was mostly playing devil's advocate
  • 0
    @irene okay then, I'd probably go with neither commeOts nor split, to be fair; unless these abstractions are extremely complex x)
  • 1
    Sounds like pedantry on his part. Wonder what he would make of this >.<
  • 0
    @SortOfTested He is anti rxjs as well. Hah
  • 1
    @irene
    You could put him in the ground for great justice. I'd totally protest outside your trial. :D
  • 0
    @CptFox The procedure is to collect and set some specific stuff in the browser’s current state so the next stuff that is done is up to date. It only happens once in a very specific situation.

    Just when you look at it you can’t tell why it is doing what it is doing.
  • 0
    @SortOfTested That means a lot to me.
  • 0
    @irene so why can't you rename the name of the function to reflect its purpose? Why code movement is even required?
  • 0
    @iiii A comment is so you can add a comment that isn’t code. If you are writing a function so that you can try to avoid adding a comment that is how idiots write code.

    The point isn’t whether or not you can do it because there are lots of dumbass things that a person can do.

    collectAndUpdateSessionStorageForLanguageSettingsFromAuthClaims()

    Versus

    // collect local storage for property comparisons
    // if the current regionalization properties in local storage exist determine if a region has changed by comparing it to the auth claims
    // if a language change is found or claims are out of date load the regionalization module with the updated settings

    Some shitty function name isn’t going to explain why I’m examining the“tokenMun” or why I’m comparing “tokenMun” to “sessionRtc” in an auth header or etc. That is what comments are for. Holding onto things that aren’t explicitly obvious.
  • 0
    @irene so probably your code is not descriptive enough
  • 0
    @iiii Not at all. The local storage in the browser has a bunch of mystery values that are all important but aren’t descriptive. They need comparison and a whole procedure to fit with business rules.

    I could rename them all by saving them as variables but then I would have to translate them all back from the invented names if I have to update the storage.
  • 0
    Why not both? Conclusive names, readable code to show the what, comments to show the why. That's how I do it.
Add Comment