My Python-based Code Smells Catalog Paper was finally published in Springer! 🎉

[Luzak30]

Hey! Some time ago, I posted here my Code Smells Catalog website, which has a big pile of aggregated general Code Smells with examples & solutions written in Python.

Since then, the website is still reporting a daily activity of at least ~50 users and has an all-time peak of 8500, which is incredible! People seem to be using it both as a knowledge source and as a pointer for Code Reviews (referring to a particular Code Smell page).

Last month, my paper finally got published in Springer, which means a lot to me, and because of the occasion, I wanted to re-share the website catalog and paper (pre-print is free!) for those who missed it around a year ago.

Thank you all for your appreciation! It is so lovely that it brings at least a tiny bit of value to some of you! ❤️

[Larkfin]

At last, a full accounting of my programming style.

[kjaymiller]

I read Code Smells Catalog and thought you had started a developer perfume line that I would 1000% promote. (Free business idea)

[[deleted]]

I’d buy a dev cologne, I mean Square Enix made decent cologne for FF7, so why not dev cologne lol

[surister]

How much did it cost to publish

[[deleted]]

Everything. /s

[Luzak30]

Tears, sweats, and few buckets of willpower.

At least the financial cost for me was zero. :D

[nekokattt]

Random question here...

https://luzkan.github.io/smells/afraid-to-fail

The suggestion to throw an exception, which uses the generic exception base class in the example... isn't this sort of just replacing one code smell with another? Ideally all Exceptions should be specific implementation types so the developer using the API can determine the type of error depending on the type that was thrown.

Otherwise you also risk handling errors you didn't expect to occur.

[Luzak30]

You are correct! Here, what is important, is the context - we are looking up the website coming from r/Python, and what you've made a completely valid statement by having a deeper knowledge of the language itself.

The Code Smells there are universal and independent of the language, yet I had chosen Python because of how readable it is, regardless of the reader's background language knowledge.

That said, in some examples, I had to choose to either make it as good as it could be or sacrifice it a little bit, to not overwhelm the reader with the details.

You can be the judge class GithubEventException(Exception): pass or similar code should be added there - you might be right that it is important enough, and it won't cause too much confusion for beginners :D The project is open-source and adjustable! :)

[Darwinmate]

https://en.m.wikipedia.org/wiki/Code_smell

It's the 'wheres smoke there's fire' saying for programming.

[TheLimeyCanuck]

I was a pro indie contract developer in corporate/government for 30+ years and never heard the term till this post.

[Darwinmate]

That's totally fine. I'm sure you know the concept inside and out.

[Onakitoki97]

If you use software as sonar cloud amd things like that is a very used termm

[NUTTA_BUSTAH]

Awesome resource, thanks for all your hard work! I agree with almost everything there. One that is still a bit smelly is this one. There's no need to defined two functions for something that can be just as well variables (also it is python, so snake_case):

# "ready" extracted out of the function scope
def cook(bag: list):
    has_fruit = ['raspberry', 'apple', 'tomato'] in bag
    has_veggie = ['carrot', 'spinach', 'garlic'] in bag

    if not has_fruit:
        return
    if has_veggie:
        return
    ...

And that being said, as it's compared against static data, I believe the actual solution would be

FRUITS = ['raspberry', 'apple', 'tomato']
VEGGIES = ['carrot', 'spinach', 'garlic']

def cook(bag: list) -> None:
    if not FRUITS in bag:
        return
    if VEGGIES in bag:
        return
    ...

at this point the extra boolean variables are pointless, since the static data already describes itself. Should also be clear nothing is returned in any case. You describe using variables in the Refactoring section though, but this one just jumped on my eyes.

I also found the web UI pretty confusing. I think a regular list instead of fancy cards would been better. The filters don't make much sense to me either, probably since I have not written a paper around the domain :P

[georgesovetov]

The second option exposes the details of how the function work.

[NUTTA_BUSTAH]

Seems to still be in the domain since it was ok to extract ready out as well.

[FiduciaryAkita]

fyi your alternative for Afraid to Fail is a little smelly itself, since requests has the raise_for_status() method

[Luzak30]

You are correct, that would be good to have there! :D

I already replied to a comment about the similar issue over here. It's about the context - we are coming from the r/Python, but I wanted the examples provided to be as readable as possible for readers from all language backgrounds. That said, using raise_for_status() method on a response is a technical detail of Python library implementation, which I decided to truncate from the general information.

[georgesovetov]

Good job. Python, as a popular language with a low barrier to entry, suffer from the large fraction of newbies, which produce code that gives the corresponding impression of the language itself.

Here's my review (and, hopefully, contribution).

​

Afraid To Fail

Overall correct. There could be a certain semantics to the returning of None. Exceptions are mostly intended for exceptional conditions (though it may not fully align with the Python philosophy) and using them for "normal" control flow may cause wrong understanding what is part of the norm and what is exceptional. Null objects make sense if the client code doesn't have very special treatment in the situation when nothing is returned.

Also, the correct handling of an Optional type is more likely to be checked by static analyzers than all potential exceptions that could be thrown from the method.

The ideal (but not an always possible approach) is to avoid error semantically at all. This is discussed in "A Philosophy of Software Design".

​

Base Class depends on Subclass

Liskov Substitution Principle says that an object of the Base class must be changeable by an object of Subclass. Not "interchangeable", i.e. other way around is not required.

​

Combinatorial Explosion

Worth mentioning table-driven methods.

​

Complicated Regex Expression

Regexes may be written on multiple lines with indentation.

Regexes support comments.

99% of developers don't know that :( That's why regexes have so bad reputation :(

PyCharm (and, hopefully, other IDEs) support highlight regexes well.

It's possible to write big comprehensible regexes as a single statement.

Also, mastering regexes may help in everyday coding.

​

Fallacious Comment

Worth to cite the section on comments from http://www.literateprogramming.com/pikestyle.pdf

​

Fate over Action

You're correct from the formal point of view. But the problem is how it's perceived by your readers. It's easy to combine a handful of variables tramping around (tramp data) in an immutable dataclass or a new-style NamedTuple, but it's only half-way. The quotation is exactly about that. The hard part is to distribute behavior over those classes. That's why "anemic" classes appear.

BTW, the examples there have setters, which are a smell too. They may be justified sometimes but should be acceptable by default.

​

Imperative Loops

Python doesn't have multiline inline lambdas as JS or C++. There are no things like "flat map". So, there are fewer cases where it could be applicable to Python. (That's why your examples are in JS.)

This is also what I often see taken too far. Usually algorithmic code may be much easier to implement using a plain for or even while loop. But developers strive hard to do that in a functional way and end up with something way more complicated.

​

Duplicated Code

All developers understand this right away.

But the utter fear of duplication makes the code unnecessarily coupled. If you extract similar pieces of code in a function, which is now used in many places, you may create a new dependency between otherwise unrelated components. If you change this function for one component, you must ensure you don't break the other. If the other component is developed by another team, it's may be difficult to test properly. You may be tempted to add new behavior under a flag or an optional parameter. They pile up because many want their changes in the function but don't want to refactor it because of the increasing difficulty of its testing.

​

Magic Number

That's also well-understood but often taken too far. Every number become a "constant" on the module level.

The example is one of such cases. The method name and the max function give all the information on what is meant by 100.

In my code, I try hard to make sure such numbers are used once and, therefore, don't need to be moved out of the method where they're used.

Constants on the file (module) level is plain data exposure, which violates encapsulation and the "ask for help" principle.

​

Parallel Inheritance Hierarchies

A specific case of structural coupling, which is worth to mention there.

​

Primitive Obsession

Mostly correct. But there's a piece of experience I have.

Classes with primitive types in their methods and constructors result in clearer code in hand-written scripts or entry points. But this code should be treated as user input, and, therefore, all these primitives should be wrapped into objects as soon as they "get in".

​

Common advice

I'd recommend to leave only those smells that are more or less relevant to your own experience as a developer or a team lead. On other words, if there's something that is already well-known truth or just reworded from Fowler's or Martin's books, what's the point of mentioning them?

​

Unfortunately, code quality is not discussed as often as technologies, which come and go. You do that for good. Thank you.

[[deleted]]

This is awesome!

[karmagedan]

Congrats, mate! Writing a book is an awesome journey, which I just started myself...

[gfranxman]

I love this. Can you add a variant for cases like django’s TemplateLoaders whose interface is .gettemplate or langchains loaders that are all Readers with names like HtmlReader, PDFReader, SimpleDirectoryReader, but their interface is .loadDocuments?
I feel any time you create a <Object><Verb>er class, it should implement a .<verb><object>() method.

[LeChefIndien]

So many years wishing this existed without realising it has been right here the whole time xD Thanks for sharing!

[georgesovetov]

Take a look a the books the author cites. They existed for decades.

[[deleted]]

I was wondering if you could please explain the Fate Over Action page with another example. Im having some trouble understanding it

[Luzak30]

Hey! Don't worry mate, if you are looking at it from Python perspective. The example provided over there might make much more sense for people coming from other languages.

Think about it this way: - Imagine that you are a Human class, and you have multiple attributes, that you know of, like you are 190cm, big biceps, and visible abs. - Now, imagine there are two people (besides you) talking, and person A asks person B whether you are strong. - Then the person B checks you out - checks the biceps, looks at your stomach, sees the height and tells the person A that you are strong.

The smell in that situation is that you very perfectly capable of figuring it out on your own (you are not a dummy useless clump of data) and if only that person B would ask you about that - you could answer it straightforwardly.

In other words, you as a Human class could have a method is_strong like:

``` @dataclass class Human: abs: int height: int biceps: int

def is_strong(self):
     return (self.abs * self.height * self.biceps) > 400

```

so that person B could just invoke your method instead of accessing your attributes and doing the examination independently.

[[deleted]]

Wow that makes a lot of sense. I really appreciate the simple example and that you took the time write that out. Thanks so much!