C++ Code Smells - Jason Turner

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:

[jplflyer]

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.

[nitsanbh]

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.

[DarwinsChihuahua]

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.

[catlord]

5:24 " Do we wanna step through every line of this ?"

Oh, now I see why this video is an hour long

[hannahcrawford]

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".

[mikel]

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.

[szaszm_]

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.

[equesm]

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.

[fennecbesixdouze]

@17:51 Having a lambda inside your function with the same name as your function is definitely a code smell.

[Attlanttizz]

If anything, C++ has gotten more complicated with each standard version.

[shavais]

Maybe extern const lets you define a bunch of constants in one place? But why not just put them in a header? I guess if it was a large file of constants, maybe including it all over the place would increase compile time?

[matthewp]

What IDE is he using? I like the live compile + assembly view

[ColinBroderickMaths]

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.

[mina]

extern const may be used for run-time library version checking. I'm surprised he doesn't recognise that.

[ABaumstumpf]

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.

[martinlicht]

We don't have teach things all compilers warn on. Except we need to teach those who might write on a compiler one day.

[lexk]

C++ best practice - use something else if possible

[nazavode]

C++ is a minefield. Anyway, this is what we have to deal with, and this is an excellent talk.

[shavais]

Raw loops, even loops that iterate over STL containers, are a code smell!? Because there's some sort of functional style STL algorithm aggregator or whatever for every conceivable need for a loop!? It hurts my brain just saying those words! Is that all_in thing guaranteed to perform as well as a raw loop that does the same thing? You're expressing intent, but you're hiding details. It's not always clear to me whether it's worse to obfuscate intent or hide details. When I'm debugging, I need to know the details, and if I have to jump around all over the place to a bunch of tiny functions or read STL header code to get the details of exactly what is being done and how, it's going to take me a lot longer. (That's not to say I never use functional style STL stuff, I do. I just don't necessarily try over hard to use it if it's not the most obvious solution.)

[MdWahidurRahmanOvi]

I have always wondered whats the editor or IDE he is using