this post was submitted on 05 Jan 2025
61 points (90.7% liked)

Programming

17810 readers
445 users here now

Welcome to the main community in programming.dev! Feel free to post anything relating to programming here!

Cross posting is strongly encouraged in the instance. If you feel your post or another person's post makes sense in another community cross post into it.

Hope you enjoy the instance!

Rules

Rules

  • Follow the programming.dev instance rules
  • Keep content related to programming in some way
  • If you're posting long videos try to add in some form of tldr for those who don't want to watch videos

Wormhole

Follow the wormhole through a path of communities !webdev@programming.dev



founded 2 years ago
MODERATORS
top 50 comments
sorted by: hot top controversial new old
[–] 1hitsong@lemmy.ml 24 points 1 week ago (2 children)

I've been rallying against clever code for years!

Sure, it makes you have less lines for your l33t code solutions, but in the real world, it sacrifices the maintainability of code that others will eventually work on.

Between a clever 1 line fix and maintainable 10 line fix, I'll choose the 10 line every time.

[–] curbstickle@lemmy.dbzer0.com 9 points 1 week ago (1 children)

As an extensive commenter, I completely agree.

I need to know wtf I was doing, making it convoluted to save a few lines is pointless.

[–] pcouy@lemmy.pierre-couy.fr 10 points 1 week ago (1 children)

It's often a good idea to make the code itself very explicit through verbose function and variable names, rather than writing comments that could lead to inconsistencies between code and comments (by not updating the comments at the same time as the code) (see "Fallacious Comments" from the catalog)

[–] curbstickle@lemmy.dbzer0.com 4 points 1 week ago (1 children)

"Some people do a bad job commenting and updating comments, so lets not do comments" is not an approach that works for me.

Most of my code is at the prototype level. I'm concepting something out, usually paired with hardware.

If someone can't follow what I'm doing, its going to lead to problems. If a change happens to the hardware being controlled, code will not be good enough on its own.

Rather than being accepting of bad commenting practice, make comments (and updating them properly) part of good practice. In my experience, It saves time in the long run and leads to better code at the end.

[–] pcouy@lemmy.pierre-couy.fr 9 points 1 week ago (1 children)

That's not what I said. I said that comments can often (but not always) be replaced with good and explicit names.

This can be pushed to some extreme by making functions that only get called at a single place in the code, just for the sake of being able to give a name to the code that's inside (instead of inlining it and adding a comment that conveys the same informations as the function's signature)

It's definetly not for everyone, but for beginners/juniors it gives something objective they can aim for when trying to build good coding habits

[–] curbstickle@lemmy.dbzer0.com 5 points 1 week ago* (last edited 1 week ago) (1 children)

I am going to disagree, comments should be an explanation.

The code is what's being done, a comment should be why its being done.

[–] pcouy@lemmy.pierre-couy.fr 6 points 1 week ago* (last edited 1 week ago) (5 children)

I'm not sure how we disagree. At least, I don't disagree with you. My whole comment was talking about "what" comments. "Why" comments are a very good thing to have where they're needed

load more comments (5 replies)
[–] MonkderVierte@lemmy.ml 6 points 1 week ago

10 lines is a bit much, that's hardly more readable than one.

Then again, it depends on the language.

[–] Boomkop3@reddthat.com 20 points 1 week ago* (last edited 1 week ago) (3 children)

Some of these mostly just someone's opinion, and I don't quite agree with all of them

[–] FizzyOrange@programming.dev 4 points 1 week ago (1 children)

Any specific ones? I've seen this before and I thought I would feel the same way as you before I read them, but actually the vast majority are pretty basic things that are not really arguable.

It's definitely nice to have a list like this to point inexperienced colleagues to in code reviews. It's a bit more authoritative than "trust me bro, I've written a lot of code".

[–] Boomkop3@reddthat.com 3 points 1 week ago (2 children)

To preface, I think it's best to focus on what the right approaches are. Not on what to avoid. And when you see a student making a mistake, showing them how a different approach is handier (if possible) is what I suggest you do.

Having something to point at doesn't help much

vertical separation

This one argues against organizing your code in a way that shares variables are in one place. There are arguments to be made either way, but normally you'd scope your variables in a way that the ones specific to a particular bit of code are not accessible from elsewhere.

null check

Suggest writing a custom class to do what most languages can solve with inheritance or even better: the ? syntax.

inconsistent names / styles

Yes, it can be annoying. No, clarity is more important than insisting on removing that extra underscore.

complicated Boolean expression

They're advocating the use of a function to replace an expression. Sometimes this works, but the task of a boolean expression is not always easily expressed in a couple words. And so you can end up with misleading function names. Instead, just put a comment in the code.

callback hell

Not even a code smell. It's an issue from back when languages like JavaScript didn't support promises yet, but callbacks were popular. Cose got hard to read with a little complexities.

[–] FizzyOrange@programming.dev 2 points 1 week ago (4 children)

There are arguments to be made either way, but normally you’d scope your variables in a way that the ones specific to a particular bit of code are not accessible from elsewhere.

Sounds like you agree with that one to me? I'm not sure I follow their arguments about regions there (I've never used regions), but the example of declaring a variable in a block way before it is every used is spot on. I've seen code written like that and 99% of the time it's a bad idea. I think a lot of it comes from people who learnt C where you have to do that (or maybe Javascript which has weird rules for var).

Suggest writing a custom class to do what most languages can solve with inheritance or even better: the ? syntax.

Yeah I'll give you that one. They even suggest using Optional as a solution, which is what their "smelly" code did in the first place!

Yes, it can be annoying. No, clarity is more important than insisting on removing that extra underscore.

Not sure what your point is here. Of course inconsistent naming is a code smell. Do you want inconsistent names?

They’re advocating the use of a function to replace an expression. Sometimes this works, but the task of a boolean expression is not always easily expressed in a couple words. And so you can end up with misleading function names. Instead, just put a comment in the code.

Erm, yeah that's why this is a code smell. They aren't saying never have complex boolean expressions - just that if you do you'd better have a good reason because probably you'd be better off splitting it up into named parts.

callback hell - Not even a code smell. It’s an issue from back when languages like JavaScript didn’t support promises yet, but callbacks were popular.

Indeed, so now it is a code smell.

[–] Boomkop3@reddthat.com 2 points 1 week ago (1 children)

Not sure what your point is here. Of course inconsistent naming is a code smell. Do you want inconsistent names?

Of course not. But in some (uncommon in my experience) cases method names can be unclear or just plain impractically long. In such cases, I would rather see an exception to the rule than having to rely on a comment to explain the name choice.

I had a great example a couple months back, but I can't remember it right now. But here's a (bad) example of such a situation.

An example of this could be a button that triggers a click. You might call it BtnClick. Then the click event for it could be BtnClickClick. In this case, I'd rather see BtnClick_click. Ugly? Yes. Bad example? yes. But the idea is that it's more clear that the _Click action is seperate from the name.

[–] FizzyOrange@programming.dev 1 points 1 week ago

an exception to the rule

There is no rule. Smells are not rules.

[–] Boomkop3@reddthat.com 1 points 1 week ago

Indeed, so now it is a code smell.

Fair enough, code from a different era smells different

[–] Boomkop3@reddthat.com 1 points 1 week ago (1 children)

They aren't saying never have complex boolean expressions...

That's not what I'm saying either. But I think this is to be judged on a case by case basis. And it can depend on your understanding of the context. I think there's simply too much nuance here to just say "this smells"

[–] FizzyOrange@programming.dev 1 points 1 week ago (1 children)

I disagree. I've seen very complex boolean expressions and they were clearly code smell. Sometimes acceptable but definitely a fertile area for refactoring.

Their example is crap to be fair - two comparisons is not complex.

load more comments (1 replies)
[–] Boomkop3@reddthat.com 1 points 1 week ago (1 children)

There are arguments to be made either way, but normally you’d scope your variables in a way that the ones specific to a particular bit of code are not accessible from elsewhere.

They're arguing to do this:

int field = 1;
void may() {
   do(field);
}

int field = 3;
void you() {
   do(field);
}

int field = 3;
void be() {
   do(field);
}

int field = 7;
void happy() {
   do(field);
}

rather than

int field = 1;
int field = 3;
int field = 3;
int field = 7;

void may() {
   do(field);
}
void you() {
   do(field);
}
void be() {
   do(field);
}
void happy() {
   do(field);
}

A bad example of encapsulation would be:

class AClass {
    private class HelloThere {
         int a = 1;
         int b = 3;
         int c = 3;
         int d = 7;
         void DoStuff(AClass self) {
              Do(a, b);
         }
    }
    private HelloThere field = new();
    void World() {
        field.DoStuff(this);
    }
}

Of course, there is nuance here. Is this class encapsulating enough that it's got a right to exist? That'll depend on the situation.

Also, c has local static variables. Depending on your use case, it might just be easier in c than in C# and similar.

// a method with a state, horrid in some contexts, great in others
void PrintCounter() {
    static int count = 0;
    Print(count);
    count += 1;
}

And just in case you're still reading and curious:

#region PingPong
    // hi! I am in a region, collapse me using your ide!
#endregion
[–] FizzyOrange@programming.dev 1 points 1 week ago (1 children)

// a method with a state, horrid in some contexts, great in others

Definitely another code smell!

https://luzkan.github.io/smells/global-data

[–] Boomkop3@reddthat.com 1 points 1 week ago* (last edited 1 week ago) (1 children)

In most cases it's a bad idea, yes.

Also, have another look at that example code snippet though: that static variable is local to that function. It's a weird feature in c.

I've used it quite often in embedded code where a single variable was only for one function, and only for that one app/device. Wrapping it in a struct would've made the code needlessly more complex (that's a code smell). And yet, these static locals are very easy to refactor to one local to a struct. May the situation change, that's still an option.

[–] FizzyOrange@programming.dev 2 points 1 week ago (1 children)

that static variable is local to that function

Yes I know how static storage durations work. It's still global state, which is a code smell. Actually I'd go as far as to say global state is just bad practice, not just a smell. Occasionally it's the only option, and it's definitely the lazy option which I won't claim to never take!

[–] Boomkop3@reddthat.com 1 points 1 week ago (1 children)

Aaand.. you didn't even bother to google it :/

This is not about storage durations, and it's local to a function

[–] FizzyOrange@programming.dev 1 points 6 days ago (1 children)

I don't need to Google anything. I have 30 years experience writing C & C++.

This is not about storage durations

Yes it is.

https://en.cppreference.com/w/c/language/storage_duration

it’s local to a function

Only the visibility is local. The data is still global state. You can call that function from anywhere and it will use the same state. That's what global state means.

https://softwareengineering.stackexchange.com/a/314983

Some of the biggest issues with global state are that is makes testing difficult and it makes concurrent code more error-prone. Both of those are still true for locally scoped static variables.

[–] Boomkop3@reddthat.com 1 points 6 days ago* (last edited 6 days ago) (1 children)

Again, it's an easy refactor to make it not global. There are cases where that extra abstraction work simply does not add value.

With your background, you should know that

[–] FizzyOrange@programming.dev 1 points 5 days ago (1 children)

it’s an easy refactor to make it not global

I have enough experience to know that making global state non-global is usually anything but easy.

[–] Boomkop3@reddthat.com 0 points 5 days ago (1 children)

Damn, suppose I won't just pass it as a pointer from the call site. That'd be so difficult to add an int to a struct

30 years my ass

[–] FizzyOrange@programming.dev 1 points 4 days ago (1 children)

Yes, and then pass the context from the call sites of that function, and all the way up to main(). Oh look you're refactored the entire app.

That's best cases too, you'd better hope your program isn't actually a shared library running in a SystemVerilog simulator with state instantiated from separate modules via DPI, or whatever.

30 years my ass

lol when you have 30 years experience you will have actually tried to do this a few times and realised it isn't usually as trivial as you hope it would be.

[–] Boomkop3@reddthat.com 0 points 4 days ago

That's not a great approach, but you do you

[–] SwordInStone@lemmy.world 1 points 1 week ago (5 children)

inheritance rarely solves anything

load more comments (5 replies)
[–] Michal@programming.dev 3 points 1 week ago (1 children)
[–] Boomkop3@reddthat.com 3 points 1 week ago

And focussing on what not to do is not the best way to get things right

load more comments (1 replies)
[–] DrDeadCrash@programming.dev 10 points 1 week ago (2 children)

I feel like if one tried to follow all of these "rules" at all times nothing would get done, at all.

[–] BradleyUffner@lemmy.world 11 points 1 week ago

A code smell isn't supposed to be automatically bad. A smell is an indication that something might be wrong. Sometimes using a smelly pattern is legitimately the only way to do something.

[–] pcouy@lemmy.pierre-couy.fr 6 points 1 week ago* (last edited 1 week ago)

Apart from the fact that, as another commenter said, "smells" are not "rules", I think most of these points come down to developing good habits, and ultimately save a lot of time in the long run by initially spending some time thinking about maintainability and preventing/limiting technical debt accumulation.

[–] morrowind@lemmy.ml 5 points 1 week ago (1 children)

half of these will make your code better lmao

[–] SwordInStone@lemmy.world 10 points 1 week ago (4 children)
[–] bhamlin@lemmy.world 12 points 1 week ago

I'll never tell

[–] shnizmuffin@lemmy.inbutts.lol 5 points 1 week ago

My code is exclusively Complicated Regular Expressions and it's screaming fast.

[–] morrowind@lemmy.ml 4 points 1 week ago

I'm not going through every one, but null checks, vertical separation, status variables and binary operator in name, are all things that often make your code better and more readable

load more comments (1 replies)
[–] DScratch@sh.itjust.works 5 points 1 week ago (1 children)

I think to present rules like this as hard rules, with little explanation and no nuance is harmful to less experienced engineers.

A prime example here is the Duplicated Code one. Which takes an absolute approach to code duplication, even when the book that is referenced highlights the Rule of Three:

The Rule of Three
Here’s a guideline Don Roberts gave me: The first time you do something,
you just do it. The second time you do something similar, you wince at the
duplication, but you do the duplicate thing anyway. The third time you do
something similar, you refactor.
Or for those who like baseball: Three strikes, then you refactor.

I've seen more junior devs bend over backwards, make their code worse and take twice as long to adhere to some rules that are really more what you'd call guidelines than actual rules.

Sure, try to avoid code duplication, but sometimes duplicating code is better than the wrangling you'd need to do to remove it.

Making extra changes also leaves extra room for bugs to creep in. So now you need to test the place you were working, and anywhere else you touched because of the refactoring.

[–] pcouy@lemmy.pierre-couy.fr 2 points 1 week ago (1 children)

Well it's in the name, they are code smells, not hard rules.

Regarding the specific example you cited, I think that with practice it becomes gradually more natural to write reusable functions and methods on the first iteration, removing the need for later DRY-related refactorings.

PS : I love how your quote for the Rule of Three is getting syntax highlighted xD (You can use markdown quotes by starting quoted lines with > )

[–] DScratch@sh.itjust.works 2 points 1 week ago

The site doesn’t define what a code smell is, though. It’s just a list of Don’t Do’s.

That’s kind of the nuance I would be hoping for.

Something like:

Code Smells are clues that something is amiss. They are not things that always must be ‘fixed’. You as an engineer will, through experience in your own codebase and reading of others, develop a sense of the harm imparted by and the cost of fixing Code Smells. It is up to you and your team to decide what is best for your codebase and project.

(The rule of 3 formatting was intentional, given the community we’re in)

load more comments
view more: next ›