I Rewrote This Entire Main File // Code Review
2024 ж. 19 Сәу.
77 374 Рет қаралды
Patreon ► / thecherno
Instagram ► / thecherno
Twitter ► / thecherno
Discord ► / discord
Hazel ► hazelengine.com
🕹️ Play the latest Hazel game FREE ► studiocherno.itch.io/portal-m...
Code ► github.com/ural89/ConsoleCraf...
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/cherno
💰 Links to stuff I use:
⌨ Keyboard ► geni.us/T2J7
🐭 Mouse ► geni.us/BuY7
💻 Monitors ► geni.us/wZFSwSK
Thumbnail facepalm icon created by Freepik - Flaticon.
Watch the rest of the Code Review for this project here: kzhead.info/sun/dZqpfZ2Dmnt8eGw/bejne.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.
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 KZhead 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.
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
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
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 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
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...
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.
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 :)
Your code reviews are really fun to watch
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. :)
I enjoyed this video much more than previous ones. This one is just straight forward, precise, no out of context talking and opinions.
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
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()
Yeah, it's really interesting to hear your thoughts on creating a pointer in a function call. Please make the next part
Please continue reviewing this code! This type of video is very helpful for beginners
"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...
i love these style of videos, please do more :))
Would love to see a continuation on this!
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
I learnt a lot! thanks for uploading
I am learning lots. Thanks.
That "I don't wanna talk about it" at the end sounded just right lol
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.
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.
As a linux user I was today years old when i learned that windows has a distinction between "console" and "windows" apps lmao 🤡
I love seeing classes like these get crammed into a single function. It's just so satisfying!
Great review
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.
Love this!
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
I really like static create functions where you do heap allocation for the object and then a destroy method that cleans up and deletes
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!
I might still not know anything but this video really useful so I can organize better, thank you
These videos are very useful for learning C++.
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.
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().
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.
Would love to see a video about pointers as you suggested at the end.
What visual studio theme are you using man? It looks amazing
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.
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"
That was useful, thanks
More of these pls
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. 😆
I want more code review series videos
great one!
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.
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.
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
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
this makes me feel like i need to learn more keyboard shortcuts, even tho i know bunch, but i aint that fast
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.)
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.
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.
@@romainvincent7346Actually those stuff is expected to be known from Junior engineer. This code is more of high schooler.
@@romainvincent7346besides 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.
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.
Yes, "pause" command doesn't exist on Linux
1:31 as much as I know it is used on Windows only. For Linux you have to use wait -p or something like that
Thank you for today's stream, I enjoyed CLion all the way)
Nice! 👍
It does matter what you return from main. The return from main is the exit code, and if you omit return 0, it does not just do a return default(int) or whatever, it does not return at all - this is UB.
Wrong. The standard says that no return in the main function is the same as return 0. You just learned something new, bud
@@sebastianschweigert7117 maybe link the source for me to read?
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 could watch refactoring for hours
I very much like your color theme: what is it called?
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.
Ah, the poison that is the "OO mindset."
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.
I wouldn't start in the constructor, but I'm not dogmatically against it.
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.
Cherno goes OCD - A documentary 😂
What theme do you use can you tel me please
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.
Hi Cherno and everyone! I want to buy a new laptop for programming... what would you recommend, a windows laptop with an intel processor and nvidia graphics card or a macbook pro with m3 chip? I want something versatile and powerful to build different kinds of apps
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.
15:59 that's good idea!
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.
All good, but I would not stack allocate Engine. A non trivial engine might be bigger in memory than the tiny stack.
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.
Hey Cherno. Im working on my own 2D game engine which uses some cool libraries. I would love you to review my engine when it’s done!
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.
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
Is it even necessary to keep the clean function on the Engine? probably the destructor would just fit equally here as before
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.
If the constructor does not start the game you can use the class in tests much easier without mocking. I would not expect this behavior of starting the game. However I don’t see the need for a game class…
Deletes Clean to make code cleaner. Commit message: Cleaned Clean
Do we want more videos like this? How come a question like this even exists? :)
I hope some of my Jr engineers give this a look.
Huh?
@@sti3167 let those gears turn buddy.
how the semantics is in my engine, is a raw pointer means that it is not owned. i use std::unique_ptr when it is owned.
This is the way. Raw pointers for ownership is always an error.
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.
Can I send you vulkan hardware ray tracer for code review?
That would be really interesting! Although he may not show it until after his ray tracing series. At the same time, it might also be suitable for showing alternative paths for possible implementations or providing ideas about which parts are particularly difficult and which Cherno could shed more light on.
This "don't tell the c++ community" is so funny!
what color scheme is this?
Excellent, excellent, excellent.
game->StartGame and game->Clean looks like something a C developer who is learning C++ would do because in C you have to manually call constructors and destructors lol
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…
But there's something you're missing when you start the game from a constructor. A constructor has no return type and if you wanted to return an error, you'd need to throw and catch an exception. Using a function would allow you to return an error code. In this code it didn't matter as there's no return code, but still. It's a valid reason for not doing stuff in a constructor when you should be avoiding exceptions. Also since it's only a pointer to the constructed class, wouldn't unique_ptr be better?
Did you not make it to the end of the video? He removes the class entirely
@@sebastianschweigert7117 I hadn't finished watching the video when I made the comment. That initial scene creation wouldn't have been in that class either, but part of the gangs startup, in my opinion.
Does anybody know what color theme cherno is using?
pretty sure it's visual assist
@@dysrhythmic Thanks!
Can U make a code review of Godot?
psv main in new java can be outside of class