A collection of Bad Code Smells in a Catalog form for Developers & Researchers. Code Smell is a typical bad code implementation, and learning these concepts immiedietly makes you a better developer!
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”.
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.
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.
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”
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.
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;
voidmay() {
do(field);
}
int field = 3;
voidyou() {
do(field);
}
int field = 3;
voidbe() {
do(field);
}
int field = 7;
voidhappy() {
do(field);
}
rather than
int field = 1;
int field = 3;
int field = 3;
int field = 7;
voidmay() {
do(field);
}
voidyou() {
do(field);
}
voidbe() {
do(field);
}
voidhappy() {
do(field);
}
A bad example of encapsulation would be:
classAClass {
privateclassHelloThere {
int a = 1;
int b = 3;
int c = 3;
int d = 7;
voidDoStuff(AClass self) {
Do(a, b);
}
}
private HelloThere field = new();
voidWorld() {
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 othersvoidPrintCounter() {
staticint 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
At that point I would argue composition/traits are the way to go.
“This extends Draggable”. That’s great but now we can’t extend “Button” to override the click handler.
Traits:
You wanna have Health, and do Damage, but don’t want to implement InventoryItem? No problem.
You wanna be an Enemy and InventoryItem? Go for it.
What’s this function take? Anything that implements InventoryItem + Consumable
The only reason why traits are considered better is because in languages like rust it can enable static dispatch. Whereas interfaces in C#, Java, Typescript, (and C++ via abstract classes, not templates) are always dynamic dispatch.
Looking at the Rust docs, it looks like it’s not much more than a difference in implementation under the hood.
It would be clunky, but in C# you could duct tape this: make a static abstract method in an interface that takes an object named ‘self’, then an extension method that extends the class and just casts then runs the function with Unsafe.As<TFrom, TTo>(ref obj), or an explicitly aligned struct with overlapping values.
I don’t expect any such implementation anytime soon though :/
ps: Typescript can go take a hike, it’s a superset for a language that was never designed for this
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”.
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.
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
).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!Not sure what your point is here. Of course inconsistent naming is a code smell. Do you want inconsistent names?
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.
Indeed, so now it is a code smell.
Fair enough, code from a different era smells different
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”
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.
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
inheritance rarely solves anything
You gotta know how to use it properly
At that point I would argue composition/traits are the way to go.
“This extends Draggable”. That’s great but now we can’t extend “Button” to override the click handler.
Traits: You wanna have Health, and do Damage, but don’t want to implement InventoryItem? No problem. You wanna be an Enemy and InventoryItem? Go for it. What’s this function take? Anything that implements InventoryItem + Consumable
use an interface?
Yeah Interfaces would be the next best thing.
The only reason why traits are considered better is because in languages like rust it can enable static dispatch. Whereas interfaces in C#, Java, Typescript, (and C++ via abstract classes, not templates) are always dynamic dispatch.
Looking at the Rust docs, it looks like it’s not much more than a difference in implementation under the hood.
It would be clunky, but in C# you could duct tape this: make a static abstract method in an interface that takes an object named ‘self’, then an extension method that extends the class and just casts then runs the function with Unsafe.As<TFrom, TTo>(ref obj), or an explicitly aligned struct with overlapping values.
I don’t expect any such implementation anytime soon though :/
ps: Typescript can go take a hike, it’s a superset for a language that was never designed for this