C++ Code Smells - Jason Turner
Vložit
- čas přidán 14. 05. 2024
- 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.
Save the date for NDC TechTown 2020 (31st of August - 3rd of September)
Check out more of our talks at:
ndctechtown.com/
www.ndcconferences.com/ - Věda a technologie
Not using STL doesn't make code smell! "How STL can improve code" should be a separate talk. And changing code that anyone can read into one that only STL experts can read is questionable in a typical business environment.
Great talk. Thank you, Jason.
Note that I didn't necessarily find the refactored code to be clearer, especially when it begins to incorporate use of lambdas or weird, non-obvious functions defined elsewhere. Two obvious for-loops may be far easier to read at a glance, for instance.
But the bulk of this talk was one big finger-wagging-at-me experience. I need to up my game.
34:18 I used “extern const” in a chess engine. I needed a huge u64[102400] pre-calculated legal moves, and I wanted to share it between files. So I declared it in a header file as “extern const” and wrote the actual data in a designated cpp file.
@17:51 Having a lambda inside your function with the same name as your function is definitely a code smell.
5:24 " Do we wanna step through every line of this ?"
Oh, now I see why this video is an hour long
const Line StepThrough(const Line line)
{
return StepThrough(StepThrough(line));
}
having a factorial calculation function that stores the factorial in a 32 bit integer? the max value it can calculate the factorial of is 12! signed or unsigned.
Int-overflow... had recently discovered a major bug with that - multiplication of 2-4 ints that each have typical values between 10 and 1000, but up to 99 999. But due to company policies and the shear size of the codebase.... well, the calculation stayed based on 32-bit signed Int but i just added 2 sanity-checks. The first is just doing the same multiplication but with floating points and then checking if the result is significantly different - if it is, redo the calc but with a subset of the requirements - if it still overflows it is a fallback to a small but fixed value.
In the whole software nobody cared to check for the range. User-input numbers are taken without any real sanitization and then stored as signed ints.
Funny if for example the expected input is say a number between 0 and 24, the code checks the input against those bounds and then sends it to the DB. Just that the user can also input 2147483653, the check says its ok as it stored it with 32 bits for the comparison, but sends the "correct" input to the next process.
And seriously, we are not running on some small embedded systems, we do not use ANY instruction-extensions like SSE (They are disabled in the compiler-flags), we are not memory-bound. There is no reason or benefit to using 32bit values for nearly everything. But it often is infuriating as most of the values can be reasoned to not exceed the 64bit limit ever and any input-value bigger than the 32bit limit should not be allowed.
const_cast ... have to use that a lot. Many classes (specially things like maps) are auto-generated and only offer const-access and no way of declaring anything mutable.
I use extern const on values that need to be used in many places and that may need to be tweaked now and then. Oftentimes, this includes UI element colors, dimensions, etc. If you put them in a .cpp instead of a .h, when you need to tweak one, you only have to recompile that one file. If it was a constexpr in the header, you would have to recompile any files that include that header directly or indirectly. I've worked on some large projects where changing one header can cause a 5-10 minute rebuild. Other than that, I see no reason to use it. I don't generally use accessors on constants like these.
Then "constexpr in the header" is important for projects that need more speed and/or less code size. I think, if the compiler knows the value inside a compilation unit it can optimize much better.
35:25 A good reason for extern const could be a constant whose value you don't want to expose in your header, so that later you are able to change the value in your library without breaking ABI. I consider it a rare but valid use case, because ABI compatibility promises are rare in the C++ world.
Same, though we usually export it (__declspec(dllexport) on MSVC and __attribute visibility on GCC). Actually, the biggest gap of interest and/or understanding I find in the C++ community is ABI across dylib/shared lib boundaries along with build times. std::vector is useless for an SDK that targets multiple compilers and standard library implementations along with vtables. We're stuck to rolling our own stuff or even using C-like stuff (ex: tables of function pointers) because of this neglect in standardization. C at least standardized ABI reasonably well.
great talk from Jason as always
Always set to W4 Warning level.
Extern is used if you need to read from memory you don't not control shared or DMA transfers done by HW or mapping system IO-Pins. You need to read the value each time to get the true value.
extern const may be used for run-time library version checking. I'm surprised he doesn't recognise that.
Also, some values are not known until link time. I've used this in some embedded applications where I have a library that needs configuration from outside, but I don't want the overhead of calling an initialization function, and I don't want to recompile the library either.
Another embedded use case similar to Joe's where there's no other option (as far as I'm aware) is so the linker can populate pointers to somewhere in flash but outside the app, so that data about the hardware can be flashed onto the micro at production time to provide runtime access to things like voltage divider resistor values, unit serial numbers etc
When he said "I could throw it but didn't want to hit somebody in an eye" it suddenly made me relax and feel at home.
I have always wondered whats the editor or IDE he is using
What IDE is he using? I like the live compile + assembly view
godbolt
11:40 Renaming the variable and refactoring into in_range() could have been done to the raw loop as well. This transformation has a "commit smell" where unrelated changes are made as a single commit.
Wow! So good examples!
I use out variables to fill in strings where some parsing may fail so that I can return a bool success/failure code. Seems very wrong to write something like if (someStringParsingFunc(some_string) == " ");
You can just return optional which will much better communicate what the function does - maybe(!) returns a string.
@@dimashynkar9855 or you could wait for c++ 2X (czcams.com/video/ARYP83yNAWk/video.html great talk)
I do too (depending if I can use optional or not though), but that's because we don't use exceptions. I think otherwise exceptions could be appropriate here to communicate parser failure
The code at 17:44 got a int value which should be double, assumes that the number of hoses ist the same like pipes because in line 12 the hose radius is multiplied with the pipes radius. Should this be like that? Strange code!
Just see in 21:33 it is fixed.
H explains that it is one of the bugs. That whole section is "tell me what bugs are in this code?"
This was very useful. Thank you!
The decomposing multi-step functions into lambdas is needlessly complex and far less readable than a simple multi-step function, when dealing with other people's code I dont wanna spend the time figuring out what "clever" solution they came up with to express intent when they could have just written a decent comment that allows me to know what the intention was and then linearly parse what's going on to find the bug, for something as simple as finding the total area of a list of objects, I dont wanna deal with the standard library if all it's doing is adding complexity for the sake of being "clever".
Yes! Give me simple code that I can read linearly from top to bottom. If the function gets a bit big, that's fine! I much rather have a big linear function then splitting out a bunch of random stuff into small "helper functions" that just make me have to context switch all the time.
@@kcarlsson89 I tend to agree in some situations; for example; I work with 3D graphics programming using OpenGL, DirectX and Vulkan. Some functions for doing specific things within the internal engine requires between 3 to a dozen steps before you can do the next thing. The only time I can see making a new function or refactoring code here is when you begin to see a pattern of code duplication. For example if you are working in Vulkan and every time you need to use a VK_Image such as a texture or some data file to apply calculations on, you can't use a VkImage handle, object or pointer directly. You need to have an Vk_ImageLayout, Vk_ImageView and you may even need Vk_Buffer objects that will store the information from the Image data before you can pass it to your rendering pipeline, shader or to be drawn to the screen. So you might have functions that can span 20 - 100 lines of code. And you will need a Command Pool and Command Buffers to record your commands and then send them over using your command pool query. So here every time you need to begin recording a command, and to stop a command again you have to create the structures of your command create info object and this code becomes redundant as it is the same thing over and over again. So here it makes sense to put these two operations into a begin and end function call. Then in the function that creates your buffers and image views, you'll end up calling begin on the command object, then setup the information for the buffers and or image view objects, then call end on that command buffer and then store that command buffer into your command pool object. Yeah Vulkan is tough and has a steep learning curve. I may not even have all of the terminology exactly correct, but there are times where you are not going to have a 1 line function for everything! There are times where a function is going to do several things before it can complete its process because it might have to calculate a dozen things first before it can get the final object it needs where one object relies on another. There will be loops and you can not avoid them, `stl` algorithms help in some places but when you are working in 3D you will have tons of double for loops and sometimes even triple for loops. If you get into 3D Procedural Terrain Generation using height maps and Perlin Noise generation, you will encounter nested double for loops. Pointers are not going away and neither is dynamic or heap memory. You are also not going to be able to make everything const because most of the data will be modified during run time! Especially when there is user interaction within a 3D movable scene with Camera Views. There is tons of linear algebra and vector calculus with tons of 3D Transformations happening at any given frame and some engines depending on the running hardware can run between 30 - 120 frames per second. So here you have to know how to manage memory, you have to know when, where and how to use threads and possibly even parallel programming. If they want everything to be a single line function call using only const objects and for everything to be a smart pointer, why don't they just use Python instead... IMHO one of the most important things to have a complete handle on in C++ is the ability to understand, know and manage the lifetime of the objects in your program. Oh and by the way I'm 100% self taught without any formal or post H.S. training or classes. Don't get me wrong; Jason Turner is a pro, and I enjoy listening to him, and learning from him as he does bring up a lot of interesting topics, how to avoid pitfalls, and what not. But C++ is such a beast of a programming language and so versatile that there is no 1 solution fits all needs.
Miss Kraya
Two things i look for during code review:
A) Can a newbie or a pro quickly understanding your code?
B) Can we quickly and painlessly fix bugs and add features?
Everything else is pure BS! I don’t care if you got Alpha Cum Laude from Carnege or MIT. People need to stop showing of “clever” skills when writing code in a Company or Business.
You can do that stuff at home on your pet projects or a coding boot camp.
@@theugobosschannel8466 My idea exactly. It's painful that I have to bring up KIS(S) to my colleagues all the time.
From my standpoint, tricky control flow is fine provided there are no side effects. Functional programming has tricky control flow and it's no problem because each unit is truly simple and capable of working on its own without dependencies to a larger picture/state/environment. It's when complex control flows mix with complex side effects that we end up with puzzle pieces we have to piece together to figure out what's going on. John Carmack:
>> The real enemy addressed by inlining is unexpected dependency and mutation of state, which functional programming solves more directly and completely. However, if you are going to make a lot of state changes, having them all happen inline does have advantages; you should be made constantly aware of the full horror of what you are doing. When it gets to be too much to take, figure out how to factor blocks out into pure functions (and don.t let them slide back into impurity!). [...] Inlining code quickly runs into conflict with modularity and OOP protections, and good judgment must be applied. The whole point of modularity is to hide details, while I am advocating increased awareness of details. Practical factors like increased multiple checkouts of source files and including more local data in the master precompiled header forcing more full rebuilds do need to be weighed. Currently I am leaning towards using heavyweight objects as the reasonable break point for combining code, and trying to reduce the use of medium sized helper objects, while making any very lightweight objects as purely functional as possible if they have to exist at all. -- John Carmack
C++ best practice - use something else if possible
In the multi-step function example, what is the advantage of using lambdas over normal free functions? I've been programming for a long time but virtually never found a scenario where I felt a lambda was advantageous. Moreover, you could legitimately call the example a single step if it didn't do the additional work of creating two brand new functions.
they're useful for chaining operations. In std:: excepted or std::optional you can use the and_then or the or_else methods to handle errors or initialised objects without using if-else statements.
If anything, C++ has gotten more complicated with each standard version.
Yes, became a weird combination on old and new syntax in the same compiler. Maybe Herb can save us with his cpp2?
What is the ide that he is using?
Not an IDE, but an online interactive compiler called Compiler Explorer (godbolt.org/). You can select whichever compiler you want and it will display ASM in real time. Great tool.
C++ is a minefield. Anyway, this is what we have to deal with, and this is an excellent talk.
We don't have teach things all compilers warn on. Except we need to teach those who might write on a compiler one day.
Brutal
12:50 There is still a raw loop, it's just been moved out of sight.
Silly talk in general. Straw-man arguments, but what is worse is that he thinks the "improved" versions of these examples are better.
@@markotikvic I think his point is that it's more readable compared to the original. The "this now reads like a sentence" comment should have hinted that.
16:08 I can see how this is probably safer in terms of int overflows and maybe even syntax issues, but damn it's not even close to readable.
For me it would be if a few renames: area -> circle_area, lhs -> total, rhs -> radius.
I had to get used to reading std algorithms, but after a few years of using them as much as possible, this is just as easy to read as the previous code for me.
Using reduce there would have been much cleaner.
What IDE is he using?
Compiler explorer. Go to godbolt.org . It's not an IDE, though.
Amazing!
24:45 I don't understand. Why does this compile if it obviously doesn't do the apparent and expected? I think this is just giving C++ a bad name.
It is explained in the talk. If you tell the compiler that i is const, it will think it is const and treat it as const. Therefore, it will return 4.
If you incorrectly use const_cast, what you do is you use the object that the compiler has been tasked to treat as const and optimize it as such, as if it were not const.
When should you use const_cast?
When the object is not actually const, but is treated as such at the moment...
It is a pretty advanced concept, but it does not give a bad name to C++ at all.
If not, it should give it a good name since it keeps you safe by reminding you of const, but it also gives you the possibility to ignore it when you know best.
@@giojo6598 OK, if I'm using some "cast" and assigning a value to something optimized into nothing, the compiler is the one who did that and it does know that my assignment won't do anything. Can't it display an error?
I get that C++ can be extremely quick & portable, however this is a perfect demonstration of why C++ is slow and costly to develop...and this talk didn't even touch upon memory allocation and deallocation.
A part of me can't understand why coding in C++ if you become assisted like in C# or something... What's the point of explicit native programming if "thinking native" is not a good thing ??
I love some of the new feats of C++ but I'm also really asking myself about the overall "trending" and mentality which spreads on the community...
If I do native it's because I MUST know which bit I use and have access to low level feats...
If I don't want to care about memory I just do C# (or something else)...
@@garryiglesias4074 I am here by curiosity because i am still not a C++ user. It's the third tentative and after few weeks with this kind of vidéo I'm very close to give up again ! As you told about mentality & community, so I will give you my feelings!
The first time I tried, it was with Qtcreator; too big for beginners and i never found any tutorials usefull; so, I decided to first learn C; I loved it, because I can do a lot of thing, I can find a lot of help and tutorials and good teachers. Then, I gave a new trial to C++, but nothing i learnt in C seems to be usefull in C++; so, I tried Python; in a very different way, i found a lot of very smart people doing very interresting thing in python; and I had a real fun to use it;
we had a very bad "dog-days" this summer so my computer fail; now, i use an old one, and i have to set up again; before doing this, i thought, let have a new try with C++ with this "empty" machine.
and so i am here. (running linux mageia7, gnome desktop, firefox).
and I wonder why should i bother me with C++ whereas i don't get what it is useful for ?
others considerations :
- i have heard that the linux kernel devellopers use C and not C++
- when i first use linux, it was with KDE plasma; I never liked it; now, i use gnome, much more user friendly; KDE is written in C++ and gnome in C
- last years, I set up a postgresql database; I feel very well with this "monster", all written in C;
so, i think there is some kind of culture built around some programming language; and i think i have a problem with the culture of C++, and the people who use it;
what is wrong with C++ people ?
i think they are professionnal, in business, making money; they don't care about teaching or develloping a community; they are not a community, they are all greedy rivals;
so, it"s a bore for a student to learn C++, everything seems to be hidden, and the only tutorials avalable are very naïve and useless.
in conclusion, if i have critical job to do, the best way is in C; if i want do some smart thing, python is very appropriate, and i think that in C++ i could only write bloated code.
@Marc-André Brun - Yeah I hear you...
I started to program circa 1989, using BASIC (Turbo Basic more precisely) for a few months, then spent a big part of my teenage teen doing Object Pascal+ASM (Borland's Turbo Pascal, Turbo Assembler, & their debugger).
At that time, hardware was RAW, but easy too... Here an simple ASM example:
mov ax ,013h
int 10h
And voila, you were in graphic mode "MCGA" 320x200, you had video memory address at 0xa0000, just 64000 bytes which are an index to a hardware palette RGB666...
Learning curve ? Pfff nothing, you saw it once, you were a kid obsessed with technology, the next afternoon you were playing tracing lines, circles, and play with color... And 30 years after, I still remember how to do it without looking any reference.
Of course you couldn't do much, but you learned a lot, had a lot of fun, and a lot of reward with a huge sense of accomplishment...
Nowadays, with the "high level" hardware, drivers and API, although you can easily make a textured cube spinning with fancy effects... With no pain and no performance risk...
...although... "easily"... Easily not really in fact: you have to "eat" a LOT of tools and layers of API and concepts and finally high level maths, because the simple one which can help to learn is already done by the hardware/drivers/system libraries... So in fact it asks a LOT of prerequisite knowledge and abstractions... Abstractions which are often so far from the hardware that you CAN use it without understanding, but at the same time IF you don't understand the whole stack, unless you want to do a "Hello World", performances have a huge change to be hit hardly...
Well I'm starting to write a bible, and and just even scratch the "learning problem" surface. There are a lot of things which are slipping towards difficulty to learn and to progress... How complexity is handled, nicely hidden or on the contrary falsely hidden and brings to more complexity... And all the human traits which goes together, the "group effect", the sectarism, "us and them", the business constraint (it must work, you can't "waste" time to learn on a personal level you MUST be efficient for the corporations, etc).
So it's difficult, we live in an imperfect world. I love some things of C++, historically C++ has been a MAJOR step. But I'm not really convinced by the path it takes. Maybe I'm just too "old" to understand some things.
BTW, I'm still programming everyday in C, C++, C#, and more occasionally Haskell, bit of LISP-like scripts and things. I don't believe in an universal language, I believe in "DSL" (Domain specific languages), and I think C++ tries to hard to be an "universal language"...
And on this Topic, I completely agree with Sussman & Abelson ( Structure and Interpretation of Computer Programs )... If you don't know this book, I highly recommend it, and its associated lecture video (freely available)... The videos are VERY old (early 80's), BUT the concepts a relevant, and Abelson & Sussman are great teachers. I enjoyed watching the whole lecture a couple of time. My DSL "instinct" was confirmed by their expose.
@@garryiglesias4074 Thanks for this answer!
I think I'm older than you; during the 80's, I was pascal devellopper on Modcomp 16 bits machine, with 2 Mbits of ram and 67 Mbits of hard drive; we ran a server on the french network 'minitel'; we can serve up to 30 clients, all written in pascal.
Then I resigned to take care of my parents and the familly farm.
And I had some pc running windows that i didn't use and a Borland turbo pascal 6.0 running on DOS, with nice and usefull libraries of object like Tstream,Tcollection, Tview, and Tapplication.
The "culture of pascal", is the best one I have known;
since I use linux, the friend who help me, told me to forget pascal and so, I put this constraint, and I never install Lazarus and free pascal, available in one clic in the "mageia control center MCC";( the app store of this distribution).
so, let's talk about pascal;
do you still use it or not and why ?
as old user of computer and pascal, your opinion on that will help me finding my way.
happy to read you again!
What about allocation/deallocation? Modern C++ guidelines actually doesn't have any direct memory management and any code written in post C++11 should not have any new/delete, other than very specialized user defined classes which should not have resource management in their direct API. The issue is that C++ is still taught as if it is still 2003 and people like jimiscott still think C++ is about manual memory management. The only reason to use 'new' in modern code is frameworks with old codebase like Qt or UE, but even there pointers are special and are more like C#/Java "pointers" and do not easily leak. Whenever I hear someone saying that C++ is hard because of memory management I understand that they have zero knowledge of C++ and its basic patters.
It's not even hard to write modern C++ code as it has quite a lot syntactic sugar added in later standards and stdlib covers a lot of features that previously were in form of 3rd party libraries. One issue is tooling and building: C++ requires a much better understanding on building process because it is much more complicated and errors are commonly hard to understand. Another issue is that C++ will remain an expert friendly language, despite all its recent additions it is not a language that one should start with.
What a lethargic talk. Best to watch before bed.
CZcams has a handy feature where you can speed up playback. I watched this at 1.5x and it was quite comfortable. Click the "gear" icon and adjust "Playback speed."
I watch most programming talks this way. Takes a 1hr talk down to 40 minutes or less, esp. if you skip the overly lengthy intros.
It's like listening to Ben Shapiro's conversation partner
Slightly disagree with the not using "out arguments stance" while not to use in all the cases, it allows better design.
I envy the move semantic, functional programming I very rarely do professionally :-)
@@higaski Do not plan to convince anyone it is always a matter of taste. But basically a basic case I can think of is where a meth/func returns a status success and takes the return value as argument rather than returning it. But as said does not apply all the time, I never do.
@@higaski that s a valid point but can depend on circumstances eg code historic and so on
@@higaski from a readability perspecive, I personally think that something along the lines of:
HitInfo OutHit;
if(Collision(OutHit)){
//do stuff with OutHit.
}
is far nicer than something like:
CollisionInfo Col = Collision();
if(Col.Success){
// do stuff with Col.HitInfo
}
I think there is a good argument to be made for out parameters.
Miss Kraya what you describe should be solved by syntax.
Logically, “Out” parameters communicate that the caller *owns* the resource being accessed (semantically the same as passing a mutable reference in Rust). I’d argue that any other use, such as simulating a value return, is misguided
“Out” params are useful for apis where you want to reuse memory (eg filling up the same byte buffer instead of allocating a new one every call).
Otherwise, the value should be returned, multiple values returned in a tuple, struct, or optional/result types for “no value” and error values respectively.
The syntactic verbosity, while a practical concern, has no bearing on the correctness of the approach. I remain open to counter examples.
Some Rust code for perspective:
if let Some(hit_info) = OnCollision() {
// process collision
}
OnCollision returns an Option which encodes that there may not be any collision.
We use nice syntax to branch on the option value, instead of using a Boolean return to hackily simulate an option.
42:45 why does he despise the std::endl on line 14? Sending
to std::cout isn’t sufficient to guarantee that the stream is flushed correctly (although in this case the program terminates shortly after so it won’t matter).
I guess excessively using std::endl is slower than normal. I think the only time you should use it is if something else is using the ostream in between cout's
@@puppergump4117 A better question is what genius hid a "flush" inside std::endl. Imagine how many unsuspecting programs have been slowed down due to this horrible interface blunder!
@@weekendwarrior3420 The world will finally realize the printf is superior.
@@weekendwarrior3420we already have a character denoting the end of a line. It is '
' if you want to guarantee that the line you just sent to cout is really in stdout, instead of the buffer, you use endl. It is as simple as that. No one is forcing programmers to use endl, we have two options for two different situations.
@@narutopoint Then call it std::flush, not endl. Im' not saying we can't do what's needed, just the name makes it easy for less experienced to make a mistake, hence, "interface blunder". Interface shouldn't set traps.
Heed -Wall and save yourself.
There is no such thiing as.perfect code, the could can be only good enough
at 17:19, it would have been better to add all of the values for all of the radius, then calculate (value^2)*M_PI.. That way you are only using one math operation.. btw, the other problem with this code is that it really should have been a double float value for precision for the result within the current code because with larger amounts of doors and hoses, the code would end up becoming exponentially less accurate due to the lack of precision.
(a + b + c + d)^2 is not equal to a^2 + b^2 + c^2 + d^2
@@edwardfanboy You can still shorten the calculation, though, as (a^2*pi+b^2*pi+c^2*pi+d^2*pi) == (a^2+b^2+c^2+d^2)*pi. That reduces 2*n to n+1 multiplications. That one really bugged me, too.
Some please help with this.
A function called eta( ) which takes a distance in miles and a speed in MPH and returns a string that displays the hours and minutes until arrival. So a function call to eta(80,55) should return the string "1 Hour 27 Minutes".
Think about it, it's VERY EASY...
"Clock maths" are teach at 6 or 7 year old...
rather the smell of lack of understanding of problem to be solved, as always
Good god, if you think the factorial example is somehow better than the original code you need a brain scan.
why is it always the case that the worst talks are the ones that beg for or try to force audience engagement? 😂 “interrupt me as much as possible” = “my talk is not engaging so you do the talking”. This talk is a engineering smell
Because this is not a talk, is an interactive session. If you did minimal effort and googled his name you would find out that he is a trainer and this style of engaging with audience is more of a professional thing. Most of his "talks" are in this format, except the ones where he has some specific topic to cover.
couldn't even make it 10 minutes -- horrible talk
he doesn't seem prepared at all
and stop asking questions -- just tell us
It picks up midway through and has a good summary at the end. But I agree, it was very slow to pick up and if I wasn't already familiar with the speaker and knew he had a really solid point he was going towards it would have been frustrating for the first 15 minutes or so.
Waste of your time. There are much more useful videos about C and C++.
Why?
It is a video of the presentation, which is oriented towards interactive audience.
It is very long. Time spent more on interaction with audience than focusing on the topic.