17
gitpush
6y

!rant

My colleague saw this code of mine and said it is annoying conditions are not on the same line, since it is a shared project I can't go "that's my style!"

What do you think should I convince him why we need to follow this approach (at least it can be easily read rather than reading a screen wide line)

Comments
  • 0
    @ThatDude from the speed of your reply I'm guessing this is your first impression, thanks man 😀
  • 5
    If there’s more then one condition then yes, multi line works a lot better for readability otherwise your scrolling right for a while trying to work out what it’s doing, Especially when you start adding brackets in to group sections together.
  • 1
    @C0D4 exactly I just hope for one thing though, if he agreed to gow tih this style, that neither me or him forget to add { } if we have only one line inside the IF condition, imagine how bad it will be for a hide and seek game lol
  • 3
    I agree with your coworker sort of, it does look bad.

    If you want to split conditions to new lines (like you should) then also split the first one like so:

    if (
    a == 1 ||
    b == 2
    ) {
    ...
    }

    That was it's much easier on the eyes and makes more sense structure wise I think
  • 0
    @Froot I understand, I'll try and see but ya I guess it will be easier for the eye
  • 2
    My company's eslint config would complain about this. I think it's this rule: https://eslint.org/docs/rules/...
    I think we have this rule set to ['error', 'none'].

    I've seen my coworkers use lodash's some method to go around this, which has very bad performance, so I'll also have to make a case to change this rule for boolean operators.
  • 1
    @shellbug why they don't allow this type? Any history of issues about it?
  • 2
    @gitpush I don't know but they usually have good reasons for these rules. I'll ask when I'm back from vacation and post back their response.
  • 4
    Personally, when this happens to me I usually move the condition into a method, so in this case this would be:

    if (comment.isValid())

    or if comment is a plain js object:

    if (isValidComment(comment))

    This way the if statement stays clean and readable.
  • 4
    @gitpush I tend to always put the {} in when using multi conditionals so I can tell where it starts and ends a little easier visually.

    I was more meaning for the grouping brackets... when for example:

    If(
    ( a + b == c ) ||
    ( a * c == b )
    ){
    print(“wtf”);
    }

    Is used.
    Breaking them up is so much nicer to deal with, when there’s a few lines of conditions.

    @Froot as for your example, I think we have something in common 😎
  • 1
    @shellbug I usually go for that approach (isCommentValid)
    But in this case it is not a reusable condition, and I really appreciate if you post back the "why they force that rule" 😀

    @C0D4 I see, I like what you and @froot had, I'll be trying it out 😀
  • 2
    @gitpush will do (I hope I don't forget 😁).
  • 1
    @shellbug I'll be bugging you don't worry 😛
  • 1
    I like it. The
    (
    )
    Approach seems more like line noise to me
  • 3
    @gitpush so I asked my team lead about this. The rule is to try to avoid long if conditions, because a short if is always more readable than a long if, inline or not.
  • 1
    @shellbug I totally forgot about it 😅
    But how can you avoid a long condition? Can you give me a case?
  • 2
    @gitpush @shellbug

    Not an answer for how to avoid a long if but suggested to avoid conditional if you can.

    I am doing my best to follow and practice this guide.

    https://github.com/jupeter/...
  • 3
    @gitpush take a look at the "encapsulate conditionals" section of the guide @CurseMeSlowly posted.
  • 2
    @CurseMeSlowly @shellbug so what guys are suggesting I move my validation to inside the model, so that I do:
    If (comment.isValid())

    Instead of what I have in my screenshot?

    This brings me to another question, in this case my business rule is now set inside the data model, it is not considered as a bad practice?
  • 3
    @gitpush if your model is meant to only store the data, yes, it's bad. In that case you can do

    if (this.isValidComment(comment))

    Or you could create some kind of validator that you could re-use and do

    if (commentValidator.isValid(comment))
  • 2
    @shellbug it is for storing data only.in this case I'll go with validators as you suggested thanks man 😀
  • 2
    @CurseMeSlowly thanks for the link, halfway through it and I like what I'm reading :)
  • 2
    @gitpush right? Right? 😬 It is like karma sutras for PHP developers 😳

    😂😂😂
  • 2
    @CurseMeSlowly I just hope everyone follows similar practices 😒
    I'm already half way through clean architecture book and man, I've been doing few things wrong for sooooooo long 😓
  • 2
    @gitpush I found it quite late too and I feel so ashamed of all my previous works 😓
  • 2
    @CurseMeSlowly well better late than never I guess 🤔
Add Comment