C++ Code Smells - Jason Turner - CppCon 2019 

I'm just starting to learn C++ and doing the research to understand this conference made me learn so many things!

I don't completely agree. I think a judicious 'using ::std' at the top of the function would be fine. Also, my pet peeve is unanchored namespaces. That's really going to get someone in a lot of trouble someday.

Maybe the code is meant to be portable between platforms that have those functions in std, in std::experimental, or getting from Boost. begin/end should _not_ be quallified, as it may need ADL -- this is the "std 2-step": using std::begin; x=begin(s);

ADL

It lets the compiler inline the function call

It is a good practice to mark the functions constexpr if you can. It'd be easier when you / another-guy want to reuse it later for compile time computation.

@@djFracture In that case the inline specifier would communicate intent better. constexpr says "this can be known at compile time" which isn't true here, given that the input is asked from the user :/

If you write factorial(5); you do know the input at the compile time.

You can calculate the factorial of a number N through the use of templates at compile time. Then there is nothing wrong with the intent of using constexpr. Yes, this program is acquiring input from the user via console input or some other form of input, however, with the function being written as it is, you can use it in a const compile-time expression somewhere else in the code... It makes the function reusable in different contexts.

I think people are excited by functional code in C++. Seems reasonable.

Comments almost always get in the way though. In almost every case, you can remove comments and replace them with well written, well structured code. Comments are the equivalent of using parentheses in language (this sentence is meant to highlight the similarity between comments in code and parentheses). Sure, maybe you need to every now and then but I hope we can agree that in most cases it would definitely get in the way. Why is accumulate and a lambda not something you like over raw loops? You replace comments with keywords. Accumulate....accumulates. what part of 'for(int i =0; i < array.size(); ++i)' says 'I am preparing to accumulate the following...' even comparably close to 'std::accumulate(...'? Yes, some of these suggestions are questionable but I think you could have not picked a worse example.

Me as well. I think they are using the object oriented argument that setters and getters allow one to change the underlying representatation

Technically, using such classes with vector, optional, variant, etc results in undefined behavior before C++20. In p0532r0 paper "On lounder()", Nicolai Josuttis explains how.

As far as I know the issue with const member variables is that your class loses the ability to have an default move and copy assignment operators, because the const member variables can't change so the compiler don't know what to do about them.

If modifying const variables is allowed, then what is the point in having a const to begin with? Allowing this behaviour denies the compiler the oppurtunity to optimize the code and forces developers to always second guess what values a variable is holding.

While the compiler could detect such a simple example, as a rule they just don't even try to detect modifications of const variables. So the modification does not prevent the optimization.

Modifying a const value in any way, including const_cast is undefined behavior. It's a rule somewhere in the specification.

Ehhh. That is debatable if it is bad. It really is not that bad imo. I use it sometimes. Not everything has to be in the standard. Every major compiler has M_PI, it might as well be used. And. It's not like it's that bad that it's a macro. I would prefer it be `static constexpr`, but nobody is naming a function `M_PI_Returner`, so I think we're fine.

c++20 has std::numbers::pi (inside )

Agreed, but in the context of this presentation, I feel that this was done to illustrate a point. The examples here are to show a general structure to make his points reasonable, not propose a real function you would use in production.

I agree. The real problem there was not that there were multiple steps, but that the steps were identical except for a change of variables. However as viewers that was not clear because he mistakenly used "pipes" in the second step instead of "hose". Factor the loop out into a separate function (not a lambda), and then it would be fine.

If a step is 100 lines long or 300 or whatever, as long as it is a step that makes sense as a whole then it shouldn't be broken into smaller functions. Breaking things into smaller functions ALWAYS reduces the amount of context to the code. What needs to be true in order for this call to work? What are the side effects of this? Is this function defined because it is generally useful or is it defined to be used at one particular place, and if so, why was it created then? The idea that just by simply breaking things into small functions you make intent clearer is stupid, you do the exact opposite. People are allergic to long functions for no reason, 1000 lines of code is 1000 lines of code regardless of whether you split it into multiple functions or not and if you can express it in the form of step1 -> step2 -> step3 and making all the mutable state explicit, then its better.

Compare these two functions - which one is better (link godbolt.org/z/otTj5A) template ForwardIter next_n(ForwardIter i, std::size_t n) { while (n-- > 0u) ++i; return i; } template ForwardIter const_next_n(const ForwardIter i, const std::size_t n) { ForwardIter r = i; for (std::size_t c = 0u; c < n; ++c) ++r; return r; }

They're not really the same. As source code, they show the programmer that the parameter will always be the same as the argument passed to it. This could be beneficial if the parameter is used in many calculations within the function, as it helps with readability/debuggability. The compiler might also be able to optimize larger functions with a const value, although I highly doubt it would ever happen in a real-life scenario with any noticeable speed. Since the differences between them are almost negligible, I think the main issue is the verbosity of having to write const everywhere, as most values often should be const. The better solution would probably be to change so we have to write which values are mutable.

to be fair, if a function name needs to be 130 characters long in order to be descriptive, the function probably has too many responsibilities

@@sc-mh3jj Function should have only one responsibility :P but even then, sometimes it is hard not to comment what this function does.

As someone who has written scientific code ( and translated "bad" scientist's code ) in C++ for 20 years, naming clarity is key. People from mainly mathematics backgrounds get used to MATLAB / Mathematical style naming. Single character variable names. Well known expressions can and should remain mathematical: b= A\x; in matrix libraries is useful if the audience is going to be MATLAB literate. These types of statements probably require a comment. But in general, I find my task to find the balance between things that should remain "formulas" and things that should be made more readable an ongoing challenge. When faced with the choice I go for readability. Comments should be rare and necessary.

​@@sc-mh3jj or you may need extract the core essence of the sentence, there should be no need to name a variable "variable_to_store_temporary_values", "temp" will do just fine; "array_of_persons" when you just can let the type and declaration express your intent as "Person persons[]"; "delete_all_things_inside_linked_list(T&& list)" instead of "delete_list_items(T&& list)" or even better "clear(T&& ilist)". It is all about expressing your intentions as clearly and as concise as possible. My 5 personal rules are: 1 - If a variable or function name is longer than whatever columns you set your horizontal width to (requires line breaking and wrapping before parameters or initialization) then it is a huge code-smell and calls for refactoring. 2 - Intent should be expressed at declaration, not at definition. 3 - If your function does not contain at least 1 other helper function, it's time for refactoring. 4 - If your function needs more parameters than can fit on your screen width before wrapping, then you need to warp some of them up in a class or struct. 5 - MACROS ARE (A NECCESARY) EVIL, use them sparsely. And always prefer constexpr or const whenever possible.

I'm 100% self-taught and I work with 3D Graphics Programming and other advanced fields such as Animation, A.I. Programming, etc... The one thing I have learned about the use of comments follows as: Comments should be used to express your intent or reasoning of why you choose to do something in a specific manner or way... Comments should not explain what the code is doing, the code should express that with intent. The only other time that I can think of comments being useful is for reminders such as // TODO: as this will help to remind you to go back to either finish, fix or refactor something that needs to be completed...

`size_t` is useful, but like all other unsigned types, it shouldn't be used haphazardly.

I'd disagree. I think they're just about twice as dangerous as integers. Integers cannot hold decimals, as they'll truncate, so you have to be extra cautious when dividing. Unsigned types cannot have negative values, as they'll wrap, so you have to be extra cautious when subtracting. Other than that, they pose little danger.

I completely disagree. There is nothing inherently wrong with size_t. If size_t causes a problem, you've used the incorrect variable type. There are cases (such as indexing an array/matrix) where size_t is more appropriate than a signed type. Your code smell is not due to size_t usage, but due to inappropriate type usage. The same argument could be made for floating point types, signed types, and wrappers.

C++20 for stuff like this has std::ssize() which returns ptrdiff_t instead which usually is int64_t

But given that it's C++, we probably shouldn't favour readability over removing chance for problems, and instead improve our modern ability to read modern features to be able to take advantage of it. It is not a beautiful language given the legacy of it by now, but we gotta deal with it somehow.

My thought exactly !

C is notorious for being super buggy lmao. "Nothing really broke" 😂 Probably the easiest language out of any to write buggy software in. And yea, iterators are better than old c for loops.

in my experience its mostly a matter of when later the code is updated so that your container types change, you dont need to track down each usage and update that too. One of the biggest issues ive faced is honestly unrelated, but its people adding vectors to objects or changing arrays to vectors, but not updating usages like "sizeof".

A simple fold operation is too complex? It's pretty straightforward tbh...