WHY did this C++ code FAIL?

📚 CHAPTERS
0:00 - Hello
1:29 - ALWAYS stack allocate if you can
4:20 - The cat is back
4:36 - API design considerations
7:27 - return 0 in the main function
8:31 - Organization and code conventions
11:22 - Variable intialization
14:44 - Deep class hierarchies
18:12 - Managing states
20:58 - defines
25:32 - Avoid copying causing unnessary heap allocations
26:38 - More on #define
28:50 - Consistent code style
30:33 - Compilation warnings
31:13 - Logging and release builds
32:30 - struct vs class
33:16 - Cleaner code
35:21 - Final thoughts and conclusion
36:48 - Use std::weak_ptr

💰 Links to stuff I use:

This video is sponsored by Brilliant.

[TheCherno]

Anything else I missed? Why do you think this code failed? 👇

[QuerijnHeijmans]

One reason I could think of why they'd have a fail is that due to the inconsistent coding style, it could be that it's plagiarized.

[ruadeil_zabelin]

I got 25 years of c++ experience. I've seen far far worse things in production code than what is being shown here. If this was a school project, i'd say good job. For a job application though... I also probably wouldn't hire unless we were specifically looking for a junior to train further.

[Debrugger]

You didn't touch on the weird shared_ptr use, which is the first thing that stood out to me and would really make me question someone's experience. There is one Game object, which is created when the program starts and destroyed when it ends. Game owns all of its members, so they all have the same lifetime (which is to say the life of the program) and there is no reason for shared_ptr. It's an atomically reference counted type that should only be used when you don't know until runtime how long an object will live, so no one can be responsible for owning and destroying it.
This is not even C++ stuff, it's about thinking fundamentally of what objects your program creates and uses, who owns what and for how long. Especially in a memory unsafe language, not thinking (or worse, knowing) about this stuff from the get go is bound to create a buggy pile of crap, memory corruption and leaks.

[ChrisBNisbet]

The defines needed brackets
E.g. not
#define SOMETHING 10 + 20
do this instead
#define SOMETHING (10 + 20)
avoids bugs like this...
int x = SOMETHING * 10;
Just one more reason to avoid defines in the first place..

[aniketbisht]

26:40 Instead of using #define you should use constexpr for defining constants known at compile time. That's exactly what it is meant for. It's also type-safe and you can actually "refer" to it as it is an object instead of it being copy-pasted wherever you write that name.

[Beatsbasteln]

I can't believe you skipped over how it made no sense to use shared_ptr on any of those objects and how tricky it is to handle ownership with it

[danzabarr]

I lost 3*5=15 marks on a coding assignment for not including 'return 0' in all five of the questions. I read the spec, and I knew about the closing curly brace being special. I arranged a meeting with the professor and later cancelled it thinking I was being petty. Then I watch this video. It feels like the world is full of people who think they know better.

[ferinzz]

"It would take entirely too long to go over every little thing"
Death by 1000 cuts. That might be what the person reviewing the code was thinking. Every step is something not quite right.

[TheTrienco]

The inconsistent naming (UpperCamel, lowerCamel, snake_case) within a few lines automatically make me think "this was copy/pasted from different sources".

Some details like not using the initializer list, the pointless unique_ptr, passing vector by value etc. make me think "someone is coming from Java".

[parvagrawal]

More of these please! I just realized how fun watching a C++ code reveiw is. Using initializer list to prevent constructing twice was quite neat.

[dongiannisiliadis]

I'd love to see a series about software architectures

[-rya]

Another issue that wasn't raised with the code is that the GameStateBase class doesn't have a virtual destructor nor does the InputObserver so destroying those polymorphically will lead to object slicing. Which is fine if you don't try to destroy them polymorphically but if you do then that is a new issue.

[makebreakrepeat]

tbh, these days when I see code with a hodge podge of styles, techniques and just weird decisions, I suspect gpt generated code and SO snippets glued together.

[jcciubs]

7:27 return 0 is actually necessary because main is defined as SDL_main. that's why you also need int argc and char* argv[]

[-rya]

12:40 std::move(this) is a code smell; Its effectively a pointer copy but very clearly not what the intention was by the writer.

[gamerk]

Speaking as a professional SW Engineer of 15 years:

One thing I ran into at my workplace: I was always *very* good at understanding how to architect code in order to achieve the desired result, but where I struggled (as I'm sure most new SW Engineers do) is knowing "best practices" when it comes to implementation. I was lucky to sit next to a *very* good engineer who was more then happy to talk shop, and I still rely on to this day to help with things (like GUI design best practices) I haven't personally done previously. But without someone like that, there's really no mechanism for people to learn what not to do, especially "if the code works".

[muB]

It took me years to finally found out how costy heap allocations are, and how C++ compilers handle the "new" keyword. My college professor who taught the first two C++ courses didn't even know about it. This should be one of the first things to mention while introducing the class concept.

[throwaway]

Deep inheritance with virtual functions is not just a stylistic choice against object oriented programming, it's also a layer of indirection for every layer. Indirection costs add up.

edit: As stated below, this is incorrect.

[aniketbisht]

26:15 Instead of const reference of std::vector I would suggest using std::span for this if you have access to C++20. std::span is basically a pointer-size pair and therefore efficient to copy. You can even provide a size known at compile time, that way it will only store the pointer. By using std::span, you can pass any contiguous range like std::vector, std::array, std::initializer_list or your own heap or stack allocated array.