C++ Code Smells - Jason Turner - CppCon 2019

  Рет қаралды 77,945

CppCon

CppCon

Күн бұрын

CppCon.org
-
Discussion & Comments: / cpp
-
Presentation Slides, PDFs, Source Code and other presenter materials are available at: github.com/CppCon/CppCon2019
-
There are a lot of rules to remember for writing good C++. Which features to use? Which to avoid? The C++ Core Guidelines would be over 500 pages long if you were to try to print it! What happens if we swap this around and instead of Best Practices look at Code Smells. Coding decisions that should make you think twice and reconsider what you are doing.
We will ask:
* What are the most important code smells?
* Does it simplify the way we write code?
-
Jason Turner
Developer, Trainer, Speaker
Greater Denver Area
Host of C++Weekly / jasonturner-lefticus , Co-host of CppCast cppcast.com, Co-creator and maintainer of the embedded scripting language for C++, ChaiScript chaiscript.com, and author and curator of the forkable coding standards document cppbestpractices.com.
I'm available for contracting and onsite training.
-
Videos Filmed & Edited by Bash Films: www.BashFilms.com
*-----*
Register Now For CppCon 2022: cppcon.org/registration/
*-----*

Пікірлер: 79
@Yupppi
@Yupppi 8 ай бұрын
I love it when the presenter says "sorry" when someone talks at the same time during their presentation :D
@von_nobody
@von_nobody 4 жыл бұрын
Last slide around 56:34 if there is no values in vector you get UB because difference cant fit `int`, even more `-min_int` is UB, `unsigned int` could avoid this problem but first you need correctly convert `int` to `unsigned`. Some thing like `((unsigned)i) + INT_MAX + 1`, this will preserve relative order of positive and negative numbers.
@kyoai
@kyoai 4 жыл бұрын
10:36 , Line 12 : I think omitting "std::" is a code smell. Just by reading the code it is not clear if "all_of", "begin" and "end" is from the std:: namespace or some custom implementation, like boost. This becomes even more confusing considering in line 4 and 6 std:: namespace is not omitted, so writing std:: in line 4 and 6 but omitting it in line 12 may suggest that line 12 uses something other than std::. Unless the namespace name is unreasonably long or is an unreasonably deeply nested namespace i think namespaces shouldn't be omitted.
@Omnifarious0
@Omnifarious0 4 жыл бұрын
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.
@JohnDlugosz
@JohnDlugosz 4 жыл бұрын
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);
@Voltra_
@Voltra_ 11 ай бұрын
ADL
@davidm.johnston8994
@davidm.johnston8994 4 жыл бұрын
Man that conference was great!
@davidm.johnston8994
@davidm.johnston8994 4 жыл бұрын
I'm just starting to learn C++ and doing the research to understand this conference made me learn so many things!
@joycesmith6455
@joycesmith6455 4 жыл бұрын
15:05 Nice. The only thing remain is to have a member named total_area and an input parameter named total_area.
@rinkaenbyou4816
@rinkaenbyou4816 3 жыл бұрын
31:11 Wow, I actually had this case where I had a constexpr complex state machine but I did not make it static. It became a performance hazard.
@alexandrustefanmiron7723
@alexandrustefanmiron7723 9 ай бұрын
I am actually leaning towards imposing out argument as a rule with reference return and optional output error argument and no side effects! Only things to reason about are already in the possession of the caller.
@tomtanner2310
@tomtanner2310 4 жыл бұрын
Something that was touched on once in the talk - I find overlarge variable scopes to be a code smell all by themselves. The compiler might be able to work things out (possibly) but the reader of the code (not to mention the writer) likely won't be able to. And re-using a variable can result in some surprises.
@MrZapper1960
@MrZapper1960 2 жыл бұрын
There are great speakers at cppcon but Jason has to be the best
@sergeikrainov2512
@sergeikrainov2512 4 жыл бұрын
42:57 - what's the point of constexpr factorial? We don't know input at compile time, right?
@djFracture
@djFracture 4 жыл бұрын
It lets the compiler inline the function call
@aniliitb10
@aniliitb10 4 жыл бұрын
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.
@AM-qx3bq
@AM-qx3bq 4 жыл бұрын
@@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 :/
@budabudimir1
@budabudimir1 4 жыл бұрын
If you write factorial(5); you do know the input at the compile time.
@skilz8098
@skilz8098 4 жыл бұрын
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.
@Don-jt7ch
@Don-jt7ch 2 жыл бұрын
Would he ever regret asking for questions mid presentation.
@kontruspech
@kontruspech 10 ай бұрын
Good job!
@sinom
@sinom 11 ай бұрын
in c that would have been an undefined nhuber of parameters because there you need (void). So they were probably thinking about that
@josee.ramirez4305
@josee.ramirez4305 3 жыл бұрын
Why is 13:49 better than 13:34?, anyone understands the "smelly code" (not only C++ people) and is only a couple lines longer. I am seriously asking the reasons why this is smelly. Seems like "lack of c++ features is smeally code"
@NovemberIGSnow
@NovemberIGSnow 11 күн бұрын
The earlier example is more error prone. For one thing, there's a bug where the author accidentally reads from the wrong array/vector/wtv in the 2nd for loop, which is an out-of-bounds read waiting to happen. In general, std algorithms avoid memory safety bugs that index based for loops do not. Plus, because of the past 40 years of C/C++ where int is the default type people reach for when indexing, it makes this code unsuitable for extremely container types. Furthermore, someone pointed out that depending on the type for hose and pipe, non-const index based access can result in a write, like on C++ maps. This is more of a problem when you're using generic templates, but using an algorithm means programmers don't even have to think about that possibility. The later example reduces code duplication and also ensures type consistency. C++ will just cast data types in the background, and so using int value for your accumulator results in a silent loss of precision. Most compilers can warn about this kind of hidden cast, but it's better in a lot of cases to just use auto to avoid the casting problem at all. It is annoying and ugly to have to pass around begin/cbegin and end/cend everywhere, but it's still generally a better practice than index based for loops.
@RedOrav
@RedOrav Жыл бұрын
I really respect Jason but I wonder just how did we get to a point where a simple for loop and adding comments to your code is a code smell, but adding a std::accumulate function with a lambda is the best practice to add some numbers up. Then you get the committee adding a spaceship operator which requires adding an extra header, that is a definite code smell like the initializer_list header or requiring tuples for structured bindings. Someone's gotta love it for sure... they make arguably useful features much more complicated and inflexible than they need to be. I guess I shouldn't be surprised at this point
@chrissherlock1748
@chrissherlock1748 8 ай бұрын
I think people are excited by functional code in C++. Seems reasonable.
@MantasXVIII
@MantasXVIII Ай бұрын
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.
@zvxcvxcz
@zvxcvxcz 4 жыл бұрын
26:08 Wait? That's really undefined? I guess it is, but probably only because it is difficult for the compiler to totally rule out indirect attempts at modification.
@DPGrupa
@DPGrupa 4 жыл бұрын
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.
@kered13
@kered13 4 жыл бұрын
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.
@simonmaracine4721
@simonmaracine4721 2 ай бұрын
Modifying a const value in any way, including const_cast is undefined behavior. It's a rule somewhere in the specification.
@kered13
@kered13 4 жыл бұрын
I'm going to need a whole talk on what the problem with const member variables is, because I don't understand. I use them many times when I have variables (especially pointers or references, but other types as well) that will never be modified even if the object itself is not const.
@ryanobrady569
@ryanobrady569 3 жыл бұрын
Me as well. I think they are using the object oriented argument that setters and getters allow one to change the underlying representatation
@danadam0
@danadam0 2 жыл бұрын
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.
@SrIgort
@SrIgort 2 жыл бұрын
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.
@code.sculptor
@code.sculptor 4 жыл бұрын
Another code smell on slides 6.x: use of a macro that isn't even in the C++ standard. M_PI, although very common, isn't actually part of the standard. Worse yet, it's a macro.
@lincolnsand5127
@lincolnsand5127 4 жыл бұрын
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.
@Sh4d0wStR1k0r
@Sh4d0wStR1k0r 3 жыл бұрын
c++20 has std::numbers::pi (inside )
@tiranito2834
@tiranito2834 5 күн бұрын
@@lincolnsand5127 the only reason it's a macro is historical. In C, const variables are not real constants, they are const as in fact that the compiler promises that it will not compile code that attempts to modifies const variables, but they are still variables, which means their value is stored somewhere in memory rather than hard coded. If a const variable is not modified in C++, it can then be used as a real constant. That's why in C, you can't use a const size_t to initialize the size of an array, but in C++ you can, because the rules in each language's standard are different... that's why M_PI is a macro. Because it gets replaced in code with a real constant value that is actually guaranteed by the compiler to be known at compiletime.
@zvxcvxcz
@zvxcvxcz 4 жыл бұрын
If a step is only 3 lines long then I'm not convinced it is a step and probably shouldn't be a function. It's just parceling and hiding code in a way that makes it less readable.
@keris3920
@keris3920 4 жыл бұрын
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.
@kered13
@kered13 4 жыл бұрын
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.
@marcossidoruk8033
@marcossidoruk8033 Жыл бұрын
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.
@valshaped
@valshaped 8 ай бұрын
Yes, yes it does :P
@tiagocerqueira9459
@tiagocerqueira9459 6 ай бұрын
Cpp bros finding out functional programming immutability is good
@MarcusTheDorkus
@MarcusTheDorkus 3 жыл бұрын
The static const bit is surprising. I get that it's the only way for the compiler to make initialization safe, but I would never have looked at that line and thought it was branching and locking. Also the solution is quite ugly... and you better hope that the library you're using accepts string_view instead of a string.
@childhood1888
@childhood1888 2 жыл бұрын
12:23
@PiotrWitoldNycz
@PiotrWitoldNycz 4 жыл бұрын
Am I the only one who thinks that writing "double area(const double)" is worst than simple "double area(double)". All these const next to trivial-type params are const-over-correctness (IMHO). I do not know/heard of even single real life example when such const helped in preventing errors or increasing performance. But I know examples when writing "template T convert(const U)" needed to be simplified to version w/o const because of types like "std::uniqiue_ptr". Moreover - in C++ world "foo(const int)" and "foo(int)" are exactly the same function signatures.
@PiotrWitoldNycz
@PiotrWitoldNycz 4 жыл бұрын
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; }
@naxaes7889
@naxaes7889 4 жыл бұрын
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.
@tiagodagostini
@tiagodagostini 4 жыл бұрын
The "you have not names your code well enough if you need a comment" is a very very limited way of thinking. It just means that you, or anyone that agrees with that has never dealt with complex systems and situations. You cannot really hope people to name functions with 130 character long names to really explain what they do on very complex domains. The reason WHY you do this or not that is CRITICALLY IMPORTANT to be commented on, otherwise someone might think a real feature is a bug .
@sc-mh3jj
@sc-mh3jj 4 жыл бұрын
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
@TrueWodzu
@TrueWodzu 4 жыл бұрын
@@sc-mh3jj Function should have only one responsibility :P but even then, sometimes it is hard not to comment what this function does.
@DeckerCreek
@DeckerCreek 4 жыл бұрын
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.
@edwinontiveros8701
@edwinontiveros8701 4 жыл бұрын
​@@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.
@skilz8098
@skilz8098 4 жыл бұрын
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...
@achan1058
@achan1058 4 жыл бұрын
size_t is incredibly dangerous. A loop like for(size_t i = x.size() - 1; i >= 0; i--) is a bug, and a much more likely one that x.size() >= 2^31. One way to think about it is like this: Imagine if you put the international date line going through the middle of America (or whatever country you are from) instead of in the middle of the Pacific Ocean. How much more likely is someone going to screw up the date?
@lincolnsand5127
@lincolnsand5127 4 жыл бұрын
`size_t` is useful, but like all other unsigned types, it shouldn't be used haphazardly.
@naxaes7889
@naxaes7889 4 жыл бұрын
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.
@keris3920
@keris3920 4 жыл бұрын
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.
@sinom
@sinom 11 ай бұрын
C++20 for stuff like this has std::ssize() which returns ptrdiff_t instead which usually is int64_t
@cartesius33
@cartesius33 3 жыл бұрын
Nice talk but sometimes making the code harder to read and more obfuscated just by using "modern" C++ features is definitely not the right way.
@Yupppi
@Yupppi 8 ай бұрын
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.
@Claasic90s
@Claasic90s 3 жыл бұрын
15:08 Seriously what's the point of doing all that fancy stuff? If I were to code review this I have to do all the dereferencing and jumping back and forth in my brain to understand what's going on, it's hard to read and debug if something went wrong
@666alikat
@666alikat 3 жыл бұрын
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".
@slyfox3333
@slyfox3333 8 ай бұрын
A simple fold operation is too complex? It's pretty straightforward tbh...
@LogicEu
@LogicEu 2 жыл бұрын
I remember the good old days of C when we could write entire programs with one or two const variables in the entire code en nothing really broke. Now we get shame for even writing a simple for loop.
@tkotg93330
@tkotg93330 2 жыл бұрын
My thought exactly !
@slyfox3333
@slyfox3333 8 ай бұрын
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.
@tiranito2834
@tiranito2834 5 күн бұрын
@@slyfox3333 "super buggy" LMAO, if you can't write correct C code then that's your skill issue, don't blame the tools for your lack of skill with them LOL
@Antagon666
@Antagon666 Жыл бұрын
Cringe
CppCon 2019: Jason Turner “The Best Parts of C++"
58:36
CppCon
Рет қаралды 90 М.
Red❤️+Green💚=
00:38
ISSEI / いっせい
Рет қаралды 90 МЛН
What it feels like cleaning up after a toddler.
00:40
Daniel LaBelle
Рет қаралды 90 МЛН
Stay on your way 🛤️✨
00:34
A4
Рет қаралды 26 МЛН
WHY did this C++ code FAIL?
38:10
The Cherno
Рет қаралды 241 М.
Advanced C: The UB and optimizations that trick good programmers.
1:12:34
Eskil Steenberg
Рет қаралды 164 М.
Branchless Programming in C++ - Fedor Pikus - CppCon 2021
1:03:57
Reacting to Controversial Opinions of Software Engineers
9:18
Fireship
Рет қаралды 2 МЛН
Code Refactoring: Learn Code Smells And Level Up Your Game!
36:25
Code With Ayush
Рет қаралды 8 М.