My 1 month projects - asking for tips and ways to improve
*I posted this 2days ago in begginers topic but i got no comments or attention so i thought maybe it will fit here more i know im asking alot here but i will appriciate this help*
Hey guys,
I've started learning and programming C++ 1 month ago.
I had some Web and batch scripts background when i started programming c++.
In the last month i made some C++ projects on consoole and last week i started
my graphical journey with SFML and i made 2 projects (TicTacToe,game i call JetWar)
all the resources i used was taken from the internet (Sprites,Sounds) with a little bit editing them for my needs(Size,Trasnparcy& such)
I uploaded my projects to Github and ill share a link below i just wanted to ask if you may review my code and tell me where i can get better what am i doing wrong because the programs are running right and clean but maybe i did some coding wrong or in a different way that takes alot of time to write or basic mistakes i should have write different.
please be honest and tell me what i need to learn more cause this is the way i can improve my self and become game developer.
Project chronologiacl order:
1.TicTacToe - console
2.BankSystem - console
3.Hangman - console
4.TicTacToe - SFML / Graphic engine
5.JetWar - SFML / Graphic Engine
*NOTE: there may be unrelated lines / comments in the code because i tested different way to do actions and methods that i forgot to delete.
Github:
https://github.com/SeanR11?tab=repositories
I know it is alot to ask so
Thank you all, appreciate your help
have a nice day :)
Hey guys,
I've started learning and programming C++ 1 month ago.
I had some Web and batch scripts background when i started programming c++.
In the last month i made some C++ projects on consoole and last week i started
my graphical journey with SFML and i made 2 projects (TicTacToe,game i call JetWar)
all the resources i used was taken from the internet (Sprites,Sounds) with a little bit editing them for my needs(Size,Trasnparcy& such)
I uploaded my projects to Github and ill share a link below i just wanted to ask if you may review my code and tell me where i can get better what am i doing wrong because the programs are running right and clean but maybe i did some coding wrong or in a different way that takes alot of time to write or basic mistakes i should have write different.
please be honest and tell me what i need to learn more cause this is the way i can improve my self and become game developer.
Project chronologiacl order:
1.TicTacToe - console
2.BankSystem - console
3.Hangman - console
4.TicTacToe - SFML / Graphic engine
5.JetWar - SFML / Graphic Engine
*NOTE: there may be unrelated lines / comments in the code because i tested different way to do actions and methods that i forgot to delete.
Github:
https://github.com/SeanR11?tab=repositories
I know it is alot to ask so
Thank you all, appreciate your help
have a nice day :)
Last edited on
I have only looked at TicTacToe console (I don't have a lot of time).
There are some thoughts.
1. DON'T EVER* define functions in a .h file. The functions should be declared in a header file and defined in another source file. The compiled objects for all of the source files are then lilnked together to form an executable.
* Inline functions can be defined in a .h file. Template functions are not true functions and MUST be defined in a .h file.
2. A blank line between definitions of functions makes your code more readable.
3. Pay attention to indentation. In TicTacToe.cpp, lines 288-298 should line up with line 287. Worse, in lines 488 - 520, an if block is indented too much making it difficult to see where the closing braces belong. I am still not sure that line 520 closes line 490 because indentation doesn't line up properly.
4. Lines 418 - 420 and lines 452 - 454 seem wrong. In appears that line 452 loops through 'w' while inside the line 418 loop through 'w' (again, indentation makes this hard to verify). I think lines 419-420 and lines 453-454 might be clearer as their own functions. And then you can figure out when the loop through w should occur. This area just need to be cleaned up a bit.
There are some thoughts.
1. DON'T EVER* define functions in a .h file. The functions should be declared in a header file and defined in another source file. The compiled objects for all of the source files are then lilnked together to form an executable.
* Inline functions can be defined in a .h file. Template functions are not true functions and MUST be defined in a .h file.
2. A blank line between definitions of functions makes your code more readable.
3. Pay attention to indentation. In TicTacToe.cpp, lines 288-298 should line up with line 287. Worse, in lines 488 - 520, an if block is indented too much making it difficult to see where the closing braces belong. I am still not sure that line 520 closes line 490 because indentation doesn't line up properly.
4. Lines 418 - 420 and lines 452 - 454 seem wrong. In appears that line 452 loops through 'w' while inside the line 418 loop through 'w' (again, indentation makes this hard to verify). I think lines 419-420 and lines 453-454 might be clearer as their own functions. And then you can figure out when the loop through w should occur. This area just need to be cleaned up a bit.
Last edited on
To add to the above:
5. Since this is C++, avoid #define, prefer constant variables instead: Instead of #define UP 72, const int UP = 72;
6. The function main() must be defined to return an int.
7. Avoid non-const global variables, pass the variables to and from the functions that require them.
8. Avoid having multiple statements on one line. Each statement should be on it's own line.
9. Some of your functions are a little long and complex, IMO. Try to keep your functions small, split large functions into smaller functions. Normally try to have your functions doing one thing extremely well.
10. Since you're using Windows API calls avoid using the conio functions, use API calls instead.
11. You have a lot of "magic numbers" strewn thought your code, think about using named constants (with meaningful names) instead.
Edit:
12. Try to keep your program logic separated from your user interface.
5. Since this is C++, avoid #define, prefer constant variables instead: Instead of #define UP 72, const int UP = 72;
6. The function main() must be defined to return an int.
7. Avoid non-const global variables, pass the variables to and from the functions that require them.
8. Avoid having multiple statements on one line. Each statement should be on it's own line.
9. Some of your functions are a little long and complex, IMO. Try to keep your functions small, split large functions into smaller functions. Normally try to have your functions doing one thing extremely well.
10. Since you're using Windows API calls avoid using the conio functions, use API calls instead.
11. You have a lot of "magic numbers" strewn thought your code, think about using named constants (with meaningful names) instead.
Edit:
12. Try to keep your program logic separated from your user interface.
Last edited on
8. Avoid having multiple statements on one line. Each statement should be on it's own line. |
This one has minor exceptions, at least to me.
I personally prefer tiny things to be lumped up, eg
else if (cond) //2 things on one line
or
if (simple) smallaction();
etc …
the key is its for tiny things. It has to be equal or better readability all on one line, which is a judgement call. Use combined lines sparingly; the only place I use them consistently is for conditional blocks, as above.
Last edited on
First of all
thank you: jonnin, jib,doug4
i appreciate your help and time spent on me.
I will use your advices on my next on projects.
last request if you can take a look on JetWar project this is the last project i made and by that it means that it should be my best project cause every project i have learned some.
So i would really appreciate to hear your good or bad notes on my JetWar project.
ReLinked: https://github.com/SeanR11/JetWar
Thank you all, have a nice day, Sean
thank you: jonnin, jib,doug4
i appreciate your help and time spent on me.
I will use your advices on my next on projects.
last request if you can take a look on JetWar project this is the last project i made and by that it means that it should be my best project cause every project i have learned some.
So i would really appreciate to hear your good or bad notes on my JetWar project.
ReLinked: https://github.com/SeanR11/JetWar
Thank you all, have a nice day, Sean
I will but I can't go to file shares @ work, we are blocked from them and I value my job too much to try to bypass it.
Generally, I think your code is pretty good. I didn't do a deep dive, but read through a bunch of it.
I do have a few issues.
You need to learn to add comments to your code. What seems intuitive to you is not intuitive to people you ask to review your code (like us). It won't seem intuitive to you in a year after you put this aside and come back to it. Spend a few hours adding comments to explain why you are doing things. (For instance, in StateManager, what are _isReplacing, _isRemoving and _isAdding and why do you need them. That was confusing on a first read.)
I hate gameDataRef. You are giving almost every object in your game access to all of the data used by everybody else. This violates the principles of encapsulation and information hiding. You essentially have a large global data structure. The data used by each class should be owned by the class.
Why do your State subclasses have a constructor and then an Init function? I never found where these classes are instantiated, but if you call Init immediately after calling the constructor, you are missing the point of constructors.
In your State subclasses, your Init functions are doing a LOT of initializing of the contained objects. There may not be a way around this, but consider wrapping Text and Sprite in your own classes to allow them to handle their own construction. (Again, this is just something to weigh. It may not make any sense at all in the real world.)
I think it is a good habit to get into to declare each member variable on its own line rather than multiple declarations on the same line. You will eventually run into issues where you want to change the type of one or comment one out temporarily. And if you start mixing pointers and references, it's fairly easy to mess yourself up if you're not careful. It simplifies things to just declare one member per line.
I think it is also a good habit to get into to wrap "if" blocks inside curly braces, even if it only a single line. Consider Player.cpp, line 138. Somewhere down the road you decided to print a log statement before the return to help debug. If you forget to add the curly braces, you will always return true, breaking your code. Get into the habit of wrapping your if and else blocks in {}. (If you do
This looks like a lot. Overall your code looked pretty good.
I do have a few issues.
You need to learn to add comments to your code. What seems intuitive to you is not intuitive to people you ask to review your code (like us). It won't seem intuitive to you in a year after you put this aside and come back to it. Spend a few hours adding comments to explain why you are doing things. (For instance, in StateManager, what are _isReplacing, _isRemoving and _isAdding and why do you need them. That was confusing on a first read.)
I hate gameDataRef. You are giving almost every object in your game access to all of the data used by everybody else. This violates the principles of encapsulation and information hiding. You essentially have a large global data structure. The data used by each class should be owned by the class.
Why do your State subclasses have a constructor and then an Init function? I never found where these classes are instantiated, but if you call Init immediately after calling the constructor, you are missing the point of constructors.
In your State subclasses, your Init functions are doing a LOT of initializing of the contained objects. There may not be a way around this, but consider wrapping Text and Sprite in your own classes to allow them to handle their own construction. (Again, this is just something to weigh. It may not make any sense at all in the real world.)
I think it is a good habit to get into to declare each member variable on its own line rather than multiple declarations on the same line. You will eventually run into issues where you want to change the type of one or comment one out temporarily. And if you start mixing pointers and references, it's fairly easy to mess yourself up if you're not careful. It simplifies things to just declare one member per line.
I think it is also a good habit to get into to wrap "if" blocks inside curly braces, even if it only a single line. Consider Player.cpp, line 138. Somewhere down the road you decided to print a log statement before the return to help debug. If you forget to add the curly braces, you will always return true, breaking your code. Get into the habit of wrapping your if and else blocks in {}. (If you do
if() code; on a single line, you can probably get away without the curlies.)This looks like a lot. Overall your code looked pretty good.
Ty doug4 appriciate it, will take all notes to my next projects
have a nice day , Sean
have a nice day , Sean
Im looking at jetwar now.
some things I see that I would change.
Highscores.cpp -- this this this this this... classes can see their members by default. You should only need a this pointer once in a while -- its a tool that we MUST have, but you don't need it on every line. Its unreadable.
if (backgroundCounter >= 1500) -- magic number. wtf is 1500? give it a name in a constant. tell me what it is in the code by its name.
Draw(float dt) -- floats are not used much unless you have an exceptional reason to do so. They take up less space in files or network transmissions, and some weird hardware may use them if you have a hardware interface. But mostly, they serve no purpose, use doubles. If you have a good reason, disregard.
readFile.open("Resources/data/highscores.dll"); hard coded path names are nothing but trouble when you go to distribute a program to users. use relative paths.
settingstate.cpp
definitions: do not use #define. use constant expressions or using statements or enums (to group similar things cleanly).
assets manager: watch having little functions that create and destroy a heavy object (I do not know if these objects are heavy or not) frequently.
Granted this is just a drive-by, looking at code and trying to find things to complain about. I see a lot of very good things in there too, so do not be discouraged by my complaints.
some things I see that I would change.
Highscores.cpp -- this this this this this... classes can see their members by default. You should only need a this pointer once in a while -- its a tool that we MUST have, but you don't need it on every line. Its unreadable.
if (backgroundCounter >= 1500) -- magic number. wtf is 1500? give it a name in a constant. tell me what it is in the code by its name.
Draw(float dt) -- floats are not used much unless you have an exceptional reason to do so. They take up less space in files or network transmissions, and some weird hardware may use them if you have a hardware interface. But mostly, they serve no purpose, use doubles. If you have a good reason, disregard.
readFile.open("Resources/data/highscores.dll"); hard coded path names are nothing but trouble when you go to distribute a program to users. use relative paths.
settingstate.cpp
|
|
definitions: do not use #define. use constant expressions or using statements or enums (to group similar things cleanly).
assets manager: watch having little functions that create and destroy a heavy object (I do not know if these objects are heavy or not) frequently.
|
|
Granted this is just a drive-by, looking at code and trying to find things to complain about. I see a lot of very good things in there too, so do not be discouraged by my complaints.
Last edited on
I recommend you to enable almost ALL static analysis rules in VS, and enable ALL warnings.
This should give you an overwhelming amount of hints on how to improve your code!
I compile all my code with maxed cl warning/analyzer rules and what's so good about it is you practice to write less buggy code and conform to C++ guidelines.
For more info see:
https://docs.microsoft.com/en-us/cpp/code-quality/using-the-cpp-core-guidelines-checkers?view=vs-2019
and
https://stackoverflow.com/questions/24075528/how-do-i-enable-all-warnings-in-visual-studio-2008-in-a-project
also, a lot of the following is implemented in analyzer ruleset.
https://github.com/isocpp/CppCoreGuidelines
This should give you an overwhelming amount of hints on how to improve your code!
I compile all my code with maxed cl warning/analyzer rules and what's so good about it is you practice to write less buggy code and conform to C++ guidelines.
For more info see:
https://docs.microsoft.com/en-us/cpp/code-quality/using-the-cpp-core-guidelines-checkers?view=vs-2019
and
https://stackoverflow.com/questions/24075528/how-do-i-enable-all-warnings-in-visual-studio-2008-in-a-project
also, a lot of the following is implemented in analyzer ruleset.
https://github.com/isocpp/CppCoreGuidelines
Last edited on
void AssetsManager::loadTexture(string name, string path)Passing strings by value is considered bad practice. Normally you pass string by const ref or use a std::string_view.
https://en.cppreference.com/w/cpp/string/basic_string_view
https://www.modernescpp.com/index.php/c-17-avoid-copying-with-std-string-view
BTW: For 1 month learning your code looks impressive.
Topic archived. No new replies allowed.