Anything else I missed? Why do you think this code failed? 👇 Don’t forget you can try everything Brilliant has to offer-free-for a full 30 days, visit brilliant.org/TheCherno . You’ll also get 20% off an annual premium subscription.
12:10 they're not storing the seconds (deltaTime) in last_frame_time, they're storing the value they got from SDL_GetTicks, which probably is milliseconds since system boot or something. Or program start? E.g. Uint32 newTicks = SDL_GetTicks; float deltaTime = (newTicks - oldTicks) / 1000.0f; this.oldTicks = newTicks; I'm more used to the 64bit 100ns/tick from e.g. QueryPerformanceCounter myself, 32 bit 1000ms/tick should be good for 48 days.
You didn't seem to touch too much on the shared pointers. I'll put this up forth: it has been a while since I've watched you, I don't know if you prefer using them or not. But as a rule of thumb, use unique pointers wherever possible. There are very few cases when a shared pointer is needed. Maybe I'm missing something and they are required in this case, but I doubt it.
The problem with last_frame_time being int is not that currentTicks is float, it's that currentTicks is Uint32. They are used to calculate the deltaTime, which is a float, but that's a different variable. Storing an unsigned int to a signed is worse than missing the fractional bits.
I'm fascinated by the triple std::move(this) in the Game constructor! What was he thinking there? Missing virtual destructors in the abstract base classes, not using references at all and so much more...
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.
@@sournois90 Hiring a junior is much less expensive than hiring a senior engineer, so I could see companies looking for juniors which might not be as expensive but when trained can become a valuable asset to their team
@@CodeStructureTalk Git history? If I copy the code off of ChatGPT or somewhere else and commit it myself, version control is going to do literal 0 favor for detecting this? Idk man
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..
defines are almost never needed in C++ unless you are doing strange things like setting compiler pragmas, its highly unusual to need to use the C pre-processor in C++.
@@monad_tcp one other situation I might think of, is defining or prototyping something differently depending on another parameter only know at compile time. For example, for debugging purpose some macro could be defined, but with the content of the macro stripped off in release mode. It's not unusual and I don't think there's another way to do that except using #ifdef everywhere in the middle of your code.
While your specific example is bad, we do actually see a mix of that used and not used in the defines. e.g FRAME_TARGET_TIME (1000/WINDOW_FPS) but TILE_SIDE WINDOW_HEIGHT/15
While you're right in C, in C++ you'd just static const these as variables. Notwithstanding the fact that just throwing these all into global space like that is an issue all on it's own. There are plenty of good reasons to use defines, but you'd never use defines for variables in C++, with some exceptions if you're really deep into metaprogramming.
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.
it also seems like they pass around several shared_ptrs as arguments to functions, even though those functions don't change their ownership. they should be passing raw pointers/references, since incrementing the atomic counter (stored on the heap behind two levels of indirection) each time a function is called would incur massive overhead
I'm only a hobbyist, and I rarely actually write C++ and that stuck out to me. In my mind when you see a bunch of shared_ptr in one place, that's a sign to look really carefully at what's going on because either someone doesn't understand shared_ptr, or they're doing something really weird (this qualifies as "something really weird").
@@atijohn8135 I guess these were first unique_ptrs and he wanted to pass them as function params and it didn't compile because unique_ptr can't be copied. So he changes it to shared_ptr and then it compiles. I did that too in the beginning, it took a bit of time before the penny dropped on how this is supposed to be done.
This is 1000x what I feel. The entire thing is everything is managed through the Game class and executed through the Game class. It simply doesn't make any sense. The main shared_ptr will be there through the entire life of the program (since it quits when Game class is closing) so there's no reason to do so. Passing raw ptrs would be far more valuable in this case (and all of the cases where the shared_ptr are used seems to be used when these objects are basically guaranteed to be in lifetime). It feels amateurish (or copied from multiple different sources which is fine but some effort needs to be put in to make it homogenized and fit into your general programming styles).
Some people use shared pointers for all kind of pointers They said why I write raw pointers and take care of deleting them rather than make it shared ptr and if I pass it any where I'm not afraid from suddenly crash and don't waste my time debug all if this shit All book I read in graphics Even hazel engine use shared ptr excessively without thinking I learned c++ from this guy and I learned how to spread shared ptra every where from him (Yan)
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.
And because it's a variable (constant, but still) it means you benefit from syntax highlights in the IDE, warning you right then and there that you're using a float to something that accepts an integer (therefore truncating the decimal part). Whereas with a macro you'll only get that warning _after_ compiling it. But IDEs are probably smarter now... They should catch that even with macros. The idea is that macros are just dumb copy-pastes with no regard for C++ syntax rules. You often want to avoid that. It's considered a code smell.
@@zdspider6778 For me it's not just a code smell but, "Please step into the 21st century and leave macros behind". Of course, C programmers don't have any other choice but use macros everywhere even for "generic" programming. Large C codebase, like the Linux kernel, are full of macro hacks and compiler intrinsics for things which could trivially be implemented in C++.
@@ferdynandkiepski5026 You can use C++ templates along with if constexpr to do so and where you can't, you can leverage your build system to conditionally include or exclude certain files depending on the platform. There is a Cppcon talk explaining that technique. I think this approach is more scalable than using pre-processors.
@@1337dingus i totally understand the idea of it. the author didn't know about initializer lists so they used smart pointers to delay initialization into the constructor, and all those classes need to know about the parent, which is why they are being given this and they used move because they confused it with constructors that have && arguments in them
I suppose unique_ptrs are a little bit harder to use than shared_ptr, so as a beginner they might have been put off by hard to understand compiler errors.
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".
The inconsistent case, although ugly, is considered normal in C-style languages, like using snake case for variable names, and using camelCase for function names is very common in PHP and C++.
@@ДавидПлеван Different naming for different things, yes. That makes sense and helps to know what you're looking at. Three different styles for member variables within a few lines.. not so much.
As a senior with juniors in the team, I can tell you that some devs just commit code with inconsistent spacing, tabbing, variable naming and case, etc. It's not necessarily copy paste, just lack of care. Which I'd be half inclined to reject, mind you, because to me it's a capital offense. Just kidding, but not really. I guess my reasoning is, if you can't be assed to fix your format or check for consistency, then I wonder if you won't just strive to think as little as possible as well when making your logic.
@@Titere05 Same boat. I think "not giving a damn" is worse and more frustrating than "not knowing better". And as you say, if someone demonstrates no effort or attention to detail with the trivial stuff, how much effort went into properly reasoning about the code itself? Even worse, when it extends to the API. When even the customer facing part of software looks like it was cobbled together by a bunch of barely literate monkeys, you are actively scaring away your customers. If it looks that bad on the (supposedly polished) outside, just how bad does it have to be on the inside?
@@tendanium you shouldn't even have "child" objects messing with the "parent" object. (By child\parent I mean they contain one another). Because what if you created a "parent" object and then moved it somewhere else. Now you need to modify the pointer to the "parent" object in every "child" as well. In this case you should've separated each game state and created a "third-party" entity, that would monitor the current game state. Something more declarative, state-machine like.
"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.
@@argon7624 thats the point. this game wouldnt stress anything either so nit picking technical details is equally pointless. in many cases good enough is exactly that. good enough. nit picking the design/inputs/mechanics would be a better place to focus.
@@danielroden9424 He covered that point in the video multiple times, and followed it up by rebutting that this was sent to employers, and saying that the quality of the code sent to employers reflects, at least to them, the effort, care, consistency, and experience that you possess as a programmer
@@danielroden9424 no because the point of stardew valley is to be a good game, the point of this game is to show off it's creators programming agility so he will get hired. It fails at it's purpose.
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.
Even if you don't it is not fine to leave the *option* to do so. Make the GameStateBase/InputObserver have a public virtual dtor if you intend to allow deleting through a pointer to base or a protected non-virtual dtor if you don't want to allow it.
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.
It's still best practice to be explicit, because you don't know who will later need to read the code you write, such as professors. But, seems to me you shouldn't have lost the points, because your code was technically correct. As a side note, with 30+ years of C and near 30 years of C++, I had forgotten about the implicit return 0.
@@nelnotheamoeba9221 I think this could have been the case with my professor too, he has been programming since the early eighties, maybe even late seventies, and the feature was added in C99. But why not look it up/raise the topic in feedback instead of just marking me down? I think he had either not heard of it in all the 25 years, or he's just set in his ways. Shame, he was a lovely guy in lectures. Maybe he thinks punishments work better the more you apply them? One thing's for sure, if you want a student to really dislike you - the best thing to do is try to control them. I will be leaving my return zeros off from now on. Job done.
I mean I don't work with c++ primarily but I do work with OOP design and have done some c++ in my job. Experience wise, I'd consider myself intermediate. Just because you can do something doesn't mean you should. If you ask me, putting the 'return 0;' there is generally better. It shows clear intent. The value of clear intent is making your code easier to read. Implicit code in general is a messy grey zone. When code is relying on implicit details, to read and fully understand it requires knowing about the implicit details. Implicit code inherently makes code less readable. Sometimes this is fine. I mean in browser-side web development, everyone know the local and session storage objects are on the window. It's fine to just write localStorage.mything instead of window.localStorage.mything. When it comes to the main function. It's defined with a return type, and that type is integer. When you exclude that, it's a function that is supposed to return an integer. But it doesn't return anything. What's going on? Oh it's because the spec says that main functions specifically will implicitly return 0 if nothing is returned. One key thing difference is that excluding 'window.' in the local storage example doesn't introduce any other form of weirdness. Whereas excluding 'return 0;' does (function with return type doesn't return anything). With that said, I don't think you should have been penalized multiple times for the same minor detail. While I agree that the assignment shouldn't be awarded 100%, the mistake is not so big to be more than a -1 point. In fact, I think I'd judge it at -0.5 points once, with a note of the value of being explicit... Maybe inviting you to discuss it further with me.
I just like to be consistent. I end up moving code between languages frequently and relying on weird features that make C++ feel more like C is not ideal.
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.
I feel like AI typically makes quite neat code that doesn't really make sense when you look deeper. Similar to how AI writes grammatically correct sentences that still have nonsense content. Though I've only tested with pretty small snippets, so maybe AI has messier code for larger projects.
@@vibaj16 I've made use of GPTs for code generation and, I'd disagree with OP. When you see inconsistency i much more expect it to be human written and looked up online. GPTs are generally pretty consistent since essentially glorified autocommplete
@@vibaj16ehh chat gpt is pretty good for these types of closed-ended tests. Like surprisingly good. If the style is super consistent, and above all, it has a ton of comments for basically every line of code, then I think that’s much more telling of AI.
@@vibaj16although with newer stuff it tends to go awry, like concepts. Especially Gemini, Gemini really sucks with recent modern C++, way more than chat gpt
He also says how it's a special function so it doesnt need to return an int. It absolutely does need to return and if you dont put a return statement at the end, the compiler will add return 0; in all branches of code that you havent put a return statement
@@josipcuric8767 what he meant is you do not need to explicitly return, it will return implicitly. Since C++11, if the main function ends without encountering a return statement, it is implicitly assumed to return 0. This is a feature intended to simplify the code and reduce boilerplate.
@@josipcuric8767 When using SDL, the CRT main function (that gets implicit return 0) is SDL_main and not main, so your main function isn’t special (other than that it gets called by SDL_main) but rather an ordinary function.
If I understand it right, SDL hijacks the "main" function of your program and calls it as a regular function Because of the framework, it looks like you are implementing the C++ main function but you're not. So you should not rely on special behaviour of the main function. (Not a SDL user tho so I'm not sure)
33:11 - 100%. I think this is both correct, exactly why style guides exist and why they should be used by folks even for hobby projects. Style guides serve as a framework through which you can apply consistency to the your architecture and code, that allows your intent to be significantly more readable. Style guides like the C++ Core Guidelines distill decades of experience into easy-to-follow principles that can serve as a launching board for deeper exploration into why they suggest certain things, and force you to justify why you might disagree with their approach.
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.
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".
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.
And the costs are not just in performance, but also in cognitive load. It's not so bad in such a tiny example, but it adds up in large projects. It's always better to avoid premature abstraction.
@farukylrdrm8498 referring to something (code/data) through a stored pointer. In this case, it's a virtual function call, because the code is implicitly accessed through a pointer
Wtf no it does not add up, you always have only one vtable inside of an object no matter how many layers of inheritance you have, therefore it's always exactly one pointer lookup for a function call
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.
@@NoNameAtAll2 In most cases, you're right, as most modern devices have more than enough resources unless you have bad coding habits. But sometimes one may have to control granular details in assemblies or may encounter significant memory limitations. Stack must be allocated during compile time and libraries that provide smart memory managements have both temporal and spatial overheads (and still use heaps anyway). Manual memory management is useful in these scenarios. For once I have worked on a project where most standard C++ libraries weren't available.
You must have had pretty bad professor. When I did C++ course during my CS studies 20 years ago, the professor made sure that I used const references instead of copied arguments and I allocated practically everything from stack. I would have failed that course with a code that looked like the one in this video.
@@MikkoRantalainen Personally I wouldn't blame him, at least for now. He did well explaining both the concepts and implementations of OOP and DS. And once these are in the picture, one essentially comes across memory blocks that should live between calls. That's where stacks cannot fulfill the purpose and heap allocations must be used. Though it would've been better if he mentioned that variables that would not live outside the function should stay on the stack and passed as a reference or a stack pointer in the code.
29:20 Using clang-format will eliminate most styling/formatting inconsistencies in C++ code and should be used in every project however big or small. Just define your formatting preferences in a .clang-format file and clang-format will do the rest.
I am finding it very hard to produce a clang-format that will format things the way I like things, it's not easy to work with, ages ago I used to use a VS Macro program I got off Codeproject when that was the big thing, it was far easier to get results I was happy with, also the older beautifier programs like AStyle seemed easier to use.
@@peternimmo74 I used to program in CLion few years ago. I preferred the way it does formatting. So when I decided to migrate to Neovim I just exported their C++ formatting style to .clangformat (there is an option to do that). Since then I have made minor changes to it. Given the importance of this tool, I think, it's worthwhile to invest some time in understanding how it works and reading its documentation. If you are familiar with C++ syntax, their configuration options just makes sense (although they are quite wordy).
I'm very triggered by the shared_ptr use. It would the THE reason for me thinking someone doesn't really understand memory ownership or memory at all. Add up the unneeded heap alloc in main, and that would be enough for me to say... hey, maybe not hire this person as a senior. As a junior, sure.
Whether #define's or something else, it's actually quite helpful to have all settings in a single file. Games and a lot of other apps often end up having a configuration file that is loaded at startup, but a single source file is a pretty common way of doing that when it's not worth the trouble to parse a config file.
In case anyone wonders "Why be so pedantic?", well ... once code is over six months old or so, it will never be removed except when there are explicit issues. If someone gets poor code checked into your project and it manages to bake for awhile, you basically get to keep it forever. Even good code decays, which means at some point you end up running as hard as you can just to stay in place. Having high standards at checkin is a way to maximize how long your project will be viable before you get bogged down and have to rewrite everything (or have a competitor replace you). [It's possible that none of this really matters for most projects, but usually people aren't asking why they can't get a coding job at an auto dealership, they are asking why they can't get a coding job at a FAANG. If you know good code hygeine, you can selectively decide to forgo it in the name of speed, but if you don't know good code hygeine, it'll take you a year or two to break your bad habits.]
@@msherif428 in general it's fine to copy-paste snippets to save time, if you understand how they work and why they might be written a certain way. However if you more like 'I have no idea what i'm doing, let me just copy-paste something I found on stack overflow to see if it solves my problem', then I would say you'd be better off spending time understanding your problem better and then exploring common ways to solve it.
The advice I give every junior is the one that was given to me. If someone asks you why you did what you did at any point in your code, you better have an answer. It can be a wrong answer, that's OK, but at least have a reason. And that really forces you to investigate what the hell you're doing
The parts that are weird and disjointed are the boilerplate bits to setup windows and interface with SDL and whatnot which frankly everybody does the first few times on a new toolkit. it's super likely that they were cribbed from various sources so the coder could concentrate on the game bits.
Actually, there is another reason why you should use member initializer lists and not assigning to variables in constructor body, namely const. You can initialize const member, but you cannot assign to it.
A (non-static) const member is almost always a bug. Having a const member means your object can not be assigned to or moved from, meaning it cant be swapped. These are fundamental behaviors required for performant use of containers and algorithms. If you want a member to not change, make it private. static const is fine and recommended best practice for C++
@@ulfben Interesting. Is it true always, or only for default move constructors and assignment operators? Also, do you have an example off the top of your head for when you would use const member?
Regarding the defines: I had a professor who really wanted defines. In Arduino code he made a variable named pin13 (for the led), then up above he defined OUTPUTPIN13LED 13; . He felt that pin13 wasn't a descriptive enough name (or even ledpin). This video isn't my code, just saying he might have a professor (or whatever) that wanted things that way.
I remember a rust lib where pins in output mode and input mode have different types, which makes sense. Should be possible in c++ to some extent though harder without ability to redeclare variables with the same type
17:30 this looks like an incorrectly implemented "interface" in C++. After Java and C# people liked interfaces and wanted them also in C++, looking as interfaces appeared after C++, people implemented them this way. Ngl they look pretty janky. To be correct, it should have defined virtual destructor and the methods should have been pure virtual methods
14:00 Actually, no. It has a huge bearing. Initializer lists allow to optimize or inline out the constructor call ( which isn't really a function, btw ), which in turn may exclude some optimizations, which has an impact on performance in tight loop situations. It is always preferred to use the initializer list instead of initializing the fields in the constructor body.
@@zeez7777 What is or isn't a function is a little blurry concept - but in case of constructors the 'function' part is its body. Which, if it is empty, the compiler will totally omit and call , in the same manner, constructors of the members without creating stack frames. It is _technically_ a function, but it is so special that it is best to consider it a separate entity with its own rules. Which is also why it is best to use initalizer lists as much as possible. On a hot path of a program this might yield massive performance gains. In one example of code I was working on I gained as much as 50% speedup from merely switching from explicitly assigning fields in constructor body to using initializer lists.
@@TryboBike Nice, i'm pretty sure this happens with any function though, its just certain compiler optimizations being "allowed" by the nature the code is laid out. I dont really agree on that functions are a blurry concept. Looking at my own examples with different levels (or no) optimization in IDA makes it pretty clear.
16:52, what you are talking about here is that the that code is violating the O in SOLID, the open closed principle. The code should be closed for modification, open for extension. The amount of events growing means you may have to keep adding functions to that class to handle different extensions, so it is not closed to mofication, neither is an open to extension, because you cannot extend the amount of events it can handle without modifying this class itself.
Actually, SDL requires 'return 0;' because it wraps the main function in a macro. Not sure of the technical details, but without return 0, your game will throw an exception on exit. Another way the #defines could've been handled is with constexpr. constexpr would have kept the data const as well as potentially inlining the data.
At my uni, our algorithms class was taught in C by an old school C guy who taught us to use #defines all the time. I assume there's something similar going on at a lot of uni's, and that's probably where people pick up the habit from.
I'm not an expert on this (I don't use C++ in my job), but I read a lot of embedded code as a hobbyist, it's really common to have define, and is probably true too for the Linux kernel IN C, I feel like it's just C++ convention that people dont do it? I see it really frequently in C code but not C++, even people coding in JavaScript (games, global const) Java (Android source, public static final) does that I think, just don't know much about modern C++ : /
You can use defines for things that wont change (pi, degrees to radians conversion, stuff like that) but when you make a video game prototype there are obviously tons of stuff you want to tweak. So you can at least declare static variables you can tune in the debugger.
I think one issue is that some C compilers won't make statically sized arrays, even if your variables are const. You *have* to use literals. whereas C++ has constexpr
12:18 he didn't lose anything here. I guess a better name for this variable would be "lastFrameTicks", but all information is preserved. Well until it overflows, cause he stores unsigned int in int
@@Raspredval1337 they use SDL ticks to compute delta time. And that delta time is not converted to int in the end. It is used in update function and that's it. The thing that gets stored is the original ticks that we got from SDL
It never gets "converted back to an int". It uses the exact value from SDL_GetTicks(). The real problem is that we store deltaTime as a float and lose the exact precision.
@@Raspredval1337 They're not converting it to a float though, yes, the Uint32 is being used for the calculation of a float, but that doesn't mean that the underlying variable is a float after that. last_frame_time is being assigned to currentTicks. currentTicks is Uint32, last_frame_time an int. It's just the unsigned to signed conversion
@@Raspredval1337 they don't convert it back. They create a new float variable based off of the int, but keep the int as itself, never modifying it (unless I missed that part of the video)
I'm the most-senior C++ engineer you'll find: I was publishing articles in magazines about it in 1989, which comes to 35 years of being proficient. I don't use m_. I think it's wrong to mark variables' scopes as part of their name. The code in the body of a function will use names that are local, parameters, members, and globals. It _does not matter_ where the variable lives when calculating with it. In fact, as code evolves over time, the variable may move: what started out as global may become a member, to allow different instances to have their own values; it may move from a constant (defined locally or globally) to become a parameter to the function. Why should that affect the code? Modern editors track the home of a variable for you, by color-coding it and showing you where it's defined.
@@0netom I don't know about m_, but just '"_name" used to be pretty common. It's a relic of the past though, when dev tools weren't as mature as they are today. The idea being that when reading the implementation(usually in .cpp) you don't have to jump back to the header file to check the scope of a variable, if it was private the "_" would make it visible at a glance
I'll be honest, and this is probably not normal, I love when an application logs random stuff. It's like a peek past the curtain without having to actually peek past the curtain, and, if used with discretion, doesn't impact much. The Legend of Zelda: Tears of the Kingdom, for example, logs an autopsy report of every actor that reaches 0 HP. That's fun and cool!
I think the biggest compliment I got in college was my engines professor saying, "Thank God you write clean code". I often overthink before writing code nowadays because I get so worried about it being unreadable to someone else, or unorthodox, or confusing to the point where I have to spend time reunderstanding my own code. So hearing that boosted my confidence a bit when sharing code with others. I'd imagine though, as a college professor, he's seen really, really bad code, so even mediocre code could seem amazing.
I suspect the prospective employer has asked for some code he has written previously to get a feel for his abilities rather than the application asking for code to be written.
To me, this looks like one of the Applications for some gamedev. In the past I applied for a bunch of these Jobs and you have a Week to do this... for every Application. I wrote 8 test back in the day until I said F***k it, I go somewhere where they do not waste my time this excessively. It is often expected of you to have experience of shipping something, but you just got out of University - how should you know this? Me, now working outside the Game Industry, can say no one expects a Junior to be able to know this - well except the Games Industry... Applying for a Junior Position in the Games Industries is a miserable experience.
instead of settings being just const, I would set them as constexpr, which would make them fixed at compile time. They are not changed anywhere anyways, and with that could be used in constexpr functions to improve code performance.
Then you probably missed the part where the macros were likely created precisely so the author could tweak the harcoded values. If they're variables (not even const) you can then adjust the values at runtime, either through dev-only input handler, or a debugging session. This would be faster to iterate than a recompilation followed by relaunch of the game.
@@dkosmari You can tweak constexpr values the same way you can tweak #define values but you dont clutter your code with defines. I get why it would be beneficial to have them as normal variables for tweaking, just saying that if you want to have a constant, it should probably be a constexpr
Bet the iostream was there for some print debugging, the fact that include was left in is enough a hint of this. Ideally of course we never would use generic print debugging, bet everyone has including the reviewer :D
I would say the reviewer who failed the code was wrong. Overall the project seems quite decent and C++ like. The biggest issue I saw was probably vectors passed by value, and too many shared_pointers for state, and some pointers to this. But that's not a big flaw. Because explaining pass by ref vs pass by value is something that would be easy to explain to a person who built project of this size. And pointers knowledge I consider a bonus, because if a person can create a working project with pointers that does not crash or leak, then it already shows a deeper level of understanding about the language. Looking over the code it looks like it has been done over some time, there are some mixed styles, but that's exactly what you would expect from a person who's learning and improving. You start from basics, you learn new methods, but you don't always go back to rewrite everything. The commit history also shows a decent code evolution. Addressing the chapters: - The shared_ptr and includes : Looks like this is an evolution from SDL_CreateWindow in commit history, which is an ok approach. Good that it was promoted to shared_ptr. And who knows, after a while, maybe some other threads will need Game object, so shared_ptr is not that awful. Includes, I don't think people clean up includes that much in real projects. - The main returning 0 : That's a good thing. Even if there is a quirk in C++ standard, you don't want to rely on other developers knowing all quirks. - The private/public blocks, _m : Good practice, and looking at the project, I think it would be easy for the developer to adapt to such coding guidelines. - The last_frame_time is an int timestamp, and the code is correct. - Initializer lists : Ok, but you would use last_frame_time{}, or C++ modules in modern code anyway. I would avoid assignment. - structs vs classes : I would say class is preferred when it contains other classes. Use structs for POD types only. Calling memset on a struct that contains classes is a bad thing, and memset use is common for structs. Struct with classes will also cause problems on module boundaries. - Class hierarchies : Reviewer should know that you can implement same logic in multiple ways. This shows the person understands some OOP, and it's a plus not a minus. - States could be more elegant, can be done simpler, but it's ok for a small project. - Defines : Ok, partially bad, because if they are not wrapped correctly you can get unexpected results. If macro expands to a sum, but is multiplied by a value in code, you'll get wrong results. But the code is based on SDL, and if you look at SDL examples, they are macro heavy. Also, macros do full compile time expansions and avoid dereferencing or operations at runtime. Purely from gamedev perspective that's fine. It doesn't matter nowadays from speed perspective, constexpr or consts would be better, but it's not completely terrible if the code is SDL like. And in the context of gamedev where you want more frames per second it's better than pure virtual config classes for example. - Code layout : Just use an auto format of the IDE. It takes some time to adapt to a coding style. It's really not a big deal. I mix brackets for a while when switching languages. - Event handlers and shared_ptr : Not necessarily, you can have a handler that's just a static function that will persist through the whole application lifetime, raw pointer would be better then. And if even handling is stopped before engine shuts down, raw pointers are just fine. No need to add extra overhead, there could be a lot of events that are emitted, why add refcount overhead? So overall, I think it's a decent project that : works, shows knowledge of pointers, shows knowledge of OOP, shows code organization skills, decent Git workflow, and ability to integrate with systems like SDL. Failing this project without a constructive feedback is a very unpolite move, sadly it is what industry defaults to these days. Project is quite cool for someone who's not a senior developer.
I think I'd have dismissed it as it looks like it was written by AI or at best extensive copy & paste and they'd not understood it enough to clean it up. It's like obvious lies on your CV "I invented the colour purple" - no you didn't, Next!
Seems like people online are always concerned with scale, so they introduce a bunch of abstraction just in case you want to add 100 of something, even though right now there are only 3. That not only slightly obfuscates what is going on for this simple case, it also is slower due to the overhead of the abstraction. I get that this project is meant to showcase what the dev is capable of, but less abstraction can also show an understanding of what I just described
Well in this case the lack of abstraction was quite, well weak. If you're having an input observer class, I'd like to see everything there and with maybe 2 events one for position inputs and one for key inputs. Either way it would be an improvement as it would allow for dynamic key assignment which would be a big plus to see. I'll add that this isn't a far off case of "scalability" anyways. In this case, fundamentally there isn't a ton wrong (since the code works and does what it's supposed to) but there's no point in the code where there's not a "questionable" decision. If you have 20 other candidates and you at best fall in the middle of the pack, you'll likely be rejected when they're hiring for one position. This is the ultimate point though.
29:20 I worked with a team of maybe 60 devs on a pretty big codebase (half a million lines of C++, almost as much Perl 🤮) who standardized on same line { except for the beginning of a function block, I’ve seen it in a few places too. It doesn’t bother me at all, I think the reason for this failing code review is almost certainly due to not passing the vector by const reference; this is super basic stuff and if input objects aren’t being changed they should always be passed by const reference. Also the “Running” as a public field instead of a function *which should also be marked const*. I think just the lack of “const” in general. Good review, you’ve earned a sub :)
In terms of variable naming conventions, such as m_Something for private member variables, k_Something for constants or pSomething for pointers, i.e. hungarian notation, I think best practice is to just follow the most commonly used style in the domain / programming environment. e.g. when you’re writing a C++ library, use snake_case() method names, but when you’re writing an Unreal plugin, use their weird naming convention, with F and A prefixes. Personally for my own C++ codebase I have developed a specific style that uses PascalCase for class names, and no prefixes for any variable names, because any IDE should be able to show the type and access modifier of a variable. But consistency is key. I’ve worked on a codebase where there were multiple competing standards, and that is not ideal.
I respect conventions and of course everyone can write code how they want. Having said that, there is no reason at all to use any kind of prefix, m_ archives NOTHING it's ugly and unnecessary. If I see someone write beautiful code and they use m_ k_ whatever, I don't care. If I see a junior dev write medium code but they still use m_ then that's a big red flag for me, especially if they can't give a reason for it. Blindly flowing "bad" advice is just dangerous, and critical thinking is extremely important in programming
@@onebacon_ I agree with m_ being ugly, but let's say hypothetically you're working on the Unity engine, and internally m_ is used everywhere, then it's important to stick to that, as inconsistency is worse than m_
@@onebacon_ I think m_ or at least _ or something else is good to distinguish members from local variables. If C++ required using this it wouldn't matter.
the m_ (or the trailing underscore, which I favor due to it not being so much of an eyesore) is not so much about knowing it's a member, but about giving you the freedom to define a getter later without the redundant noise word "get". It pulls complexity inwards which is always a good thing.
@@isodoubIet funny you mention that, I do exactly the same thing in my c++ codebase, e.g. a public vertexBuffer() method that returns the private variable vertexBuffer_ (with const-correctness and references vs. pointers and everything of course)
The naming convensions of m_ is in my opinion just useless clutter that makes code unreadable. It made sense a long, long time ago when we didn't have proper access specifiers and such. But it has been obsolete for some decades.
The feeling I got from the code is that the author does not really understand how things like memory, ownership, and lifetimes work. Passing stuff by value? Using shared pointers everywhere, even where the game object should be owning the (I presume) one single game state? Vectors of raw pointers? While all this may work for this small, single-threaded application, such an approach will become a major headache down the line. Regarding the style, that is really good advice. I often see new programmers scoffing at following best practices when it comes to code style and formatting. But to any professional, that is the first thing that they see, and the first impression they get. Spend an hour making your style acceptable and consistent, if only not to trigger the reviewer needlessly. Regarding the formatting, if you ever want to share your project with others, especially for interviews and such, just set up an auto-formatter, like clang-format. Takes a minute to download and a couple minutes to tweak the config file to your liking, and practically all IDEs support it.
Honestly, if this was for a junior job application and it's not like FAANG or whatever... seems a bit harsh to reject it. I think the major red flag, as you pointed out, was the heap allocation everywhere which could indicate that this is a person that doesn't understand memory management. I would at least have a on-site follow up interview after this to be honest :)
Exactly. Seems like most comments expect perfectly formatted code without any issues. The classic 5 years of C++23 experience for junior position. All flaws in the code can be fixed in like 1h after a code review with a senior developer, which is part of their job. But apparently that's not how we roll these days. Project looks like a lot of effort, and GitHub history looks quite logical.
@@CodeStructureTalk Depends. If it's a highly competitive position (and there are 20 other applications which is the case at most large studios), some applications would be rejected outright. For Game Dev positions (which I'm assuming this was for), having a personal github with good projects has a lot less importance compared to a solid resume. At the studio I work at, we don't really look at github projects (unless they get to the shortlist in the first place). This isn't a reason to reject someone, but if you have 5 other people who write better code and/or have a better resume that matches what you're looking for, you can easily get passed over without ever receiving an interview. It's unfortunate, but it is what it is.
@@wakannnai1 Ok, fine. But the issue with this is that the interviews are completely random. There might be resume filtering that drops the resumes that miss some keyword, I mean, maybe you're missing 'pthreads', then the resume can be dropped because the particular reviewer does not like two column layout and prefers tags at the top. Another reviewer might drop the resume because there was a mix of camel case and snake case. Another might drop because you used templates instead of pure virtual classes. Another will drop because events are not abstracted. Another because shared_ptr is used instead of unique_ptr. The reality is, the process is completely random. And doing assignments is mostly an unpaid waste of time. The person who submitted the code was left wondering what is wrong, and possibly shaping his view about the industry as a whole. While the popular comments regarding rejection are: The naming conventions and brackets are wrong; He should have had a senior developer level of knowledge regarding constexpr! SDL uses macros, but your macros are bad, and similar cosmetic issues. Number of actually relevant notes in the comment section is not that large. In the end, it is a game you cannot win. Might as well be a random number generator selection. No useful feedback, no one actually looks at the code evolution and how person has achieved a goal. Complete waste of effort.
I guess maybe there were enough better interviewees to fill all the available positions. I find it shitty that they didn't provide feedback, though, forcing this person to ask a TH-camr for a code review. Telling someone this is not good enough without telling them why is just wrong.
The allman/k&r curly brace style shift makes sense in this case because 1) function scopes cannot be nested and 2) having it on newline makes grepping for functions easier (but in this case the return type would have to be on a newline as well)
A lot of this seems style related. Some just inefficiency. My big issue is the triple std::move(this). Structurally this is wrong because this is declaring “this” be invalidated but then it’s used on the next line (twice) which is bad. It doesn’t crash because move won’t invalidate a pointer and is effectively a copy. It makes me think he was doing something different than what he thought or meant to do.
Move doesn't invalidate, the value must be valid past being moved, and in fact all stdlib types work with the explicit rule that moves leave the moved-from object in a "valid but unspecified state" -- it's not a problem in itself to continue using the moved-from value, but you cannot assume anything of it's state post-move. It's a logic error, but not undefined behavior.
One thing i guess i can contribute finally is about the defines. at least when you write bare metal C/C++ you dont really want to put more load onto the little crappy microprocessor than you need to. So having the preprocessor insert loads of things is preferred. compared to filling up the tiny little memory on this little thing with constant values that never change.
11:54 that is actually correct, it's supposed to be an int (ok, technically it should have been an "Uint32") because that's what SDL_GetTicks() returns, it's the number of milliseconds elapsed since SDL was initialized, no loss of precision there. And it's perfectly fine to then calculate deltaTime as a float, since deltaTime usually something you'll use to interpolate stuff later, so you want that variable to be a floating point.
8:56 I don't think you have mentioned naming conventions like this in the C++ series--would you be able to do a video on the ones that would stand out to people in the industry? I don't work in the industry, but I don't want to develop idiosyncratic coding habits that makes my code look weird to potential collaborators.
There's more code style conventions than stars in the galaxy, so there's little point in focusing on any one specifically. What's important (especially for an interview like here) is consistency, not only for aesthetics and "clean"-ness, but mostly to show that you know why conventions exist. Seeing a mess of mixed naming styles, declarations everywhere, random member visibility and mixing all types of member initializiation for no reason immediately says "this person has never worked on anything bigger than 1000 lines and has no idea how to manage growing complexity if they can't even get use a consistent style for trivial stuff like visibilities"
In the C# programming language convention is to use lower camel case names for the private fields, and generally also starting with an underscore, but that might differ per project. The eye hurting old school `m` is almost never used in front of the underscore. Which is also very unneeded in my opinion. And public fields and properties (of which the latter are a concept not known in C++ but similar to getters and setters but then with usage like it where a variable) are used written in Upper camel case. But I also like the naming conventions of Rust a lot, which are super consistent, like many incredibly great things in the language. Go does it different again, and uses Upper vs lower case to automatically make functions public vs private. Which I kinda like for it's simplicity.
The only thing that reasonably matters is consistency. Most people agree on common conventions like local variables being camelCase or snake_case, while types and structs are PascalCase... but not even the C++ standard library follows these consistently. The most important thing is that your own style is consistent and has clear intent, unlike the code in this video which was obviously copy-pasted from various sources.
Older languages like c++, c, etc, often do not have a single standard for how to write things. So there the most important thing is consistency (as people here have said). Newer languages like C#, rust, Java, etc. often do have an explicit standard as part of the language. In that case you should follow that standard. You shouldn't write a rust program in camel case for example.
one thing I note early on, is inconsistent case usage, it starts with Pascal case, then snake case and then goes into camel case. That's kind of messy. I'd definitely give a negative point for inconsistency.
I can say as someone who constantly switches between languages with different conventions, I sometimes find myself using both snake case and camel case.. and curl braces on the same line and a new line (really only applies to c++ with that one which I don't really use).. and it irks me when I do it and dont catch it
28:58 i love the rant here! Since I control the CI build processes, I put in code beautifiers that immediately fail PR builds if the source doesn't exactly match the beautification. It's an automatic pre-inspection for me because the juniors need to have their builds run green before I even start my review. Similar with a lot of other things you found: add a linter gauntlet to the build process so everyone needs clean code from the automation checks before the human code reviews start.
That was a lot of hanging/aligned braces inconsistency and defines. I've never used or probably even wrote that many defines before. Not even when I use Unreal's debug helpers as macros in a separate header until shipping
I love these videos @TheCherno . Very instructive. Also would like some videos where you go deeper into a topic using actual code like this as a way to show how to better approach a problem.
About public private members. When designing class, you expect your teammates to use that class. If some member variable is public, it gives programmer a hint that they should modify that variable outside the class. I work on unity game and I am going nuts when somebody initializes member variable as public ClassName VariableName only to find out they just wanted to assign it in unity's inspector. It always makes me question, should I assign that variable somewhere? If I do not is it a null? It just takes extra time to figure it out.
Same line braces is generally recommended in JavaScript, due to the fact the language doesn't force you to use a terminator at the end of lines, e.g. ; so something like: return { property: value; } Will return undefined instead of the object you might have expected.
The configuration system of Grbl CNC is based on # define macros, and this ended up being adopted by Marlin Firmware for 3D printers. These codebases depend on meta-programming with liberal use of macros, including some ugly recursive macros. The main challenge for these projects is to be able to target multiple CPUs, from 8-bit AVR to 32-bit ARM (plus a simulator on Linux/macOS/Windows), producing the smallest and most optimal binary for the enabled features. It uses the PlatformIO build system (based on SCONS) which provides hooks that allow for simplified configuration, but at some level these codebases still need to radically optimize for size, and # defines do the job. These codebases would be interesting cases to see whether new and better approaches could be applied to this kind of problem.
I'm in two minds about the defines thou, yes they are not real variables but sometimes, they don't need to be at all! They just serve has a easy of life for typing repeating stuff, they also make it easy to turn on and off peaces of your code and also make compilation way faster (by not being compiled at all...). Has a c++ coder I don't think you should lean to much on C defines, when a constexp or even a older const type could be used but I also don't think c++ programmers should be afraid or hate defines, in the right hands defines are very powerful. Just my opinion.
For DEFINES that are meant to be used locally in the same file: Consider making them variables and sticking them in an anonymous namespace in that file. Plus, consider leaving them capitalized for readability reasons. The reader will know that they are configurable constants at the top of the file.
At around 8:15, I think there's a strong reason to return zero, because the function definition itself specifes that it returns an int. If you're not doing it here, then there's the possibility you're also forgetting to do it in other functions. Compilers /can/ let you get away with this if you haven't set the compiler to be pedantic enough, and it can lead to some strange bugs that are VERY difficult to track down. I've made this mistake before and it resulted in an entirely unrelated variable not updating in the code. What was even worse was the fact it only happened in the compiler I was using with a particular microcontroller. The same section of buggy code in an online compiler ran fine!
2:45 This might be nitpicky, but you absolutely don't need to free or delete a raw pointer just before the end of the main function. That memory will be freed anyways on termination
I have had to stop caring about which style of curly braces I like better. I use both JavaScript (well actually TypeScript) and C# and the standards for both are different. C# wants curly braces on a new line, JS/TS wants it on the same line so I regularly use both.
@@zeez7777 Do you use a new line for if statement curlies as well? I’m still at a point where I prefer the curly at the end. I feel a new line curly adds unnecessary white space, idk, it feels as if the function body is detached from its head. It feels even worse for if statements. If you don’t use a new line curly for ifs then your symmetry argument becomes invalid. If you do, then I fear your code will be hard to read, at least for me.
@@CoderDBF Yeah i use it everywhere including if's. For me it becomes a lot easier to read because of the "free" line you get, otherwise consider an if -> else if -> else with curly on the same line: it feels needlessly compact and squished, making it arguably harder to read. Of course there are limits but spacing things out is a universal way of making this look better, in code in user interfaces, pretty much everywhere.
@@zeez7777 I’m going to give it a try this week. Perhaps after a few days I will change my mind. You have compelling arguments, it might be a matter of experience on my end.
What do you think about a namespace of constexpr instead of the defines? Of course I prefer the struct as owned by Game, but if you're going to make it global anyways, why not a namespace?
For things that are truly constant, this is the way, imo. Drop project-scoped constants can be in a named namespace, and file-scoped constants into an anonymous namespace to keep linkage internal to the compilation unit.
I also prefer namespaces because classes are supposed to have instances. I see no reason for instance of config to exist (barring Json reading). If no instance necessary, might as well omit the class.
33:59 you're focusing on the code-style, but not on the basic performance reason in that: defining function inside the class definition will tell compiler that this is inline function. Best choice here is to keep balance. You should put these funcs in CPP and explicitly call them inline.
What's important is that should be picked even by compiler warnings. So it's double fail - not doing dtr and not reading compiler output. I swear to god people need to program with Werror for some time to learn manners
One thing that struck me was I didn't notice a sleep(1/fps) statement somewhere in the update() method, it just while'd away the cpu cycles by polling if frame target time has passed or not. If you while() loop something, it'll just hog all the cpu or cpu core cycles. I'd use a timer to execute the update() method, with the 1/fps as interval. Most desktop operating systems are an event driven system after all, not a microcontroller where it's more common to run something in an endless while loop, though even then you'd put the device to sleep with an interrupt or watchdog timer. It would draw unnecessary amounts of power and potentially slow other processes. That could be a major factor for a fail, especially if the application was geared towards game design. Also, all the string literals could be placed in language files so you can translate them into other languages, then use defines for the indexes so it's clear what text goes where. Settings could be an ini file. Using defines for settings is the same as hardcoding literals in your application and per definition not a 'setting' that could change. Yeah I know you could redefine them at compile time but that's just another level of ugly imo.
The mismatched case style and {}s ... reeks of copy-pasting would be my first thought, definitively lacks attention to details like you mentioned. Interesting discussion about #defines, I personally don't have an issue with it, but I'm programming micro-controllers so every little byte count, I'm more picky about variable types myself. Not a fan of having public class variables,. Great Series, I'm learning a lot; Thank you!
Re #define memory usage, wouldn't it be the other way around? Say I have `#define MY_STRING "hello, world!"` and I use that in multiple places in my program. Since the preprocessor essentially does string replacement, doesn't that mean that after preprocessing, the program will have multiple occurences of the same string literal, which will mean the executable becomes larger? and the executable obviously has to be loaded into memory at runtime. Whereas with a const struct, there's only one copy of the string, and references to it. Am I totally off base here?
i have never been a fan of the `m_` coding stuff it means "member" but then `this.`, `this->`, `obj.`, `obj->` means member already, and then `()` means method no brackets means variable, i have no idea why `this->m_var` would be more clear than `this->var`. i do on the other hand see the use cases in ppl doing stuff like `this->sptr_Variable`, `this->uptr_Variable`, `this->ptr_Variable` to tell everyone this is a shared, unique or raw pointer from the name.
Hey Cherno, any chance you can upload the stream vods to youtube of you recreating your old game using raylib please? And possibly even the vod of you adding the web platform for it as I'd love to watch the longer versions of these to help with learning.
That iostream include breezed through too fast. I don't know if it's still the case but back in gcc-4.x including iostream (which could happen through unwanted exceptions) allocated around 100kB of static objects to the binary. Just by including the header with no other interaction. Terrible for embedded.
C++11 gave us in-class member initializers and this is the first time I've ever seen them in the wild... and I've seen a lot of C++ code from a lot of places. I didn't think it was valid C++ at first, that's how rarely they come up. I had to check the spec, thinking it was something new I missed in C++23.
One of OOP principles - encapsulation. By making a variable private you can decide how it can be interacted in code. For example your class has two numeric variables that always should stay proportional, or they cannot be equal to 0, you can implement that in setter method. Or you need something to happen after value is retrieved, through getter. That's about it
@@Username-bin Yeah and I totally get that, but then you see countless examples where the getters and setters do nothing but get and set the variable without any validation/transformation/etc..
@@pk10x When you have an invariant (a set of conditions that make an object valid, otherwise it's invalid) you use a setter. Ideally, your classes either have invariants for everything, or for nothing (so we call the class a "struct"). In the real world, you might end up with some member of the class needing invariants (so they need a setter), others don't. So, for the sake of code consistency, you put a setter on all the members. Just having a setter that does nothing but changing the value of the member variable, that's an anti-pattern. And it provides no encapsulation either, as every private member ends up being exposed anyway.
I usually use them like this because it is easier later, if needed, to: - Add logs on set/get - Put a breakpoint to see who changes the value at a specific point - Add whatever and not break ABI, for example, mutexes, if I decide it should be thread safe
37:29 the main usecase of a std::weak_ptr is to break cycles of shared_ptrs owning each other, there would be no advantage of using them here. Non-owning raw pointer parameters are perfectly fine if the function doesn't care about the lifetime of the object, but in this case a reference to an InputObserver might be the best solution, so if the caller has a shared_ptr they would only need to use the dereference operator on it when passing it to the function. This way the function wouldn't impose on the caller how the lifetime of the object should be managed, and it would avoid the overhead of using the smart pointer's copy constructor as well as any unwanted copying of the data. Dangling references are easy to avoid with some discipline, and they can be detected by clang-tidy anyway. The C++ Core Guidelines suggest the same thing if I remember correctly.
I am not *at all* an experienced C++ developer, so this comment is more for my benefit (to check my understanding against that of the more experienced commenters in here). The first thought that struck me as I saw the first bits of the code was that it seems almost like "I know this is for an application so I want to use as many fancy things, or things I consider to be fancy, as possible. So I'll make loads of 'special' pointers." It feels to me like they started off with ideas of which special things they wanted to add instead of just getting the game to work well. That's actually something I wonder about with experienced coders and optimisation: do you typically start off trying to optimise a lot of stuff or at first just try to get the concept to work and then start optimising and refactoring where you can? I imagine it's probably a bit of both, where the more experienced you are, the more optimised your first version of the code already probably is. Right?
Spoken like a true game dev: "you've gotta do this" while moving the bracket on the next line "or this if you're weird" while putting it back on the same line 🤣 It's a fact of life that we, in game dev, universally prefer Allman indenting style (bracket on next line) while open-source developers love the Linux Kernel indenting style with the bracket on same line. Some in open source even like horrors like this: if (cond) { // code } else { // more code } I'm not starting on the horrors which have zig-zag indentation.
5:45 I think this is the minimum level of pedantic that any C++ code should be. If you don't want to think about this kind of details, you shouldn't be using C++ at all.
'heap costs more to access than stack'? Allocation takes longer but access is the same, and a single allocation over the lifetime of the code costs 0. The dereference is moved outside the loop by all modern compilers as well.
Pretty sure stack is more likely to be in the cache in which case access would absolutely be faster. And I don't believe that's the only reason stack access is faster, it also can be optimized out easier, in some optimized cases the stack object can be completely removed by the compiler even in O1. (which won't happen to heap allocations unless the compiler can see through what you're doing which with heap it almost never does)
Heap objects have an additional layer of indirection, since you must first read the address off a stack variable before you know where to read from, while stack objects are a static offset from the stack base pointer. On repeat accesses this is often moot since the address is likely already loaded into a register, but that's far from guaranteed.
@@cookieshade197 Passing a heap or stack allocated object into another function is an indirection no matter how they were created. Within the function that created them it's going to be stored in a register address unless it's hideously complicated with enormous register pressure. Try it for yourself on compiler explorer.
@@Spartan322 cache is going to be 64 bytes wide no matter what part of memory is being cached. I'm not sure if you're talking RVO (allocate on the caller's stack instead of yours) or something else with the second half, but stack allocated objects don't guarantee no heap, their initialization may well invoke heap allocators as well.
@@danmerillat The stack is always gonna be quicker to access regardless, aside from indirection, the stack is closer to the execution in memory. And I never said its guaranteed, but in most cases the stack is in the cache.
I had a lot of issues regarding this review. Suggesting to use map, allocating the GAME class on the stack, complaining about defines when they are completely valid. Saying there is no reason to have a vector of pointers? There are some reasons, especially for callbacks for objects managed by another entity. creating some data structure to manage each one of those will be wasteful and meaningless instead use raw pointers. But I had more issues with the actual code.
I am an interviewer and I would've probably failed this code (except if applying for a junior position) because I would think that the writer does not understand stack allocation, type system, encapsulation or ownership. All of which I usually demand from senior C++ programmers.
@@uuu12343that's the impression I got, anyway. Anyone asks me to make them a complete program for me to be considered for a position, I'll just laugh and look somewhere else for work. If this was for a job interview, it clearly demonstrated the interviewer didn't value this person's time. For a university exam, failing it seems needlessly harsh unless the task description explicitly asks for something this code didn't achieve.
His code is fine, only thing that could make it better is if he dropped OOP. All of chernos criticism is just superficial, and your expectations for what makes a programmer a senior one are skewed ( yes knowing about stack allocation is important to know) but just because the author of the code put it on the heap, it doesn't mean he doesn't understand the stack, also the way someone is naming variables doesn't matter at all and cherno said it himself, so why focus on that at all.
Some criticism is valid, while others are not. Doing the make shared in the constructor initializer is detrimental to readability. Also, it is supposed that shared_ptr default constructor is lightweight.
Well, it doesnt have to be detrimental to readability. Normally, when I end up with long constructor initializer lists, I stack the member variables veritically and align them.
Having the application object (game) on the stack is not a good advice. That object lifetime is as long as the application itself, so there will be a reserved amount of stack for the whole application execution. There is no contract on that object size from the consumer perspective (main function in this case), so if the application object size grows (due to refactoring or extension of Game class), there is a risk of introducing a stack overflow. One way of avoiding heap allocation in this particular case is to use static, but unique_ptr is actually cleaner. In the general case of local variables, the stack should be used when the size of the variable is known to be small to medium size, otherwise the heap is the better choice.
You could argue about every single type then, "it could grow the stack too much." Do you put your `std::string` objects into smart pointers too? Because these too, have no guarantee of size, now that everyone uses SSO. It's not like the author of the main function is unaware of what the App/Game class is.
12:30 My team lead will ding us on code review for not using initializer lists. We get dinged on code review all the time for any kind of unnecessary copy operation. As primarily a C# developer this concept took a lot of getting used to.
As the one doing a lot of code reviews at work, I tend to suggest just using a plugin like SonarLint (don't know what they support outside of VS and VS Code). Not too many annoying rules to disable, finds a lot of stupid mistakes and I occasionally learn new things (like why you should use std::less for maps with a string key). It also makes life easier, since it flags pretty much everything I would point out in a review and getting that feedback as you write the code saves a roundtrip through another review.
I think that if someone wants to allocate memory for `Game` type on the stack, they need to be aware of that `Game` instance not to exceed that the stack memory limit provided by the c/c++ runtime. For example, the runtime allow only 1mb of memory on the stack, but then if `Game` instance is 2mb then Stack overflow will occur. If you compile and run in debug mode maybe there will be debug assertion failed message will pop up. If you run in release mode.. Idk what will happen.
@17:00 I appreciate this object separation, even if it is a rather simple program due to the fact that it really gives room for the program to grow and a much more easily understood manner.
I come from an embedded background. I was taught to basically make everything using defines or constexpr. Everything done during compile time is free, and everything done in runtime is expensive. Compute is cheap, memory is expensive
How much memory do you actually work with? we're not in 1980 anymore, even the smallest systems have MB of ram, and a few variables won't make a difference. What will make a difference is being able to tune these variables immediately, instead of having to wait 5mn to recompile.
There are lots of.embedded processors with less than 1MB. RISC-V MCU - CH32V307 for example is 64KB SRAM + 256KB Flash. Lots of MCU don't need 1MB
14 วันที่ผ่านมา
@@feandil666 In embedded systems you don't always have MBs of ram. Heck, I've written code for a device with 1k words of rom and 192 bytes of ram in this decade. That was an extreme case for me, admittedly, but it was still a case.
The issue with the next-line-same-line bracket typically happens when your style is same-line bracket, but the code generators and IDE features typically use next-line brackets. I run into that frequently myself, mainly when it's not changable
I don't know, I wouldn't necessarily give fail to this code. I looked through the whole code myself, and first thing came to my mind is that the code is designed in such a way to piss off the evaluator. It has too many unneccesary abstractions and meaningless files, which just drains the time for someone who have to evaluate heaps of projects from heaps of students. But still, I would say author somewhat understands code architecture and coding in general, produced working program (I hope so, didn't tried to run it myself). I'm not sure if it was graded as F or it just was pass/fail type of test, maybe for pass/fail some people might give fail, but definitely not F level of code. Edit: I thought it was university project, if it was job application, this definitely is fail.
If it was a university project and there was no indication as to why the project failed, then in my opinion the university failed. Should be considered passed because the project worked even if it had not been completed with top marks.
@@FlorianZier if my goal was to teach someone how to code, I'd turned this down, BUT with some clear hints on what to improve. This is too messy and clunky to deserve a pass
if you want to tweak magic numbers live but don't want to expose them as editable property for something as simple as a prototype, a good practice is to use static variables in your function, this way you can break in the debugger, change the value manually, and continue running.
Anything else I missed? Why do you think this code failed? 👇
Don’t forget you can try everything Brilliant has to offer-free-for a full 30 days, visit brilliant.org/TheCherno . You’ll also get 20% off an annual premium subscription.
Hey, how can I send you code for review? :)
12:10 they're not storing the seconds (deltaTime) in last_frame_time, they're storing the value they got from SDL_GetTicks, which probably is milliseconds since system boot or something. Or program start?
E.g.
Uint32 newTicks = SDL_GetTicks;
float deltaTime = (newTicks - oldTicks) / 1000.0f;
this.oldTicks = newTicks;
I'm more used to the 64bit 100ns/tick from e.g. QueryPerformanceCounter myself, 32 bit 1000ms/tick should be good for 48 days.
You didn't seem to touch too much on the shared pointers. I'll put this up forth: it has been a while since I've watched you, I don't know if you prefer using them or not.
But as a rule of thumb, use unique pointers wherever possible. There are very few cases when a shared pointer is needed. Maybe I'm missing something and they are required in this case, but I doubt it.
The problem with last_frame_time being int is not that currentTicks is float, it's that currentTicks is Uint32. They are used to calculate the deltaTime, which is a float, but that's a different variable. Storing an unsigned int to a signed is worse than missing the fractional bits.
I'm fascinated by the triple std::move(this) in the Game constructor! What was he thinking there? Missing virtual destructors in the abstract base classes, not using references at all and so much more...
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.
is it rare to find companies looking to hire juniors like that?
@@TommyScott-g1p i asked
this is definitely enough for a junior, no doubt.
@@sournois90 Hiring a junior is much less expensive than hiring a senior engineer, so I could see companies looking for juniors which might not be as expensive but when trained can become a valuable asset to their team
@@ty.davis3 Hiring a junior also means you're waiting much longer for them to be onboarded and become useful though.
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.
Same thoughts... or.... ai generated :D
32:16 Cherno realized he exposed him lol
If only we could look at the Git history before accusing people and jumping to conclusions in the industry.
Yeah I was curious about the mix of camel case and snake case. Odd. Most programmers with any amount of experience will stick to a single convention.
@@CodeStructureTalk Git history? If I copy the code off of ChatGPT or somewhere else and commit it myself, version control is going to do literal 0 favor for detecting this? Idk man
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..
defines are almost never needed in C++ unless you are doing strange things like setting compiler pragmas, its highly unusual to need to use the C pre-processor in C++.
@@monad_tcp one other situation I might think of, is defining or prototyping something differently depending on another parameter only know at compile time. For example, for debugging purpose some macro could be defined, but with the content of the macro stripped off in release mode. It's not unusual and I don't think there's another way to do that except using #ifdef everywhere in the middle of your code.
While your specific example is bad, we do actually see a mix of that used and not used in the defines. e.g FRAME_TARGET_TIME (1000/WINDOW_FPS) but TILE_SIDE WINDOW_HEIGHT/15
s/brackets/parenthesis/ but I agree if you truly want to use # defines.
While you're right in C, in C++ you'd just static const these as variables. Notwithstanding the fact that just throwing these all into global space like that is an issue all on it's own.
There are plenty of good reasons to use defines, but you'd never use defines for variables in C++, with some exceptions if you're really deep into metaprogramming.
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.
it also seems like they pass around several shared_ptrs as arguments to functions, even though those functions don't change their ownership. they should be passing raw pointers/references, since incrementing the atomic counter (stored on the heap behind two levels of indirection) each time a function is called would incur massive overhead
I'm only a hobbyist, and I rarely actually write C++ and that stuck out to me. In my mind when you see a bunch of shared_ptr in one place, that's a sign to look really carefully at what's going on because either someone doesn't understand shared_ptr, or they're doing something really weird (this qualifies as "something really weird").
@@atijohn8135
I guess these were first unique_ptrs and he wanted to pass them as function params and it didn't compile because unique_ptr can't be copied. So he changes it to shared_ptr and then it compiles. I did that too in the beginning, it took a bit of time before the penny dropped on how this is supposed to be done.
This is 1000x what I feel. The entire thing is everything is managed through the Game class and executed through the Game class. It simply doesn't make any sense. The main shared_ptr will be there through the entire life of the program (since it quits when Game class is closing) so there's no reason to do so. Passing raw ptrs would be far more valuable in this case (and all of the cases where the shared_ptr are used seems to be used when these objects are basically guaranteed to be in lifetime). It feels amateurish (or copied from multiple different sources which is fine but some effort needs to be put in to make it homogenized and fit into your general programming styles).
Some people use shared pointers for all kind of pointers
They said why I write raw pointers and take care of deleting them rather than make it shared ptr and if I pass it any where I'm not afraid from suddenly crash and don't waste my time debug all if this shit
All book I read in graphics
Even hazel engine use shared ptr excessively without thinking
I learned c++ from this guy and I learned how to spread shared ptra every where from him (Yan)
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.
And because it's a variable (constant, but still) it means you benefit from syntax highlights in the IDE, warning you right then and there that you're using a float to something that accepts an integer (therefore truncating the decimal part). Whereas with a macro you'll only get that warning _after_ compiling it. But IDEs are probably smarter now... They should catch that even with macros. The idea is that macros are just dumb copy-pastes with no regard for C++ syntax rules. You often want to avoid that. It's considered a code smell.
@@zdspider6778 For me it's not just a code smell but, "Please step into the 21st century and leave macros behind". Of course, C programmers don't have any other choice but use macros everywhere even for "generic" programming. Large C codebase, like the Linux kernel, are full of macro hacks and compiler intrinsics for things which could trivially be implemented in C++.
Comment I was looking for. I see #define as regex, and I'am not fan of coding in regex.
#define is still the only way that I know of for conditionally compiling parts of the code
@@ferdynandkiepski5026 You can use C++ templates along with if constexpr to do so and where you can't, you can leverage your build system to conditionally include or exclude certain files depending on the platform. There is a Cppcon talk explaining that technique. I think this approach is more scalable than using pre-processors.
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
i feel like he watched a bit too many cpp tips videos
Or whatever the hell "std::move(this)" is supposed to do, especially three times in a row...???!
@@1337dingus fr that part broke me
@@1337dingus i totally understand the idea of it. the author didn't know about initializer lists so they used smart pointers to delay initialization into the constructor, and all those classes need to know about the parent, which is why they are being given this and they used move because they confused it with constructors that have && arguments in them
I suppose unique_ptrs are a little bit harder to use than shared_ptr, so as a beginner they might have been put off by hard to understand compiler errors.
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".
The inconsistent case, although ugly, is considered normal in C-style languages, like using snake case for variable names, and using camelCase for function names is very common in PHP and C++.
@@ДавидПлеван Different naming for different things, yes. That makes sense and helps to know what you're looking at. Three different styles for member variables within a few lines.. not so much.
As a senior with juniors in the team, I can tell you that some devs just commit code with inconsistent spacing, tabbing, variable naming and case, etc. It's not necessarily copy paste, just lack of care. Which I'd be half inclined to reject, mind you, because to me it's a capital offense. Just kidding, but not really. I guess my reasoning is, if you can't be assed to fix your format or check for consistency, then I wonder if you won't just strive to think as little as possible as well when making your logic.
@@Titere05 Same boat. I think "not giving a damn" is worse and more frustrating than "not knowing better". And as you say, if someone demonstrates no effort or attention to detail with the trivial stuff, how much effort went into properly reasoning about the code itself? Even worse, when it extends to the API. When even the customer facing part of software looks like it was cobbled together by a bunch of barely literate monkeys, you are actively scaring away your customers. If it looks that bad on the (supposedly polished) outside, just how bad does it have to be on the inside?
Its not even unique_ptr, its shared_ptr, so twice as bad when there is no reason to use atomic reference counting. (and also not even using weak_ptr)
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.
In this scenario, should you just put "this" as an argument? Or do something else?
More than a smell imo, it shows that a person understands neither what a pointer is nor move semantics and any more advanced C++ features
@@Debruggerthis
std::move @@literallynull
@@tendanium you shouldn't even have "child" objects messing with the "parent" object. (By child\parent I mean they contain one another). Because what if you created a "parent" object and then moved it somewhere else. Now you need to modify the pointer to the "parent" object in every "child" as well. In this case you should've separated each game state and created a "third-party" entity, that would monitor the current game state. Something more declarative, state-machine like.
"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.
and thus was born stardew valley an insanely popular money making machine with coding much worse than this.
@danielroden9424 Stardew Valley doesn't stress the machine very much, so the code doesn't need to be very good. It succeeded by its game design.
@@argon7624 thats the point. this game wouldnt stress anything either so nit picking technical details is equally pointless. in many cases good enough is exactly that. good enough. nit picking the design/inputs/mechanics would be a better place to focus.
@@danielroden9424 He covered that point in the video multiple times, and followed it up by rebutting that this was sent to employers, and saying that the quality of the code sent to employers reflects, at least to them, the effort, care, consistency, and experience that you possess as a programmer
@@danielroden9424 no because the point of stardew valley is to be a good game, the point of this game is to show off it's creators programming agility so he will get hired. It fails at it's purpose.
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.
Even if you don't it is not fine to leave the *option* to do so. Make the GameStateBase/InputObserver have a public virtual dtor if you intend to allow deleting through a pointer to base or a protected non-virtual dtor if you don't want to allow it.
Its insane how much you guys do shit just in case, and then wonder why your utter shit um apps took 8 years for you to code.
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.
That's brutal, it's a shame you cancelled. Don't doubt yourself!
It's still best practice to be explicit, because you don't know who will later need to read the code you write, such as professors. But, seems to me you shouldn't have lost the points, because your code was technically correct.
As a side note, with 30+ years of C and near 30 years of C++, I had forgotten about the implicit return 0.
@@nelnotheamoeba9221 I think this could have been the case with my professor too, he has been programming since the early eighties, maybe even late seventies, and the feature was added in C99. But why not look it up/raise the topic in feedback instead of just marking me down? I think he had either not heard of it in all the 25 years, or he's just set in his ways. Shame, he was a lovely guy in lectures. Maybe he thinks punishments work better the more you apply them? One thing's for sure, if you want a student to really dislike you - the best thing to do is try to control them. I will be leaving my return zeros off from now on. Job done.
I mean I don't work with c++ primarily but I do work with OOP design and have done some c++ in my job.
Experience wise, I'd consider myself intermediate.
Just because you can do something doesn't mean you should.
If you ask me, putting the 'return 0;' there is generally better. It shows clear intent.
The value of clear intent is making your code easier to read.
Implicit code in general is a messy grey zone.
When code is relying on implicit details, to read and fully understand it requires knowing about the implicit details.
Implicit code inherently makes code less readable.
Sometimes this is fine. I mean in browser-side web development, everyone know the local and session storage objects are on the window. It's fine to just write localStorage.mything instead of window.localStorage.mything.
When it comes to the main function. It's defined with a return type, and that type is integer.
When you exclude that, it's a function that is supposed to return an integer. But it doesn't return anything. What's going on?
Oh it's because the spec says that main functions specifically will implicitly return 0 if nothing is returned.
One key thing difference is that excluding 'window.' in the local storage example doesn't introduce any other form of weirdness.
Whereas excluding 'return 0;' does (function with return type doesn't return anything).
With that said, I don't think you should have been penalized multiple times for the same minor detail. While I agree that the assignment shouldn't be awarded 100%, the mistake is not so big to be more than a -1 point.
In fact, I think I'd judge it at -0.5 points once, with a note of the value of being explicit... Maybe inviting you to discuss it further with me.
I just like to be consistent. I end up moving code between languages frequently and relying on weird features that make C++ feel more like C is not ideal.
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.
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.
I feel like AI typically makes quite neat code that doesn't really make sense when you look deeper. Similar to how AI writes grammatically correct sentences that still have nonsense content. Though I've only tested with pretty small snippets, so maybe AI has messier code for larger projects.
@@vibaj16 I've made use of GPTs for code generation and, I'd disagree with OP.
When you see inconsistency i much more expect it to be human written and looked up online.
GPTs are generally pretty consistent since essentially glorified autocommplete
@@vibaj16ehh chat gpt is pretty good for these types of closed-ended tests. Like surprisingly good. If the style is super consistent, and above all, it has a ton of comments for basically every line of code, then I think that’s much more telling of AI.
@@vibaj16although with newer stuff it tends to go awry, like concepts. Especially Gemini, Gemini really sucks with recent modern C++, way more than chat gpt
Yep, it does look like patching many answers from stack overflow without really understanding what is going on.
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[]
He also says how it's a special function so it doesnt need to return an int. It absolutely does need to return and if you dont put a return statement at the end, the compiler will add return 0; in all branches of code that you havent put a return statement
@@josipcuric8767 what he meant is you do not need to explicitly return, it will return implicitly.
Since C++11, if the main function ends without encountering a return statement, it is implicitly assumed to return 0. This is a feature intended to simplify the code and reduce boilerplate.
@@josipcuric8767 When using SDL, the CRT main function (that gets implicit return 0) is SDL_main and not main, so your main function isn’t special (other than that it gets called by SDL_main) but rather an ordinary function.
@@albertcheong8497 same thing, return 0 is added becuase its needed by the OS
If I understand it right, SDL hijacks the "main" function of your program and calls it as a regular function
Because of the framework, it looks like you are implementing the C++ main function but you're not. So you should not rely on special behaviour of the main function.
(Not a SDL user tho so I'm not sure)
33:11 - 100%. I think this is both correct, exactly why style guides exist and why they should be used by folks even for hobby projects.
Style guides serve as a framework through which you can apply consistency to the your architecture and code, that allows your intent to be significantly more readable. Style guides like the C++ Core Guidelines distill decades of experience into easy-to-follow principles that can serve as a launching board for deeper exploration into why they suggest certain things, and force you to justify why you might disagree with their approach.
I'd love to see a series about software architectures
just look up a book or some YT series about SOLID principles.
I suggest Refactoring Guru Design Patterns website as a good resource.
Most books in elementary computer science cover this. I don't think it's much of a mystery nowadays.
@@Raspredval1337 He meant a series from Cherno. Others are boring
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.
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".
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.
And the costs are not just in performance, but also in cognitive load. It's not so bad in such a tiny example, but it adds up in large projects.
It's always better to avoid premature abstraction.
what does indirection mean
@@farukyldrm8498Indirection is when you need to jump around to understand something.
@farukylrdrm8498 referring to something (code/data) through a stored pointer. In this case, it's a virtual function call, because the code is implicitly accessed through a pointer
Wtf no it does not add up, you always have only one vtable inside of an object no matter how many layers of inheritance you have, therefore it's always exactly one pointer lookup for a function call
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.
there's zero reason to use `new` in modern c++
you are not going to be making new containers, I guarantee you
just use all the standard ones
@@NoNameAtAll2
In most cases, you're right, as most modern devices have more than enough resources unless you have bad coding habits. But sometimes one may have to control granular details in assemblies or may encounter significant memory limitations. Stack must be allocated during compile time and libraries that provide smart memory managements have both temporal and spatial overheads (and still use heaps anyway). Manual memory management is useful in these scenarios. For once I have worked on a project where most standard C++ libraries weren't available.
You must have had pretty bad professor. When I did C++ course during my CS studies 20 years ago, the professor made sure that I used const references instead of copied arguments and I allocated practically everything from stack. I would have failed that course with a code that looked like the one in this video.
@@MikkoRantalainen
Personally I wouldn't blame him, at least for now. He did well explaining both the concepts and implementations of OOP and DS. And once these are in the picture, one essentially comes across memory blocks that should live between calls. That's where stacks cannot fulfill the purpose and heap allocations must be used. Though it would've been better if he mentioned that variables that would not live outside the function should stay on the stack and passed as a reference or a stack pointer in the code.
@@NoNameAtAll2 new is no more use in modern c++ 😮😮😮
and... what you used ?!
29:20 Using clang-format will eliminate most styling/formatting inconsistencies in C++ code and should be used in every project however big or small. Just define your formatting preferences in a .clang-format file and clang-format will do the rest.
I am finding it very hard to produce a clang-format that will format things the way I like things, it's not easy to work with, ages ago I used to use a VS Macro program I got off Codeproject when that was the big thing, it was far easier to get results I was happy with, also the older beautifier programs like AStyle seemed easier to use.
I wish there were sets of "Moron formatting" examples for each setting, as otherwise the changes each setting does can be too subtle on normal code.
@@peternimmo74 I used to program in CLion few years ago. I preferred the way it does formatting. So when I decided to migrate to Neovim I just exported their C++ formatting style to .clangformat (there is an option to do that). Since then I have made minor changes to it. Given the importance of this tool, I think, it's worthwhile to invest some time in understanding how it works and reading its documentation. If you are familiar with C++ syntax, their configuration options just makes sense (although they are quite wordy).
Thanks
I'm very triggered by the shared_ptr use. It would the THE reason for me thinking someone doesn't really understand memory ownership or memory at all. Add up the unneeded heap alloc in main, and that would be enough for me to say... hey, maybe not hire this person as a senior. As a junior, sure.
Whether #define's or something else, it's actually quite helpful to have all settings in a single file. Games and a lot of other apps often end up having a configuration file that is loaded at startup, but a single source file is a pretty common way of doing that when it's not worth the trouble to parse a config file.
In case anyone wonders "Why be so pedantic?", well ... once code is over six months old or so, it will never be removed except when there are explicit issues. If someone gets poor code checked into your project and it manages to bake for awhile, you basically get to keep it forever. Even good code decays, which means at some point you end up running as hard as you can just to stay in place. Having high standards at checkin is a way to maximize how long your project will be viable before you get bogged down and have to rewrite everything (or have a competitor replace you).
[It's possible that none of this really matters for most projects, but usually people aren't asking why they can't get a coding job at an auto dealership, they are asking why they can't get a coding job at a FAANG. If you know good code hygeine, you can selectively decide to forgo it in the name of speed, but if you don't know good code hygeine, it'll take you a year or two to break your bad habits.]
This code gives off a cargo cult vibe, as though the writer copypasted from various sources without actually understanding what each element is for.
As a somewhat beginner, this is my worst fear when it comes to others checking out my code lol
@@msherif428 lmaoo so true
@@msherif428 in general it's fine to copy-paste snippets to save time, if you understand how they work and why they might be written a certain way. However if you more like 'I have no idea what i'm doing, let me just copy-paste something I found on stack overflow to see if it solves my problem', then I would say you'd be better off spending time understanding your problem better and then exploring common ways to solve it.
The advice I give every junior is the one that was given to me. If someone asks you why you did what you did at any point in your code, you better have an answer. It can be a wrong answer, that's OK, but at least have a reason. And that really forces you to investigate what the hell you're doing
The parts that are weird and disjointed are the boilerplate bits to setup windows and interface with SDL and whatnot which frankly everybody does the first few times on a new toolkit. it's super likely that they were cribbed from various sources so the coder could concentrate on the game bits.
Actually, there is another reason why you should use member initializer lists and not assigning to variables in constructor body, namely const. You can initialize const member, but you cannot assign to it.
A (non-static) const member is almost always a bug. Having a const member means your object can not be assigned to or moved from, meaning it cant be swapped. These are fundamental behaviors required for performant use of containers and algorithms.
If you want a member to not change, make it private.
static const is fine and recommended best practice for C++
@@ulfben Interesting. Is it true always, or only for default move constructors and assignment operators? Also, do you have an example off the top of your head for when you would use const member?
Regarding the defines: I had a professor who really wanted defines. In Arduino code he made a variable named pin13 (for the led), then up above he defined OUTPUTPIN13LED 13; . He felt that pin13 wasn't a descriptive enough name (or even ledpin). This video isn't my code, just saying he might have a professor (or whatever) that wanted things that way.
I remember a rust lib where pins in output mode and input mode have different types, which makes sense. Should be possible in c++ to some extent though harder without ability to redeclare variables with the same type
17:30 this looks like an incorrectly implemented "interface" in C++. After Java and C# people liked interfaces and wanted them also in C++, looking as interfaces appeared after C++, people implemented them this way. Ngl they look pretty janky. To be correct, it should have defined virtual destructor and the methods should have been pure virtual methods
14:00 Actually, no. It has a huge bearing. Initializer lists allow to optimize or inline out the constructor call ( which isn't really a function, btw ), which in turn may exclude some optimizations, which has an impact on performance in tight loop situations. It is always preferred to use the initializer list instead of initializing the fields in the constructor body.
Constructors are special functions, but they're very much functions.
What do you mean the constructor is not really a function? Everything is just global functions in the final binary
@@zeez7777 What is or isn't a function is a little blurry concept - but in case of constructors the 'function' part is its body. Which, if it is empty, the compiler will totally omit and call , in the same manner, constructors of the members without creating stack frames.
It is _technically_ a function, but it is so special that it is best to consider it a separate entity with its own rules. Which is also why it is best to use initalizer lists as much as possible. On a hot path of a program this might yield massive performance gains. In one example of code I was working on I gained as much as 50% speedup from merely switching from explicitly assigning fields in constructor body to using initializer lists.
@@TryboBike Nice, i'm pretty sure this happens with any function though, its just certain compiler optimizations being "allowed" by the nature the code is laid out. I dont really agree on that functions are a blurry concept. Looking at my own examples with different levels (or no) optimization in IDA makes it pretty clear.
@@TryboBike A constructor is a function, whatever happens afterwards, doesn't disqualify it as a function
16:52, what you are talking about here is that the that code is violating the O in SOLID, the open closed principle. The code should be closed for modification, open for extension. The amount of events growing means you may have to keep adding functions to that class to handle different extensions, so it is not closed to mofication, neither is an open to extension, because you cannot extend the amount of events it can handle without modifying this class itself.
Actually, SDL requires 'return 0;' because it wraps the main function in a macro. Not sure of the technical details, but without return 0, your game will throw an exception on exit.
Another way the #defines could've been handled is with constexpr. constexpr would have kept the data const as well as potentially inlining the data.
At my uni, our algorithms class was taught in C by an old school C guy who taught us to use #defines all the time. I assume there's something similar going on at a lot of uni's, and that's probably where people pick up the habit from.
I'm not an expert on this (I don't use C++ in my job), but I read a lot of embedded code as a hobbyist, it's really common to have define, and is probably true too for the Linux kernel IN C, I feel like it's just C++ convention that people dont do it? I see it really frequently in C code but not C++, even people coding in JavaScript (games, global const) Java (Android source, public static final) does that I think, just don't know much about modern C++ : /
Same. I think it's an embedded thing. When you only have 8kb of ram you don't want every setting to take valuable variable space.
You can use defines for things that wont change (pi, degrees to radians conversion, stuff like that) but when you make a video game prototype there are obviously tons of stuff you want to tweak. So you can at least declare static variables you can tune in the debugger.
@@timonix2 all variables need not to be in the data or stack, they may just be optimized out and embedded into instructions by the compiler
I think one issue is that some C compilers won't make statically sized arrays, even if your variables are const. You *have* to use literals. whereas C++ has constexpr
12:18 he didn't lose anything here. I guess a better name for this variable would be "lastFrameTicks", but all information is preserved. Well until it overflows, cause he stores unsigned int in int
still, they use uint32 sdl type to then convert it to a float and then to an int, why? 🤷
@@Raspredval1337 they use SDL ticks to compute delta time. And that delta time is not converted to int in the end. It is used in update function and that's it. The thing that gets stored is the original ticks that we got from SDL
It never gets "converted back to an int". It uses the exact value from SDL_GetTicks(). The real problem is that we store deltaTime as a float and lose the exact precision.
@@Raspredval1337 They're not converting it to a float though, yes, the Uint32 is being used for the calculation of a float, but that doesn't mean that the underlying variable is a float after that. last_frame_time is being assigned to currentTicks. currentTicks is Uint32, last_frame_time an int.
It's just the unsigned to signed conversion
@@Raspredval1337 they don't convert it back. They create a new float variable based off of the int, but keep the int as itself, never modifying it (unless I missed that part of the video)
Bro you should make more of these. I found this hella interesting and learned quite a bit. Keep up all of the good work :D
I'm the most-senior C++ engineer you'll find: I was publishing articles in magazines about it in 1989, which comes to 35 years of being proficient.
I don't use m_. I think it's wrong to mark variables' scopes as part of their name.
The code in the body of a function will use names that are local, parameters, members, and globals. It _does not matter_ where the variable lives when calculating with it. In fact, as code evolves over time, the variable may move: what started out as global may become a member, to allow different instances to have their own values; it may move from a constant (defined locally or globally) to become a parameter to the function. Why should that affect the code?
Modern editors track the home of a variable for you, by color-coding it and showing you where it's defined.
Yep. These prefixes are an artifact of using old, bad tools.
100% agree.
also, doesn't m_ stand for member?
what does it have to do with its visibility?
@@0netom I don't know about m_, but just '"_name" used to be pretty common. It's a relic of the past though, when dev tools weren't as mature as they are today. The idea being that when reading the implementation(usually in .cpp) you don't have to jump back to the header file to check the scope of a variable, if it was private the "_" would make it visible at a glance
I'll be honest, and this is probably not normal, I love when an application logs random stuff. It's like a peek past the curtain without having to actually peek past the curtain, and, if used with discretion, doesn't impact much.
The Legend of Zelda: Tears of the Kingdom, for example, logs an autopsy report of every actor that reaches 0 HP. That's fun and cool!
where is that record stored?
@@Zeutomehr Play Report telemetry data gathered by BCAT, uploaded periodically to Nintendo. They're stored in system save 8000...00A1, iirc?
@@valshaped thanks :)
I think the biggest compliment I got in college was my engines professor saying, "Thank God you write clean code".
I often overthink before writing code nowadays because I get so worried about it being unreadable to someone else, or unorthodox, or confusing to the point where I have to spend time reunderstanding my own code.
So hearing that boosted my confidence a bit when sharing code with others. I'd imagine though, as a college professor, he's seen really, really bad code, so even mediocre code could seem amazing.
this code surely has a lot of problems, but this is a huge project for a job application
I suspect the prospective employer has asked for some code he has written previously to get a feel for his abilities rather than the application asking for code to be written.
To me, this looks like one of the Applications for some gamedev. In the past I applied for a bunch of these Jobs and you have a Week to do this... for every Application.
I wrote 8 test back in the day until I said F***k it, I go somewhere where they do not waste my time this excessively.
It is often expected of you to have experience of shipping something, but you just got out of University - how should you know this?
Me, now working outside the Game Industry, can say no one expects a Junior to be able to know this - well except the Games Industry...
Applying for a Junior Position in the Games Industries is a miserable experience.
It's tiny. The only reason why it looks huge is the code style choice.
Don't know about 'huge', looks pretty typical to me.
@@KoraktorXVI remember reading a JavaScript book, and the author pretty much said if you value your sanity never be a game dev, lol
instead of settings being just const, I would set them as constexpr, which would make them fixed at compile time. They are not changed anywhere anyways, and with that could be used in constexpr functions to improve code performance.
Then you probably missed the part where the macros were likely created precisely so the author could tweak the harcoded values.
If they're variables (not even const) you can then adjust the values at runtime, either through dev-only input handler, or a debugging session. This would be faster to iterate than a recompilation followed by relaunch of the game.
@@dkosmari You can tweak constexpr values the same way you can tweak #define values but you dont clutter your code with defines.
I get why it would be beneficial to have them as normal variables for tweaking, just saying that if you want to have a constant, it should probably be a constexpr
Bet the iostream was there for some print debugging, the fact that include was left in is enough a hint of this. Ideally of course we never would use generic print debugging, bet everyone has including the reviewer :D
I would say the reviewer who failed the code was wrong.
Overall the project seems quite decent and C++ like. The biggest issue I saw was probably vectors passed by value, and too many shared_pointers for state, and some pointers to this.
But that's not a big flaw. Because explaining pass by ref vs pass by value is something that would be easy to explain to a person who built project of this size. And pointers knowledge I consider a bonus, because if a person can create a working project with pointers that does not crash or leak, then it already shows a deeper level of understanding about the language.
Looking over the code it looks like it has been done over some time, there are some mixed styles, but that's exactly what you would expect from a person who's learning and improving. You start from basics, you learn new methods, but you don't always go back to rewrite everything. The commit history also shows a decent code evolution.
Addressing the chapters:
- The shared_ptr and includes : Looks like this is an evolution from SDL_CreateWindow in commit history, which is an ok approach. Good that it was promoted to shared_ptr. And who knows, after a while, maybe some other threads will need Game object, so shared_ptr is not that awful. Includes, I don't think people clean up includes that much in real projects.
- The main returning 0 : That's a good thing. Even if there is a quirk in C++ standard, you don't want to rely on other developers knowing all quirks.
- The private/public blocks, _m : Good practice, and looking at the project, I think it would be easy for the developer to adapt to such coding guidelines.
- The last_frame_time is an int timestamp, and the code is correct.
- Initializer lists : Ok, but you would use last_frame_time{}, or C++ modules in modern code anyway. I would avoid assignment.
- structs vs classes : I would say class is preferred when it contains other classes. Use structs for POD types only. Calling memset on a struct that contains classes is a bad thing, and memset use is common for structs. Struct with classes will also cause problems on module boundaries.
- Class hierarchies : Reviewer should know that you can implement same logic in multiple ways. This shows the person understands some OOP, and it's a plus not a minus.
- States could be more elegant, can be done simpler, but it's ok for a small project.
- Defines : Ok, partially bad, because if they are not wrapped correctly you can get unexpected results. If macro expands to a sum, but is multiplied by a value in code, you'll get wrong results. But the code is based on SDL, and if you look at SDL examples, they are macro heavy. Also, macros do full compile time expansions and avoid dereferencing or operations at runtime. Purely from gamedev perspective that's fine. It doesn't matter nowadays from speed perspective, constexpr or consts would be better, but it's not completely terrible if the code is SDL like. And in the context of gamedev where you want more frames per second it's better than pure virtual config classes for example.
- Code layout : Just use an auto format of the IDE. It takes some time to adapt to a coding style. It's really not a big deal. I mix brackets for a while when switching languages.
- Event handlers and shared_ptr : Not necessarily, you can have a handler that's just a static function that will persist through the whole application lifetime, raw pointer would be better then. And if even handling is stopped before engine shuts down, raw pointers are just fine. No need to add extra overhead, there could be a lot of events that are emitted, why add refcount overhead?
So overall, I think it's a decent project that : works, shows knowledge of pointers, shows knowledge of OOP, shows code organization skills, decent Git workflow, and ability to integrate with systems like SDL.
Failing this project without a constructive feedback is a very unpolite move, sadly it is what industry defaults to these days.
Project is quite cool for someone who's not a senior developer.
I think I'd have dismissed it as it looks like it was written by AI or at best extensive copy & paste and they'd not understood it enough to clean it up. It's like obvious lies on your CV "I invented the colour purple" - no you didn't, Next!
Seems like people online are always concerned with scale, so they introduce a bunch of abstraction just in case you want to add 100 of something, even though right now there are only 3. That not only slightly obfuscates what is going on for this simple case, it also is slower due to the overhead of the abstraction. I get that this project is meant to showcase what the dev is capable of, but less abstraction can also show an understanding of what I just described
it all depends on who is your interviewer and how well you can defend your architecture/code decisions
Well in this case the lack of abstraction was quite, well weak. If you're having an input observer class, I'd like to see everything there and with maybe 2 events one for position inputs and one for key inputs. Either way it would be an improvement as it would allow for dynamic key assignment which would be a big plus to see. I'll add that this isn't a far off case of "scalability" anyways. In this case, fundamentally there isn't a ton wrong (since the code works and does what it's supposed to) but there's no point in the code where there's not a "questionable" decision. If you have 20 other candidates and you at best fall in the middle of the pack, you'll likely be rejected when they're hiring for one position. This is the ultimate point though.
29:20 I worked with a team of maybe 60 devs on a pretty big codebase (half a million lines of C++, almost as much Perl 🤮) who standardized on same line { except for the beginning of a function block, I’ve seen it in a few places too. It doesn’t bother me at all, I think the reason for this failing code review is almost certainly due to not passing the vector by const reference; this is super basic stuff and if input objects aren’t being changed they should always be passed by const reference. Also the “Running” as a public field instead of a function *which should also be marked const*. I think just the lack of “const” in general. Good review, you’ve earned a sub :)
In terms of variable naming conventions, such as m_Something for private member variables, k_Something for constants or pSomething for pointers, i.e. hungarian notation, I think best practice is to just follow the most commonly used style in the domain / programming environment.
e.g. when you’re writing a C++ library, use snake_case() method names, but when you’re writing an Unreal plugin, use their weird naming convention, with F and A prefixes.
Personally for my own C++ codebase I have developed a specific style that uses PascalCase for class names, and no prefixes for any variable names, because any IDE should be able to show the type and access modifier of a variable.
But consistency is key. I’ve worked on a codebase where there were multiple competing standards, and that is not ideal.
I respect conventions and of course everyone can write code how they want. Having said that, there is no reason at all to use any kind of prefix, m_ archives NOTHING it's ugly and unnecessary. If I see someone write beautiful code and they use m_ k_ whatever, I don't care. If I see a junior dev write medium code but they still use m_ then that's a big red flag for me, especially if they can't give a reason for it.
Blindly flowing "bad" advice is just dangerous, and critical thinking is extremely important in programming
@@onebacon_ I agree with m_ being ugly, but let's say hypothetically you're working on the Unity engine, and internally m_ is used everywhere, then it's important to stick to that, as inconsistency is worse than m_
@@onebacon_ I think m_ or at least _ or something else is good to distinguish members from local variables. If C++ required using this it wouldn't matter.
the m_ (or the trailing underscore, which I favor due to it not being so much of an eyesore) is not so much about knowing it's a member, but about giving you the freedom to define a getter later without the redundant noise word "get". It pulls complexity inwards which is always a good thing.
@@isodoubIet funny you mention that, I do exactly the same thing in my c++ codebase, e.g. a public vertexBuffer() method that returns the private variable vertexBuffer_ (with const-correctness and references vs. pointers and everything of course)
Olivka (оливка) is a cool name for a cat!
после произношения этого имени я в серьез задумался над местом рождения Cherno))
mb it Czech or Polish or something like these. Oliwka.
@@IvanConor well for me it sounded with perfect Russian pronounciation) So I'd say more like Russian, Ukranian or Belorussian.
I'm glad he got her input. She looked like she had some criticisms she wanted to share about that dodgy code.
The naming convensions of m_ is in my opinion just useless clutter that makes code unreadable. It made sense a long, long time ago when we didn't have proper access specifiers and such. But it has been obsolete for some decades.
The feeling I got from the code is that the author does not really understand how things like memory, ownership, and lifetimes work.
Passing stuff by value? Using shared pointers everywhere, even where the game object should be owning the (I presume) one single game state? Vectors of raw pointers?
While all this may work for this small, single-threaded application, such an approach will become a major headache down the line.
Regarding the style, that is really good advice. I often see new programmers scoffing at following best practices when it comes to code style and formatting. But to any professional, that is the first thing that they see, and the first impression they get. Spend an hour making your style acceptable and consistent, if only not to trigger the reviewer needlessly.
Regarding the formatting, if you ever want to share your project with others, especially for interviews and such, just set up an auto-formatter, like clang-format. Takes a minute to download and a couple minutes to tweak the config file to your liking, and practically all IDEs support it.
Honestly, if this was for a junior job application and it's not like FAANG or whatever... seems a bit harsh to reject it. I think the major red flag, as you pointed out, was the heap allocation everywhere which could indicate that this is a person that doesn't understand memory management. I would at least have a on-site follow up interview after this to be honest :)
Exactly. Seems like most comments expect perfectly formatted code without any issues. The classic 5 years of C++23 experience for junior position. All flaws in the code can be fixed in like 1h after a code review with a senior developer, which is part of their job. But apparently that's not how we roll these days. Project looks like a lot of effort, and GitHub history looks quite logical.
@@CodeStructureTalk Depends. If it's a highly competitive position (and there are 20 other applications which is the case at most large studios), some applications would be rejected outright. For Game Dev positions (which I'm assuming this was for), having a personal github with good projects has a lot less importance compared to a solid resume. At the studio I work at, we don't really look at github projects (unless they get to the shortlist in the first place). This isn't a reason to reject someone, but if you have 5 other people who write better code and/or have a better resume that matches what you're looking for, you can easily get passed over without ever receiving an interview. It's unfortunate, but it is what it is.
@@wakannnai1 Ok, fine. But the issue with this is that the interviews are completely random. There might be resume filtering that drops the resumes that miss some keyword, I mean, maybe you're missing 'pthreads', then the resume can be dropped because the particular reviewer does not like two column layout and prefers tags at the top. Another reviewer might drop the resume because there was a mix of camel case and snake case. Another might drop because you used templates instead of pure virtual classes. Another will drop because events are not abstracted. Another because shared_ptr is used instead of unique_ptr.
The reality is, the process is completely random. And doing assignments is mostly an unpaid waste of time.
The person who submitted the code was left wondering what is wrong, and possibly shaping his view about the industry as a whole. While the popular comments regarding rejection are: The naming conventions and brackets are wrong; He should have had a senior developer level of knowledge regarding constexpr! SDL uses macros, but your macros are bad, and similar cosmetic issues. Number of actually relevant notes in the comment section is not that large.
In the end, it is a game you cannot win. Might as well be a random number generator selection. No useful feedback, no one actually looks at the code evolution and how person has achieved a goal. Complete waste of effort.
@@CodeStructureTalkWonderful comment, 100% agree
I guess maybe there were enough better interviewees to fill all the available positions. I find it shitty that they didn't provide feedback, though, forcing this person to ask a TH-camr for a code review. Telling someone this is not good enough without telling them why is just wrong.
The allman/k&r curly brace style shift makes sense in this case because 1) function scopes cannot be nested and 2) having it on newline makes grepping for functions easier (but in this case the return type would have to be on a newline as well)
A lot of this seems style related. Some just inefficiency.
My big issue is the triple std::move(this). Structurally this is wrong because this is declaring “this” be invalidated but then it’s used on the next line (twice) which is bad. It doesn’t crash because move won’t invalidate a pointer and is effectively a copy. It makes me think he was doing something different than what he thought or meant to do.
Move doesn't invalidate, the value must be valid past being moved, and in fact all stdlib types work with the explicit rule that moves leave the moved-from object in a "valid but unspecified state" -- it's not a problem in itself to continue using the moved-from value, but you cannot assume anything of it's state post-move.
It's a logic error, but not undefined behavior.
One thing i guess i can contribute finally is about the defines. at least when you write bare metal C/C++ you dont really want to put more load onto the little crappy microprocessor than you need to. So having the preprocessor insert loads of things is preferred. compared to filling up the tiny little memory on this little thing with constant values that never change.
Right off the bat, if you don’t know why you failed then whoever failed you had failed as an educator.
11:54 that is actually correct, it's supposed to be an int (ok, technically it should have been an "Uint32") because that's what SDL_GetTicks() returns, it's the number of milliseconds elapsed since SDL was initialized, no loss of precision there. And it's perfectly fine to then calculate deltaTime as a float, since deltaTime usually something you'll use to interpolate stuff later, so you want that variable to be a floating point.
8:56 I don't think you have mentioned naming conventions like this in the C++ series--would you be able to do a video on the ones that would stand out to people in the industry? I don't work in the industry, but I don't want to develop idiosyncratic coding habits that makes my code look weird to potential collaborators.
There's more code style conventions than stars in the galaxy, so there's little point in focusing on any one specifically. What's important (especially for an interview like here) is consistency, not only for aesthetics and "clean"-ness, but mostly to show that you know why conventions exist. Seeing a mess of mixed naming styles, declarations everywhere, random member visibility and mixing all types of member initializiation for no reason immediately says "this person has never worked on anything bigger than 1000 lines and has no idea how to manage growing complexity if they can't even get use a consistent style for trivial stuff like visibilities"
@@Debrugger That's why I was asking for things that would stand out rather than a preferred convention.
In the C# programming language convention is to use lower camel case names for the private fields, and generally also starting with an underscore, but that might differ per project.
The eye hurting old school `m` is almost never used in front of the underscore. Which is also very unneeded in my opinion.
And public fields and properties (of which the latter are a concept not known in C++ but similar to getters and setters but then with usage like it where a variable) are used written in Upper camel case.
But I also like the naming conventions of Rust a lot, which are super consistent, like many incredibly great things in the language.
Go does it different again, and uses Upper vs lower case to automatically make functions public vs private. Which I kinda like for it's simplicity.
The only thing that reasonably matters is consistency. Most people agree on common conventions like local variables being camelCase or snake_case, while types and structs are PascalCase... but not even the C++ standard library follows these consistently. The most important thing is that your own style is consistent and has clear intent, unlike the code in this video which was obviously copy-pasted from various sources.
Older languages like c++, c, etc, often do not have a single standard for how to write things. So there the most important thing is consistency (as people here have said). Newer languages like C#, rust, Java, etc. often do have an explicit standard as part of the language. In that case you should follow that standard. You shouldn't write a rust program in camel case for example.
one thing I note early on, is inconsistent case usage, it starts with Pascal case, then snake case and then goes into camel case. That's kind of messy. I'd definitely give a negative point for inconsistency.
I can say as someone who constantly switches between languages with different conventions, I sometimes find myself using both snake case and camel case.. and curl braces on the same line and a new line (really only applies to c++ with that one which I don't really use).. and it irks me when I do it and dont catch it
Zig uses mix of styles. Snake for vars. Camel for types and functions. I felt so confused when started to learn it
28:58 i love the rant here! Since I control the CI build processes, I put in code beautifiers that immediately fail PR builds if the source doesn't exactly match the beautification. It's an automatic pre-inspection for me because the juniors need to have their builds run green before I even start my review. Similar with a lot of other things you found: add a linter gauntlet to the build process so everyone needs clean code from the automation checks before the human code reviews start.
That was a lot of hanging/aligned braces inconsistency and defines. I've never used or probably even wrote that many defines before. Not even when I use Unreal's debug helpers as macros in a separate header until shipping
I love these videos @TheCherno . Very instructive. Also would like some videos where you go deeper into a topic using actual code like this as a way to show how to better approach a problem.
About public private members. When designing class, you expect your teammates to use that class. If some member variable is public, it gives programmer a hint that they should modify that variable outside the class. I work on unity game and I am going nuts when somebody initializes member variable as public ClassName VariableName only to find out they just wanted to assign it in unity's inspector.
It always makes me question, should I assign that variable somewhere? If I do not is it a null? It just takes extra time to figure it out.
Same line braces is generally recommended in JavaScript, due to the fact the language doesn't force you to use a terminator at the end of lines, e.g. ; so something like:
return
{
property: value;
}
Will return undefined instead of the object you might have expected.
I feel like i just had a mini stroke reading this. Javascrpit... whitespace terminating. I can't.
That's one of the most ridiculous things I've ever read
JavaScript has idiotic ASI (Automatic Semicolon Insertion) which can cause subtle bugs and there is *no way to turn it off!*
@@MichaelPohoreski Stupid, yes, but modern tooling points it out right away, so it's not that much of a problem nowadays.
The configuration system of Grbl CNC is based on # define macros, and this ended up being adopted by Marlin Firmware for 3D printers. These codebases depend on meta-programming with liberal use of macros, including some ugly recursive macros. The main challenge for these projects is to be able to target multiple CPUs, from 8-bit AVR to 32-bit ARM (plus a simulator on Linux/macOS/Windows), producing the smallest and most optimal binary for the enabled features. It uses the PlatformIO build system (based on SCONS) which provides hooks that allow for simplified configuration, but at some level these codebases still need to radically optimize for size, and # defines do the job. These codebases would be interesting cases to see whether new and better approaches could be applied to this kind of problem.
I'm in two minds about the defines thou, yes they are not real variables but sometimes, they don't need to be at all! They just serve has a easy of life for typing repeating stuff, they also make it easy to turn on and off peaces of your code and also make compilation way faster (by not being compiled at all...).
Has a c++ coder I don't think you should lean to much on C defines, when a constexp or even a older const type could be used but I also don't think c++ programmers should be afraid or hate defines, in the right hands defines are very powerful. Just my opinion.
I think both has uses... I use defines to unlock some behavior in included file. Sometimes I want debug logs to not happen.
For DEFINES that are meant to be used locally in the same file: Consider making them variables and sticking them in an anonymous namespace in that file. Plus, consider leaving them capitalized for readability reasons. The reader will know that they are configurable constants at the top of the file.
At around 8:15, I think there's a strong reason to return zero, because the function definition itself specifes that it returns an int. If you're not doing it here, then there's the possibility you're also forgetting to do it in other functions. Compilers /can/ let you get away with this if you haven't set the compiler to be pedantic enough, and it can lead to some strange bugs that are VERY difficult to track down. I've made this mistake before and it resulted in an entirely unrelated variable not updating in the code. What was even worse was the fact it only happened in the compiler I was using with a particular microcontroller. The same section of buggy code in an online compiler ran fine!
2:45 This might be nitpicky, but you absolutely don't need to free or delete a raw pointer just before the end of the main function. That memory will be freed anyways on termination
I have had to stop caring about which style of curly braces I like better. I use both JavaScript (well actually TypeScript) and C# and the standards for both are different. C# wants curly braces on a new line, JS/TS wants it on the same line so I regularly use both.
Heresy!!
I never understood curly brace on same line, it makes no sense for many reasons, the most obvious one is that the symmetry is broken.
@@zeez7777
Do you use a new line for if statement curlies as well?
I’m still at a point where I prefer the curly at the end.
I feel a new line curly adds unnecessary white space, idk, it feels as if the function body is detached from its head.
It feels even worse for if statements.
If you don’t use a new line curly for ifs then your symmetry argument becomes invalid.
If you do, then I fear your code will be hard to read, at least for me.
@@CoderDBF Yeah i use it everywhere including if's. For me it becomes a lot easier to read because of the "free" line you get, otherwise consider an if -> else if -> else with curly on the same line: it feels needlessly compact and squished, making it arguably harder to read. Of course there are limits but spacing things out is a universal way of making this look better, in code in user interfaces, pretty much everywhere.
@@zeez7777
I’m going to give it a try this week.
Perhaps after a few days I will change my mind.
You have compelling arguments, it might be a matter of experience on my end.
Man, college me would have LOVED getting a free code review of my projects haha
What do you think about a namespace of constexpr instead of the defines? Of course I prefer the struct as owned by Game, but if you're going to make it global anyways, why not a namespace?
For things that are truly constant, this is the way, imo.
Drop project-scoped constants can be in a named namespace, and file-scoped constants into an anonymous namespace to keep linkage internal to the compilation unit.
@@anthonybrigante9087 Nice, glad to hear I'm not too far off the mark then :)
I also prefer namespaces because classes are supposed to have instances. I see no reason for instance of config to exist (barring Json reading). If no instance necessary, might as well omit the class.
33:59 you're focusing on the code-style, but not on the basic performance reason in that: defining function inside the class definition will tell compiler that this is inline function. Best choice here is to keep balance. You should put these funcs in CPP and explicitly call them inline.
Missing virtual destructors was enough for a fail in my book.
What's important is that should be picked even by compiler warnings. So it's double fail - not doing dtr and not reading compiler output.
I swear to god people need to program with Werror for some time to learn manners
One thing that struck me was I didn't notice a sleep(1/fps) statement somewhere in the update() method, it just while'd away the cpu cycles by polling if frame target time has passed or not. If you while() loop something, it'll just hog all the cpu or cpu core cycles. I'd use a timer to execute the update() method, with the 1/fps as interval. Most desktop operating systems are an event driven system after all, not a microcontroller where it's more common to run something in an endless while loop, though even then you'd put the device to sleep with an interrupt or watchdog timer. It would draw unnecessary amounts of power and potentially slow other processes. That could be a major factor for a fail, especially if the application was geared towards game design.
Also, all the string literals could be placed in language files so you can translate them into other languages, then use defines for the indexes so it's clear what text goes where. Settings could be an ini file. Using defines for settings is the same as hardcoding literals in your application and per definition not a 'setting' that could change. Yeah I know you could redefine them at compile time but that's just another level of ugly imo.
The mismatched case style and {}s ... reeks of copy-pasting would be my first thought, definitively lacks attention to details like you mentioned.
Interesting discussion about #defines, I personally don't have an issue with it, but I'm programming micro-controllers so every little byte count, I'm more picky about variable types myself. Not a fan of having public class variables,.
Great Series, I'm learning a lot; Thank you!
Re #define memory usage, wouldn't it be the other way around? Say I have `#define MY_STRING "hello, world!"` and I use that in multiple places in my program. Since the preprocessor essentially does string replacement, doesn't that mean that after preprocessing, the program will have multiple occurences of the same string literal, which will mean the executable becomes larger? and the executable obviously has to be loaded into memory at runtime. Whereas with a const struct, there's only one copy of the string, and references to it. Am I totally off base here?
i have never been a fan of the `m_` coding stuff it means "member" but then `this.`, `this->`, `obj.`, `obj->` means member already, and then `()` means method no brackets means variable, i have no idea why `this->m_var` would be more clear than `this->var`. i do on the other hand see the use cases in ppl doing stuff like `this->sptr_Variable`, `this->uptr_Variable`, `this->ptr_Variable` to tell everyone this is a shared, unique or raw pointer from the name.
Hey Cherno, any chance you can upload the stream vods to youtube of you recreating your old game using raylib please? And possibly even the vod of you adding the web platform for it as I'd love to watch the longer versions of these to help with learning.
If it works, and the user is happy, its not a fail. if it also runs fast on low range hardware, its good code. No matter what others say.
That iostream include breezed through too fast. I don't know if it's still the case but back in gcc-4.x including iostream (which could happen through unwanted exceptions) allocated around 100kB of static objects to the binary. Just by including the header with no other interaction. Terrible for embedded.
C++11 gave us in-class member initializers and this is the first time I've ever seen them in the wild... and I've seen a lot of C++ code from a lot of places. I didn't think it was valid C++ at first, that's how rarely they come up. I had to check the spec, thinking it was something new I missed in C++23.
I've never truly understood why have a private variable when you have a public function that changes that variable. Seems like an extra step
Encapsulation
One of OOP principles - encapsulation. By making a variable private you can decide how it can be interacted in code.
For example your class has two numeric variables that always should stay proportional, or they cannot be equal to 0, you can implement that in setter method.
Or you need something to happen after value is retrieved, through getter.
That's about it
@@Username-bin Yeah and I totally get that, but then you see countless examples where the getters and setters do nothing but get and set the variable without any validation/transformation/etc..
@@pk10x When you have an invariant (a set of conditions that make an object valid, otherwise it's invalid) you use a setter. Ideally, your classes either have invariants for everything, or for nothing (so we call the class a "struct"). In the real world, you might end up with some member of the class needing invariants (so they need a setter), others don't. So, for the sake of code consistency, you put a setter on all the members.
Just having a setter that does nothing but changing the value of the member variable, that's an anti-pattern. And it provides no encapsulation either, as every private member ends up being exposed anyway.
I usually use them like this because it is easier later, if needed, to:
- Add logs on set/get
- Put a breakpoint to see who changes the value at a specific point
- Add whatever and not break ABI, for example, mutexes, if I decide it should be thread safe
37:29 the main usecase of a std::weak_ptr is to break cycles of shared_ptrs owning each other, there would be no advantage of using them here. Non-owning raw pointer parameters are perfectly fine if the function doesn't care about the lifetime of the object, but in this case a reference to an InputObserver might be the best solution, so if the caller has a shared_ptr they would only need to use the dereference operator on it when passing it to the function. This way the function wouldn't impose on the caller how the lifetime of the object should be managed, and it would avoid the overhead of using the smart pointer's copy constructor as well as any unwanted copying of the data. Dangling references are easy to avoid with some discipline, and they can be detected by clang-tidy anyway. The C++ Core Guidelines suggest the same thing if I remember correctly.
My OOP teacher used to say that if they see any public variable they would instantly fail you the test...
Your OOP teacher wasn't good enough, they should have focussed on when to use public members and when not instead of just banning them.
I am not *at all* an experienced C++ developer, so this comment is more for my benefit (to check my understanding against that of the more experienced commenters in here). The first thought that struck me as I saw the first bits of the code was that it seems almost like "I know this is for an application so I want to use as many fancy things, or things I consider to be fancy, as possible. So I'll make loads of 'special' pointers." It feels to me like they started off with ideas of which special things they wanted to add instead of just getting the game to work well. That's actually something I wonder about with experienced coders and optimisation: do you typically start off trying to optimise a lot of stuff or at first just try to get the concept to work and then start optimising and refactoring where you can? I imagine it's probably a bit of both, where the more experienced you are, the more optimised your first version of the code already probably is. Right?
Spoken like a true game dev: "you've gotta do this" while moving the bracket on the next line "or this if you're weird" while putting it back on the same line 🤣
It's a fact of life that we, in game dev, universally prefer Allman indenting style (bracket on next line) while open-source developers love the Linux Kernel indenting style with the bracket on same line. Some in open source even like horrors like this:
if (cond) {
// code
} else {
// more code
}
I'm not starting on the horrors which have zig-zag indentation.
My supreme game is your worst nightmare
5:45 I think this is the minimum level of pedantic that any C++ code should be. If you don't want to think about this kind of details, you shouldn't be using C++ at all.
'heap costs more to access than stack'? Allocation takes longer but access is the same, and a single allocation over the lifetime of the code costs 0. The dereference is moved outside the loop by all modern compilers as well.
Pretty sure stack is more likely to be in the cache in which case access would absolutely be faster. And I don't believe that's the only reason stack access is faster, it also can be optimized out easier, in some optimized cases the stack object can be completely removed by the compiler even in O1. (which won't happen to heap allocations unless the compiler can see through what you're doing which with heap it almost never does)
Heap objects have an additional layer of indirection, since you must first read the address off a stack variable before you know where to read from, while stack objects are a static offset from the stack base pointer. On repeat accesses this is often moot since the address is likely already loaded into a register, but that's far from guaranteed.
@@cookieshade197 Passing a heap or stack allocated object into another function is an indirection no matter how they were created. Within the function that created them it's going to be stored in a register address unless it's hideously complicated with enormous register pressure.
Try it for yourself on compiler explorer.
@@Spartan322 cache is going to be 64 bytes wide no matter what part of memory is being cached. I'm not sure if you're talking RVO (allocate on the caller's stack instead of yours) or something else with the second half, but stack allocated objects don't guarantee no heap, their initialization may well invoke heap allocators as well.
@@danmerillat The stack is always gonna be quicker to access regardless, aside from indirection, the stack is closer to the execution in memory. And I never said its guaranteed, but in most cases the stack is in the cache.
I had a lot of issues regarding this review.
Suggesting to use map, allocating the GAME class on the stack, complaining about defines when they are completely valid.
Saying there is no reason to have a vector of pointers?
There are some reasons, especially for callbacks for objects managed by another entity.
creating some data structure to manage each one of those will be wasteful and meaningless instead use raw pointers.
But I had more issues with the actual code.
I am an interviewer and I would've probably failed this code (except if applying for a junior position) because I would think that the writer does not understand stack allocation, type system, encapsulation or ownership. All of which I usually demand from senior C++ programmers.
Isnt the poster using the code for some kind of university exam?
Seems like it, anyways
@@uuu12343that's the impression I got, anyway. Anyone asks me to make them a complete program for me to be considered for a position, I'll just laugh and look somewhere else for work. If this was for a job interview, it clearly demonstrated the interviewer didn't value this person's time. For a university exam, failing it seems needlessly harsh unless the task description explicitly asks for something this code didn't achieve.
His code is fine, only thing that could make it better is if he dropped OOP. All of chernos criticism is just superficial, and your expectations for what makes a programmer a senior one are skewed ( yes knowing about stack allocation is important to know) but just because the author of the code put it on the heap, it doesn't mean he doesn't understand the stack, also the way someone is naming variables doesn't matter at all and cherno said it himself, so why focus on that at all.
If the syntax is what grabs your attention and not what the program actually does, then you're focusing on the wrong thing
Some criticism is valid, while others are not. Doing the make shared in the constructor initializer is detrimental to readability. Also, it is supposed that shared_ptr default constructor is lightweight.
Well, it doesnt have to be detrimental to readability. Normally, when I end up with long constructor initializer lists, I stack the member variables veritically and align them.
Having the application object (game) on the stack is not a good advice. That object lifetime is as long as the application itself, so there will be a reserved amount of stack for the whole application execution. There is no contract on that object size from the consumer perspective (main function in this case), so if the application object size grows (due to refactoring or extension of Game class), there is a risk of introducing a stack overflow. One way of avoiding heap allocation in this particular case is to use static, but unique_ptr is actually cleaner.
In the general case of local variables, the stack should be used when the size of the variable is known to be small to medium size, otherwise the heap is the better choice.
You could argue about every single type then, "it could grow the stack too much." Do you put your `std::string` objects into smart pointers too? Because these too, have no guarantee of size, now that everyone uses SSO. It's not like the author of the main function is unaware of what the App/Game class is.
12:30 My team lead will ding us on code review for not using initializer lists. We get dinged on code review all the time for any kind of unnecessary copy operation. As primarily a C# developer this concept took a lot of getting used to.
As the one doing a lot of code reviews at work, I tend to suggest just using a plugin like SonarLint (don't know what they support outside of VS and VS Code). Not too many annoying rules to disable, finds a lot of stupid mistakes and I occasionally learn new things (like why you should use std::less for maps with a string key).
It also makes life easier, since it flags pretty much everything I would point out in a review and getting that feedback as you write the code saves a roundtrip through another review.
I think that if someone wants to allocate memory for `Game` type on the stack, they need to be aware of that `Game` instance not to exceed that the stack memory limit provided by the c/c++ runtime.
For example, the runtime allow only 1mb of memory on the stack, but then if `Game` instance is 2mb then Stack overflow will occur.
If you compile and run in debug mode maybe there will be debug assertion failed message will pop up.
If you run in release mode..
Idk what will happen.
It won't because there's basically zero chance you'll have that much in that class to begin with.
@17:00 I appreciate this object separation, even if it is a rather simple program due to the fact that it really gives room for the program to grow and a much more easily understood manner.
I come from an embedded background. I was taught to basically make everything using defines or constexpr. Everything done during compile time is free, and everything done in runtime is expensive. Compute is cheap, memory is expensive
How much memory do you actually work with? we're not in 1980 anymore, even the smallest systems have MB of ram, and a few variables won't make a difference.
What will make a difference is being able to tune these variables immediately, instead of having to wait 5mn to recompile.
Attiny procesor still requires you to save as much memory as possible.
@@feandil666there's not enough memory on the target to make the compilation take 5 minutes on a decade old laptop
There are lots of.embedded processors with less than 1MB.
RISC-V MCU - CH32V307 for example is 64KB SRAM + 256KB Flash. Lots of MCU don't need 1MB
@@feandil666 In embedded systems you don't always have MBs of ram. Heck, I've written code for a device with 1k words of rom and 192 bytes of ram in this decade. That was an extreme case for me, admittedly, but it was still a case.
The issue with the next-line-same-line bracket typically happens when your style is same-line bracket, but the code generators and IDE features typically use next-line brackets. I run into that frequently myself, mainly when it's not changable
IDEs generally can format your code. Just run reformat on everything before submit?
i finished watching the video
Nice man, did you figure out why it failed?
yeah I'd like to know as well
Inconsistent code with mixture of styles. The key thing here is "as if part of the code has been written by someone else" )
I don't know, I wouldn't necessarily give fail to this code.
I looked through the whole code myself, and first thing came to my mind is that the code is designed in such a way to piss off the evaluator. It has too many unneccesary abstractions and meaningless files, which just drains the time for someone who have to evaluate heaps of projects from heaps of students.
But still, I would say author somewhat understands code architecture and coding in general, produced working program (I hope so, didn't tried to run it myself). I'm not sure if it was graded as F or it just was pass/fail type of test, maybe for pass/fail some people might give fail, but definitely not F level of code.
Edit: I thought it was university project, if it was job application, this definitely is fail.
In my university this would have been an easy pass, pretty much any working program passed.
If it was a university project and there was no indication as to why the project failed, then in my opinion the university failed. Should be considered passed because the project worked even if it had not been completed with top marks.
@@FlorianZier if my goal was to teach someone how to code, I'd turned this down, BUT with some clear hints on what to improve. This is too messy and clunky to deserve a pass
if you want to tweak magic numbers live but don't want to expose them as editable property for something as simple as a prototype, a good practice is to use static variables in your function, this way you can break in the debugger, change the value manually, and continue running.
This code makes my skin itch.