I Rewrote This Entire Main File // Code Review
ฝัง
- เผยแพร่เมื่อ 18 พ.ย. 2024
- Patreon ► / thecherno
Instagram ► / thecherno
Twitter ► / thecherno
Discord ► / discord
Hazel ► hazelengine.com
🕹️ Play the latest Hazel game FREE ► studiocherno.i...
Code ► github.com/ura...
Send an email to chernoreview@gmail.com with your source code, a brief explanation, and what you need help with/want me to review and you could be in the next episode of my Code Review series! Also let me know if you would like to remain anonymous.
🌏 Need web hosting? ► hostinger.com/...
💰 Links to stuff I use:
⌨ Keyboard ► geni.us/T2J7
🐭 Mouse ► geni.us/BuY7
💻 Monitors ► geni.us/wZFSwSK
Thumbnail facepalm icon created by Freepik - Flaticon.
![กี่หมื่นฟ้า | Your Sky Series EP.1 [2/4]](http://i.ytimg.com/vi/qJbQs_nhjD4/mqdefault.jpg)
Watch the rest of the Code Review for this project here: th-cam.com/video/B6pM9KcIFE4/w-d-xo.html
In terms of performance,
is the function runs faster if it's a bare function, rather than a function embedded in a class ?
If you use class maybe it's little bit slower because you later need to create a pointer for it.
In terms of size is it the same memory size usage or not. But, I think it's the same.
Can you share some use cases of `std::shared_pointer`.
I've seen much of `std::unique_ptr` as of now but shared pointer is kinda of rare use case.
@@abacaabaca8131 Unless it's a virtual function, the function call should not be noticeably slower. It basically gets an extra this pointer argument. Also note that a pointer contains just the address of the object, so that is pretty cheap to obtain; for global/static and stack objects, you don't even have to touch memory to obtain it, as the address of a global/static variable is known at compile time, and the address of the stack variable is calculated from the frame pointer, which is in a register.
Passing the this pointer to the member function also usually won't cause any extra memory access, as most calling conventions would pass it in registers. The only time you'd get an extra memory access because of it is if you have just so many arguments to the function that the extra this pointer makes it run out of registers and pushes one argument to the stack. But in that case, I'd argue that the function is likely so complex that the extra memory access doesn't matter (and also, the function probably needs some refactoring anyway).
Also, in the case here, the member functions were all inline, and therefore wouldn't even show up in the compiled code (unless you don't optimize).
Note that if the compiler knows the static type of the object, even a virtual function call can be fast, as the compiler can optimize away the virtual function call mechanism for that call, as the result is already known.
Merging the destructor and Clean makes sense, since a destructor is meant to clean up, and do whatever needs to be done before an object is deleted.
However, I wouldn't expect a constructor to also start the game.
yeah that also made me pause... constructor doing non-initialization related things (like run a whole game) sounds like an extremely surprising behavior
Depends. It's more common to have things start on construction (ie: std::thread) but those usually don't block.
game.run(); makes more sense (and imo thread.run(); also makes more sense).
Totally agree
Why you cannot do that ?
If you have several member method that needs to be called in a specific order, why bother the hassle calling those methods after instantiation of the class.
I've seen someone on TH-cam doing the same thing i.e calling member method in constructor albeit it is in python.
Is there any critical error / side effects with that approach ?
Unless the member method/s that is supposed to be called in constructor will return a value, then you have no option but to call it later (after instantiation).
@@abacaabaca8131 I would expect a constructor to finish very quickly, not block.
For calling multiple things in a specific order, wrap them in another function/method.
StartGame should not be part of the constructor. The constructor should bring the object into an initial valid state by allocating resources, setting default values, etc. and nothing more. A constructor should not be used to run "business logic" of an object.
I would say it’s contextual
I have seen a lot of popular application calling business functions on constructor.
It usually calls some function that jumpstarts some specific business logic
I completely agree. The pattern he ended up with read as if you were creating an object and doing nothing with it, not starting a blocking game loop.
yeah, i have that and then I call a run method to start the game
I think it was more intended as a gateway to show that you can discard the entire class, which he did.
@@edjemonkeys4896I think that was exactly the point, this is a lesson in the fact that you don't need objects everywhere.
bro literally made a constructor that blocks for the entire duration of the program
The moment he put a function call in a constructor - I knew the shit's gonna go down in the comments 🤣
I have seen professional famous code bases, made by programmers with decades of experience, putting function calls on constructors.
Some people in the programming circle, is to hellbent on rules and "good practices" and never stop to think, if they really matter and if they really help on making good fast software.
Actually, there is nothing inherently wrong in calling instance methods from inside constructors. If it was something illegal, than the compiler would not allow such code to be compiled. The actual constructor is called on the object which has already been fully allocated and it’s purpose it to bring it to the valid state for the user to use, arbitrary code can be executed from with it. The only reason why people say “it’s bad to call instance methods from within a constructor” is because the arbitrary instance method generally relies on the object to be in a fully initialized/valid state, which is not the case if you call such a method before you have finished setting up the object’s state in the constructor. But other than that it is perfectly valid to execute arbitrary code inside a constructor. The problem here I guess is that people don’t like the fact that the game is launched on the creation of a game object, because at the moment it is constructed it is immediately played, and then the actual object serves no purpose to the user, except that he has to call a Clean() method on it, which might as well be called by the constructor too. But at this point you no longer have an object, you just have a function😅
@@kostiapereguda The compiler allows you to create a vector of references too, while it is ill formed
@@nevemlaci2486 first of all, no, it doesn’t. And secondly, this doesn’t invalidate my point about constructors
@@kostiapereguda It doesn't let you create a classic array of them, but I'm pretty sure it did allow me to create an std::vector out of references
The more logic there is in a constructor, the more ways it can fail. Constructors can't easily return a value, so you have to use exceptions to report a failure or store an error code that you can retrieve later or use a reference parameter to return an error code. Also, not too sure about it… but having an exception in a constructor might leave you with a partially initialized object
Not really, exception invokes stack unwinding so if you don't explicitely mess with catching it, there is no possibility to access partially constructed object, but even then, catch scope is different scope than try scope.
Also it makes creating subclasses a headache. I feel like the ShooterGame class probably shouldn’t exist though so whatever.
I disagree with starting the game in a constructor. If you put StartGame() into the constructor, you may as well put the Clean() into the constructor too. But then, the whole point of an object is gone, and it can just be a static function. So, I would leave the initialization in the constructor (adding the scene to the game), and let the user of the object explicitly control when he wants to start the game with a separate instance method
That's because startGame() is executing the game loop and not just initializing stuff as the name should indicate. With proper naming like initGame() and additional runLoop() I don't think you would mind having initGame() in your constructor and then call runLoop() after the object is created. That point was made afterwards.
@@reup6943 the point that was made later in the video was quite different from what I said. The author’s point was that you don’t even need an object to start up and run game, you can use a function (which is essentially the same thing as I said at the beginning of my comment, so I agree with this). But my actual point wasn’t about this, my actual point was that if you are going to represent a game as an object, then you should also give the user a proper control of that object. Perhaps you want to launch the game from a different thread, or you want to pass the representation of that game around, store it somewhere without actually launching the run loop, perhaps you have a launcher that lets you pick from a set of games, or perhaps you have a load/save functionality that constructs and returns you an instance of the game object on load, whatever.
The reason I wrote this comment afterall, is because I’ve indeed seen real people writing code that launches a game loop in the constructor of a game object. Sure, it may be cool for a code like “new Game();” to actually launch the game, but i personally don’t think that this is the right way to use objects
Also, moving the object to the stack as he does in an intermediate state is not quite the same as the original code - the execution of the destructor/clean function is moved after the "pause" command, which means if your engine is reporting warnings or errors in the shutdown state it may not be visible to the user (assuming they execute the game from a gui rather than a command terminal). I like how he ends up with the static function RunGame() but his intermediate code is not quite the same as a refactor.
I think the heap allocation comes from the fact that they're a C# or Java programmer, since they do the whole
shooterGame game = new shooterGame;
And are just used to using new
"Everything in Java has to be inside a class" - I've made it my personal mission to do exactly the opposite, and have been quite successful in storing functions in enums, annotations, maps ... just for some extra spice.
Enums are objects in Java...
How the f did you put a function in an enum. It may be clear that I've used java the least
@@hamzaiqbal7178 The dark side is a pathway to many abilities some would consider... unnatural.
public enum Something {
Value(() -> System.out.println("hi"));
public final Runnable func;
Something(Runnable func){
this.func = runnable;
}
}
You're a danger to society
I understand the others, but enum what?
The "you don't need classes for everything" is such a good point. I just wrote a basic SDL engine and ended up using about 10 different singleton classes that all didn't *really* need to be classes at all - I just made them classes because I was coming from c# and that's just what you do over there.
Idk about the constructor starting the game, that feels really odd, especially when you then change it to be stack allocated. Maybe it's because I don't really do a lot of C++, but when I read:
int main() {
ShooterGame game;
}
I really so not expect anything to "run". It's fine that it pushes the ShooterScene on the engines vector, so that it is, in fact, a "ShooterGame". But actually running the game from a line of code that to me basically looks like a variable declaration seems really weird and unintuitive for me (and therefore hard to read).
But personally I would probably just put it in directly in main anyway myself haha.
As a non C++ programmer myself, I wouldn't even expect an object there, but just an empty typed variable.
So, no, I would definitely not expect this declaration to run anything.
But that is not a simple variable declaration, is a class object creation on the stack (does automatic cleanup) and a c++ programmer should know that, like he/her should also know that creating a class object, is calling the default constructor or other custom constructor that may have been defined, so as C++ programmers, they should have in mind that depending on who wrote the class, some functions could very well be called/run at object creation time. Is very common despite what some here claim to be "bad practice", to put function calls in a class constructor.
Also he never said that was what people should do, he only showed that is something you can do, very different things. And IMO a good way to teach what is happening behind the curtain when creation a c++ class object.
@@Argoon1981It isn't, no, but standards exist for a reason. Constructors should construct, not execute.
3:35 it actually is not the same, there can be a massive difference. When you use “default” or don’t define a destructor that means that the class is trivially destructible. However if you define the destructor EVEN if it’s empty the class is not trivially destructible anymore.
This can have quite a big impact on the compilers ability to optimize your code, it can affect how certain containers or templated types like std::optional will work with your class and much more like even the ability to memcpy the class.
Basically: follow the rule of 0. Don’t define a destructor if you don’t need to.
Man, loved the video!
Your code, style, approach and explanation are such a joy and truly evokes the most positive emotions. Thank you so much for what you do. I really miss working in a good company with good projects/codebase :)
The constructor starting the game is not good very self explanatory
For a game object like this generally I would keep that start game function. But I think the point he was trying to make was that since there is basically nothing in the class you might as well combine them. Honestly I wouldn't take an object oriented approach...
I completely agree that a class should not be used here. When I write code, the first thing I think of when it comes to the question of "should this be a class" is whether or not it is responsible for managing data. Will this object be managing data such as conditions or objects? If not, it is much more suitable to make functions to perform the task.
Although, if the logic performed in a function is only a few lines of code like it is here, and the code is not intended to be repeatedly executed, I would just write the code where the function would be called with a comment above it to specify its role.
Avoiding new and delete will probably be a debate lasting to the end of humanity... I know, "who asked for my opinion?", but I'd say avoid using it unless the object needs to be accessed outside of the scope, such as initializing a new object then returning a pointer (like a factory, in which case, the class that made the object should keep a record and delete it when required, lifting the burden of memory management from the caller).
I still need to learn smart pointers, it looks like they would really help with memory management.
Side note (and thanks if you're still reading), I see a lot of people complaining about starting the game inside the constructor. This really depends on the purpose of the method that starts the game, as Cherno states, it should be 'immediately terminated', and I agree. Starting the game should do nothing more than flip some application state which says that the game should now run - this takes almost 0 process time, and doing this in the constructor of this pointless class serves only to make the code cleaner.
Realistically (and without a class), the method to run the game should just be called immediately after intialization and loading the game, as I cannot possibly see a reason why a run method would be called without the intention of starting the game.
I need to edit again! Forgot to say this was a great video, people coding in C++ (including myself, I am not perfect at this by any means) need to learn the semantics of these core C++ constructs, and even if you're an experienced coder, I've been coding C++ since I was 11, we still can easily mess this up - self-review is important, never write code once, see if it works, then leave it: what you wrote may have a much better solution you didn't see at the time.
I enjoyed this video much more than previous ones. This one is just straight forward, precise, no out of context talking and opinions.
I love seeing classes like these get crammed into a single function. It's just so satisfying!
I love these videos, and I'm not a beginner, I''ve created my own game engines, other softwares, etc.
But it's great to watch these videos and trying to predict what you're gonna say.
It's kind of like a game trying to reach the end predicting everything you're gonna say, sometimes we forget something and we think "Oh God how could I've forgotten this?".
Please continue to do these videos, they are great. :)
And yeah, it's very irritating when someone uses classes and heap memory for everything, in C++ we're really free, my favorite language.
PS: You should have the unofficial C++ mascot plush there, it would look great in the videos, as sometimes you review some pretty unhealthy code and the mascot would fit perfectly, and you could exchange that mascot by a healthy rat plush at the end to show how the code got healthier. :)
Your code reviews are really fun to watch
If you create a user defined destructor (or = default), you should be following the rule of five, which is a really great incentive to not use a class if you don't have to.
That "I don't wanna talk about it" at the end sounded just right lol
Please continue reviewing this code! This type of video is very helpful for beginners
i've been following your videos since i started learning programming since then im a medior dev, and its so nice to see that in the beginning i was learning something new every video, now i was thinking almost the same stuff, and would refactor it in a very similar way :D you helped me improve a lot thanks for your work
Yeah, it's really interesting to hear your thoughts on creating a pointer in a function call. Please make the next part
Watching this makes me happy that I learnt Rust in the past year.
It just automatically makes you aware of all these issues and when you get the hang of it writing C++ will basically be a piece of cake!
This is such a typical code review it should go into a museum. Bike shedding a perfectly readable code, suggesting changes based on personal taste, never delving into the actual parts that define the functionality
Yep, it's a typical horrible code review with bad advice for beginners. I'm not a C++ dev, but I'm a senior dev in object oriented languages, and the original code looked perfectly fine to me. This guy should stick to giving feedback on code that's objectively bad.
It's frustrating getting your code reviewed by interviewers since their personal preferences always differs, which makes it impossible to satisfy them.
So refreshing to listen to you and basically agree with everything you said. That is usually not the case with me!
This is so valuable, I like the way how you talk and optimize it without any hesitation. Being direct and strict about practices and code style is a good thing IMO. Especially if you can argue why it is good/better. Would love to see more of this series
I like the approach to look at your code like you are shipping a library other devs are using. It often makes sense to just use pseudo code or stubs to get a feel for what is needed.
PdfTool pdf;
pdf.Load("path");
auto page = pdf.Pages.Add("P1");
page.AddText("hello");
pdf.Pages["P1"].AddText(" world");
page.Elements.RemoveAllByContent("hello");
and so on ... just from this little example, you can see, what classes you need and what the code flow for the user will be etc...
Great video! Always too short.
What people need to extract from this video is that you want to simplify the way you solve problems.
Because sure, in a simple file, all of this seems irrelevant, but if you add up bad practice upon bad practice in hundreds of files, this becomes a nightmare.
had absolutely no idea you could rename variables and have it update across an entire source. Ive been wanting to update some variable names but had no mind to dig through every source file and update the variable name. Wow, thanks cherno
Would love to see a continuation on this!
Annoying when you spend 5 minutes writing a comment and the TH-cam bot deletes it because of technical jargon, I'm assuming.
Could be because it saw a lot of unusual symbols and made a "better safe than sorry" assumption to try and avoid an unknown attack of some kind. Maybe. Some stuff that YT auto-deletes is just insane so it could just as easily be that someone at Google is offended by the word "template" or something lol
i love these style of videos, please do more :))
I enjoyed this, I'm glad you pointed out the law of demeter violation. Im also not a big fan that you have to add a scene before calling StartGame. If the engine requires an initial scene(s) it should require it via constructor instead of using temporal coupling.
I think the reason people use heap allocation everywhere with "new" and "delete" rather than relying on the stack is how C++ is taught in college. Until I took on learning more about C++ myself during university, I use to do this as well. Utilizing "new" and "delete" everywhere seems to be something the professors learnt from Java for some reason.
In regards to logic in constructor/destructors, you do you boo, but beware that calling virtual functions of the object in its constructor/destructor, it will not call any overridden versions of said functions due to the derived type not yet having been created, or in the case of a destructor, it's already been destroyed. Just one of those fun little quirks of C++ worth thinking of when adding logic to constructors/destructors.
should then be an abstract class for that?
These videos are very useful for learning C++.
I am learning lots. Thanks.
I really like static create functions where you do heap allocation for the object and then a destroy method that cleans up and deletes
I think the only change I would add on top of what you suggested is to just have the constructor initialize the engine and require calling a function to actually start something running. Otherwise, I agree with all the changes you suggested.
The second I saw std::vector my heart skipped a beat, kinda like a car pulls out in front of me on the highway or something.
The last thing about handling pointers, that's a cliffhanger, would love to see a video on that as well. Guess that'd cover ownership aspect
10:50 Actually that was what I was doing for years in C++, but with time, by also using other languages, I got obsessed by adding classes/structs for everything, even in C++, Idk I just think it's cleaner and more organized. But I agree, in the case showed in this video, it make sense not using a class.
One thing I found weird particularly about this code though is how this main is structured. I mean in pratically every cases when creating a game (or every app in general) you will want to do something like this:
int main()
{
Game game;
while (game.IsRunning())
{
game.Update();
}
game.Stop();
}
I mean the way this main is done is weird to me, you generally want to declare an object "Game" or "Application", check if it's running, update loop, and then call a Stop() fonction (or just add everything in the destructor of the class, but I usually prefer to have a "Close" or "Stop" function, it make things more clear and cleaner.
I'm very new to C++ but the first thing that I didn't understand was the destructor on the first line of the class. Anyway thanks Cherno for the video, every time it's like a back to basics for me.
I might still not know anything but this video really useful so I can organize better, thank you
There's nothing better than seeing Cherno shit on code that you're guilty of yourself. Incredibly informative as always and thank you to the person who contributed.
As a linux user I was today years old when i learned that windows has a distinction between "console" and "windows" apps lmao 🤡
How do you create GUI applications on Linux? Do you write, say, all Qt’s boilerplate every time from the scratch?
that's not a bad thing though
equivalent to windows API would be Xorg or Wayland API - these are "providers" for linux user interface. Though they are not interchangable. So alternatively, you use FLTK, Raylib (initially made for games, but can be used for UI), GTK and Qt, there is a variety of optionz
I can't edit a comment for some reason, but i'll add that fltk, gtk and raylib can also be used on windows, mac e.t.c..
@@konstantinsotov6251 right, and if one uses QT Creator or GNOME Builder, then, for a new project, one needs to choose between “window” or “console” application. For the latter, a lot of unnecessary boilerplate will not be generated.
5:10, this makes sense if the project is too short and simple. But in games we have the new game option, which is better to have a f() for that, instead of constructor, which would demand to rebuild the entire object.
i never get the reason for deallocating memory (cleanup) RIGHT before your program exits.
just exit the program without cleanup, this is not only operating systems job, this also makes your program exit faster
edit: nvm he says this in the video
If you have a software with multiple threads and those threads eventually depend on each other, just exiting your program might end up in a mess. It might look to you as your software exists "normally", but in reality, especially with aforementioned dependencies, you can cause "internal crashes", to phrase it in simple terms. So, for example by popularity, Windows attempts to shutdown your threads, they hang, the shutdown hangs, eventually up to 30 seconds later, the software is closed. Internally it (the system) would call some exception handling to get things untangled and hopefully freed correctly. For you, depending on your compiler and compiler settings (release build and debug build can have sometimes really big differences in this type of behavior), you might see absolutely nothing, besides the 30 seconds of silence, or you see a crash report.
I know a lot of people who shrugged it off with "Software is closed, why should I care?" I mean, imagine your threads, which might still be up, still work(ed) on resources, which are not properly closed or they might lose access to instances, in the middle of their routine, because the instance's destructor was called "randomly". That's bound to create data loss or corruption at some unlucky point. You surely don't want that to happen.
@@leviathanx0815 yea, i don't like multi threating, its hard and i don't understand it
there is at least one usecase for cleaning up memory when exiting the program: if you're using address sanitizer it wont give you a bunch of false positives
I learnt a lot! thanks for uploading
Cherno Code Review Cheat Sheet:
- See "new";
- Rewrite file;
- Don't look at anything else.
Yeah half of what all is said is almost just personal preference…was the earlier code ugly…not very much except missing some constructor, destructors, and having a clearcut API segregation…
13:50 I immediately know that Cherno would not like Zig because of this reason.
I would argue that in certain performance situation the caller should be aware of what the underlying data structures they are using (but of course not in the situation shown in the video.)
I always thought that performing significant class functionality inside the constructor was, if not technically problematic, perhaps a bit obscure. It also doesn't feel "self-documenting" to any reviewer because they're not expecting the constructor to be the main thread of execution. Maybe I'm just old-school, but I think the stack construction, followed by game.run() in the main function would be relatively clean and moderately self-descriptive as to intent.
This is why IMO being mind locked deep into OOP is not good, it conditions people minds into thinking everything should be a class. And this many times causes code to be way more complex, harder to read and at times way slower.
Doing pure functions has its problems but it also has advantages and IMO OOP C++ programmers, shouldn't be afraid of using some C functional programming ideas.
Return values in main might have implications for scripts used by build farms to check if the runtime terminated without error .
I think I also have seen compiler warnings in cases where functions are declared to return a value but don't. Maybe this is ignored for main().
I wouldn't start in the constructor, but I'm not dogmatically against it.
All good, but I would not stack allocate Engine. A non trivial engine might be bigger in memory than the tiny stack.
Releasing resources has more implications than just memory; if you're holding onto file handles, create temp files that need to be cleaned up, etc then it does matter whether or not you're explicitly destroying those. I think it's better to encourage clean-up hygiene than to handwave it as unneeded even if it's generally a non-issue most of the time... similar to multithreading, most of the time it'll work fine, but then that one edge case that could have been prevented by proper coding hygiene turns into a month long debug-fest.
The other comments on error handling & control flow being difficult in constructors is also good feedback, though I think in this case (class acting as entry point for the game) it's a non-issue. Probably worth a note on when it's preferable to use initializers vs constructors when doing this type of refactor.
Actually, since Java 21 it's not necessary to write the whole "public static void" in "public static void main" one can just get away with writing "void main"
I feel like you should record a "standard disclaimer" and link it at the beginning of every CR - something along the lines of: "If you aren't a trained programmer and you've managed to write an application that works, it's already an amazing deed and you already are an amazing human being. That said, code reviewing is a procedure coming from enterprise environments that's supposed to bring up your code on par with the company standard or, if you're lucky to have such talented peers, to help you progress as a developer to an even higher standard, therefore, it is mostly criticism, but given in good faith and without any prejudice. It's not supposed to put you down - it's supposed to widen your thinking horizon and to make you both: more understanding of the inner workings of your code and better at seeing some issues coming at you from far away, so that you could avoid them early and save yourself a headache at later point". Because, man, we do get off-putting to the newbies when presenting our code religion. 😆
7:00 stack allocations aren't always better: if you're allocating big objects, it's often better to have it placed outside of the stack e.g. using the static keyword than to allocate them on the stack directly, since it can lead to a stack overflow if your function call graph goes deep enough and you have a lot of big objects on the stack.
in this case it's not that bad, since it's just 40 bytes, but a 4kB massive blob (and a GameEngine object could as well weigh that much) would be better put in static storage
Please make more videos on this code review. This video was very informative. I'd like to see your approach to the replacement for the parameter of AddScene()
Ocd hitting cherno hard😂🤣 (pls bring back ray tracing series)
This is not ocd, this is senior developer reviewing junior code.
Cherno humbly said all of this was his opinion, but I think it's more good practice coming from experience, than opinion.
@@TheAxeForgetsTheTreeRemembersActually those stuff is expected to be known from Junior engineer. This code is more of high schooler.
@@TheAxeForgetsTheTreeRemembersbesides ocd isn't just being "neat", it's SO much more than that, it's a genuine mental illness with many different ways it can present.
Everyone arguing if the function call should be in the constructor or not is missing the obvious. The ShootingGame class shouldn't even exist. It's just a wrapper for the engine object. There's no need to even have a class there. You just have a non-member function such push_back the ShooterScene, StartGame() and then Clean(). That would dramatically clean up the code.
Edit: ff to the end that's what he ended up doing. So why are people in the comments arguing about this intermediate change? Did they not make it to the end of the video?
I agree with moving ShooterGame::Clean() to the destructor, but I strongly disagree with moving ShooterGame::StartGame() to the constructor. The reason being that, I assume, StartGame() can fail, which would mean the constructor can fail, and constructors should never contain conditions that fail in a non-recoverable way, if avoidable. Granted, in this case it's not a big deal because it's the main class of the program, and any failure just results in an exit condition anyway, but in larger projects where a class failing to do it's job is not necessarily fatal, it could be an issue. Having failure conditions in constructors is a bad habit to get into. That's just my two cents.
If you're using Visual Studio and you don't want the console to immediately close, don't do anything in your code, just change Tools -> Options -> Debugging -> Automatically close the console window when debugging stops. EZ
Would love to see a video about pointers as you suggested at the end.
Constructors and destructors aren't a benefit of OOP, they're a benefit of the known lifetimes in C++. You can't use RAII in, for example, Python, because objects are cleaned up whenever the runtime feels like it, and you need to use a context manager instead.
Remarkable how that four letter initialism doesn't relate to cleanup time.
Yes please to the video on your feelings about pointers
Few people would be interested about non gaming country turning into gaming country with a very first shot missile, but it's about to be discussed.
I actually prefer the classless approach for some tasks even in language like C# and Java. It's just cleaner and less complicated in the end
I once began turning a console project into a Windows project. I switched main to wWinMain during the transfer process, and just to make sure things were working correctly, I ran the project. It compiled nice and fine, but as soon as it ran, my antivirus flagged the executable as suspicious and prevented it from running. Once I changed it to actually spawn a window, the antivirus was okay with everything going forward. I suspect that antivirus software treats any software that uses WinMain but doesn't spawn a window as a virus.
Hi! I recently started to watch your "Code Review" playlist and I love it. I am learning a lot, even if I am a programmer with some years of experience.
but.... I got one question:
in many projects, the "application" variable ( in this video the ShotterGame class ) is initialized as a pointer, a heap allocation object:
- - - ShooterGame* game = new ShooterGame();
and your advice is always to make it a stack allocated object... like
- - - ShooterGame game;
Then... when you show the code for your Hazel game... you are also doing the same
- - - Application* app = CreateApplication(argc, argv);
is there a reason for this?
Probably because this game is a small game we are talking about some bytes his Application class is probably more mb's
I do think it can make sense to have it in a class, obviously not if your main function literally just creates the object and then does nothing else with it and then the main function ends like in this case, but if you wanted to expose this class for use in other programs, they might wanna manage the state of one game instance by having a game variable and then start and end (or restart, pause etc..) it when they like, might wanna make multiple instances, etc. And I don't think the constructor should automatically start the game.
Others have already provided valuable insight regarding the logic behind why constructors shouln't handle business logic. I'd add that there is no reason to use hungarian notation. It is redundant.
"there is no need for new and delete here"
at the same time Engine class contains 1 gigabyte fixed size array
RIP
this makes me feel like i need to learn more keyboard shortcuts, even tho i know bunch, but i aint that fast
Deletes Clean to make code cleaner. Commit message: Cleaned Clean
I hope some of my Jr engineers give this a look.
Huh?
@@stiliok let those gears turn buddy.
I am using Visual Studio Code (I don't write C++ so that's why not VS itself) but I really like your color highlighting, what theme do you use in Visual Studio?
I want more code review series videos
my homie cherno spittin' straight facts here no capp
if you remove return 0 from main, does it always return 0 or what does the program return?
If you don't return anything from main then the compiler will effectively insert a "return 0;" for you.
There are a bunch of special rules like that that only apply to main and no other function.
@@superscatboy Oh I see, thank you for clarifying. I know that it returns an undefined value when you don't but the return in other functions.
@@JannisAdmek From the c++ iso draft:
[basic.start.main#5]
"If control flows off the end of the compound-statement of main, the effect is equivalent to a return with operand 0"
I could watch refactoring for hours
What visual studio theme are you using man? It looks amazing
13:00 The distinction you're drawing between "StartGame" and "RunGame" is called in linguistics "telicity." StartGame is telic, because it has a definite end point after which it is done, and RunGame is atelic because it doesn't. When is the game done starting? When it's started. When is the game done running? Not until it's told to stop. If you want less obscure terms, you might call StartGame a momentaneous action (one that takes place "in a moment") and RunGame a durative action (one that takes place over a long duration).
14:40 I am trying to wrap my head around the audio engine in hazel and there are a bunch of vectors with raw pointers to SoundObjects. They might be RefCounted though, I don’t really remember
The early factoring in this video was weird. It starts getting better near the introduction of "RunGame".
- the constructor only allocates memory and brings the object to a consistent initial state (everyone got that one). _No,_ you don't run an entire application session from within one constructor. Imagine all the open files, various open streams, and runtime error-catching living inside that lifespan? Terrible practice. Impossible to test with mocking, hard dependency injection because bad inversion of responsibility, the list goes on
- "engine.Clean" should indeed be immediately after "engine.StartGame", in the same function. Not in the destructor. We can assume that "engine.Clean" closes all open streams for example.
- Unlike Cherno, I keep the Game class because I'm assuming that "engine" is a subsystem of it and in a real-life project there are other subsystems to start (logging?)
- no destructor
Constructors should never do anything other than setting up state and initializing member variables. It would be poor form to run your game from the constructor. If you need to do anything else, then it really needs a good reason as to why you're doing it in the constructor.
Also, TheCherno should not just assume you can put engine.Clean() in the destructor the way he did, without a full inspection of the code. I understand that this game is poorly coded, but it should at least get a mention. Exceptions that are thrown from a destructor can never be caught outside of the destructor and your program WILL crash. You just need to handle them within the destructor.
I agree with most of the comments, including the changed behavior of moving the destruction point with RAII) but I believe the (intermediate) advice to move startGame inside the constructor is bad advice, especially if the class is used for something else, and is used for inheritance and might contain (in future) virtual functions that are overridden by other functions.
The risk here is that the virtual functions became counterintuitive because "startGame" is called before the class is completely built, and the object might not be fully initialized (e.g. a subclass won't be).
If you want to prevent that, the class should be marked final (but yeah, you removed it).
Thank you for today's stream, I enjoyed CLion all the way)
An app set with subsystem Console doesn't even need system pause, VS will automatically make the window persist, but people keep writing system pause for some reason. It baffles me.
You should not start a game from within a constructor. The constructor should be used for initialization-related stuff.
If there is an exception thrown from the constructor, you have to clean them there as well, as there will be no object to call its destructor later. And the delete method will fail.
starting game in constructor is crazy
None of this is so untidy that it matters either way, especially so close to the entry point where you're not going to end up with a memory leak.. Perhaps the change from StartGame to RunGame might add clarity. Changing things for preference is a bad idea. You can get away with it on smaller projects.
Great review
Love this!
8:33 On certain platforms it's not true, like microcontrollers, where there is no operating system, so it's better to clean up explicitly for portability
did he mention certain platforms? I thought he specifically talked about operating systems?
Is it really a problem in that case? If you don't have an operating system, then I suppose there is no one who manages ownership except what you do within your progrram, and so you cannot "free" your program's memory anyway, because no one else can claim it. If we're talking about cleaning up the memory, my understanding is that freeing memory does nothing to clean it - i.e. the memory bits are not set to 0 or anything like that. Except, I suppose, if you do that in your own delete operator implementation.
Usually a microcontroller only runs a single app. So yeah, this device only runs the damn game. When you quit, the machine has nothing left to do. So freeing memory shouldn't matter.
Usually with a microcontroller, you do not ever want to hit the end of the main function and if you do, then you need to go back to your design and rewrite it. What you find is that after the return of main, the destructor code is run and then it hangs indefinitely. It is possible, and I have seen it on STM32 micros that interrupts are not disabled so that if you don't clean up properly, your ISR could still trigger and cause problems.
However, speaking more to operating system applications, I would always, again ALWAYS try to clean up before you exit your application.
1. Your resources might need to be released in a specific way, closing a file might satisfy the operating system but might be problematic for your specific circumstance.
2. Some resources that are shared, i.e. you might have exclusivity while your program runs and if you change this, you better change this back to how you were given it because the OS isn't going to do this for you. Example is the TTY state in Linux. If I change the baud rate of a serial port from 9600 to 115200 in my program, I need to set this back to 9600 when I terminate because there may be other software that relies on this 9600 rate.
3. It is just good housekeeping. That is, don't rely on others to clean up your mess.
He specifically talked about it being a console game, where that cleanup is typically important to do things like restore character echo, turn raw input off, and switch to text instead of graphic characters.