Sign in to follow this  

C++ SFML Code Review

This topic is 1526 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Hello! i have been programming in C++ for about a year now. I was just wondering if some of you veteran programmers could give me your thoughts on my code, let me know what i have done wrong, what ive done well, ect. don't hesitate to say something in here is absolutely horrible, please let me know. there is a few parts in here that i hate the way i did, but wasn't sure about another way of doing it. and it just being a prototype some of the things i just didn't bother messing with.

 

one thing im going to add before moving switching back to using OOP, is timing. if you guys have any advice on that, it would be appreciated as well.

im interested in what you think about how i handled the states, if i should go about it another way, or if the way i did it is good.

Here is a link to paste-bin if you don't want to download the .zip attached below. http://pastebin.com/bmCmwYRA

this is using the SFML 2.1 Lib, in case anyone is looking at this is interested in SFML/Seeking Help with SFML.

 

Thanks!

Share this post


Link to post
Share on other sites

For a start you could avoid including a huge amount of headers just in case you need them a year later maybe, split up those 500 line functions and not waste 40 characters on an average line cause of overly deep indentation that messes up viewing the code and split your code into more files.wink.png

Share this post


Link to post
Share on other sites
void StringChangeDif(sf::Text & t1, int & a)
{
                switch(a)
                {
                case 0:
                                t1.setString("Cake");
                                break;
                case 1:
                                t1.setString("Easy");
                                break;
                case 2:
                                t1.setString("Medium");
                                break;
                case 3:
                                t1.setString("Hard");
                                break;
                case 4:
                                t1.setString("Impossible");
                                break;
                }
}
I suggest doing this instead:
void StringChangeDif(sf::Text & t1, int a) // No need to pass primitive types by reference if you are not going to modify them
{
  static std::string const difficulty_name[5] = {"Cake", "Easy", "Medium", "Hard", "Impossible"};
  
  t1.setString(difficulty_name[a]);
}
Similarly for most `switch' statements in your code and some `if-else if' sections. Whenever you have repetitive code like that, you should probably try to use a data structure instead.

Share this post


Link to post
Share on other sites

I don't like the switch or Alvaro's code ;)

 

The switch is better behaved since it wont do anything if passed a value not in the range 0 to 4. I'd put in a default case that asserts or something though. All switch statements should have a default case!

 

Alvaro's code doesn't do any range checking at all :(

Share this post


Link to post
Share on other sites

For a start you could avoid including a huge amount of headers just in case you need them a year later maybe, split up those 500 line functions and not waste 40 characters on an average line cause of overly deep indentation that messes up viewing the code and split your code into more files.wink.png

 

the headers aren't really an issue as this is a prototype. once i get the hang of everything im trying this project ends. will take the note in the future for those OOP focused projects. formatting wont be an issue in the future, again, this being a dinky prototype im not going to spend too much time making it look pretty,i can navigate around it well enough. the indentation looks the best IMO and works with my widescreen monitors, unfortunately on pastebin it is not formatted the same way, and depending on your IDE and formatting rules i suppose it would also look different. this being a prototype i didn't want to use classes, im still trying to learn how to handle certain game logic, and using classes interferes with the work that really needs to get done.

thanks for your input.

Share this post


Link to post
Share on other sites

Alvaro's code doesn't do any range checking at all sad.png


Ah, we have a philosophical difference of opinion here. I don't think the code should be responsible for doing something reasonable if you use it wrong. You may not like my philosophy, but the whole language is built that way.

I would normally write asserts in a function like that, but I didn't want to complicate things.

Share this post


Link to post
Share on other sites
void StringChangeDif(sf::Text & t1, int & a)
{
                switch(a)
                {
                case 0:
                                t1.setString("Cake");
                                break;
                case 1:
                                t1.setString("Easy");
                                break;
                case 2:
                                t1.setString("Medium");
                                break;
                case 3:
                                t1.setString("Hard");
                                break;
                case 4:
                                t1.setString("Impossible");
                                break;
                }
}
I suggest doing this instead:
void StringChangeDif(sf::Text & t1, int a) // No need to pass primitive types by reference if you are not going to modify them
{
  static std::string const difficulty_name[5] = {"Cake", "Easy", "Medium", "Hard", "Impossible"};
  
  t1.setString(difficulty_name[a]);
}
Similarly for most `switch' statements in your code and some `if-else if' sections. Whenever you have repetitive code like that, you should probably try to use a data structure instead.

 

wonderful! i knew there had to be a better way of doing that. that was the main thing that aggravated me in my code was those absurdly long switch's/ifs. Thanks!

Share this post


Link to post
Share on other sites

For a start you could avoid including a huge amount of headers just in case you need them a year later maybe, split up those 500 line functions and not waste 40 characters on an average line cause of overly deep indentation that messes up viewing the code and split your code into more files.wink.png

 
the headers aren't really an issue as this is a prototype. once i get the hang of everything im trying this project ends. will take the note in the future for those OOP focused projects. formatting wont be an issue in the future, again, this being a dinky prototype im not going to spend too much time making it look pretty,i can navigate around it well enough. the indentation looks the best IMO and works with my widescreen monitors, unfortunately on pastebin it is not formatted the same way, and depending on your IDE and formatting rules i suppose it would also look different. this being a prototype i didn't want to use classes, im still trying to learn how to handle certain game logic, and using classes interferes with the work that really needs to get done.
thanks for your input.


Since this is a learning project, you should try to make the code as good as possible, use classes (if they make sense), break down large functions into smaller functions (if you can give them good names), avoid using global variables, etc. If you allow yourself bad habits in a tiny program where it wouldn't be that much work to do things right, heaven knows what you will do in a large project where doing things right sometimes requires huge efforts.

Share this post


Link to post
Share on other sites

I don't like the switch or Alvaro's code ;)

 

The switch is better behaved since it wont do anything if passed a value not in the range 0 to 4. I'd put in a default case that asserts or something though. All switch statements should have a default case!

 

Alvaro's code doesn't do any range checking at all sad.png

i think he was just trying to get the point across, not write my code for me. i prefer his way of doing it just because its less tedious repetitive crap to do :)

Share this post


Link to post
Share on other sites

 

 

For a start you could avoid including a huge amount of headers just in case you need them a year later maybe, split up those 500 line functions and not waste 40 characters on an average line cause of overly deep indentation that messes up viewing the code and split your code into more files.wink.png

 
the headers aren't really an issue as this is a prototype. once i get the hang of everything im trying this project ends. will take the note in the future for those OOP focused projects. formatting wont be an issue in the future, again, this being a dinky prototype im not going to spend too much time making it look pretty,i can navigate around it well enough. the indentation looks the best IMO and works with my widescreen monitors, unfortunately on pastebin it is not formatted the same way, and depending on your IDE and formatting rules i suppose it would also look different. this being a prototype i didn't want to use classes, im still trying to learn how to handle certain game logic, and using classes interferes with the work that really needs to get done.
thanks for your input.

 


Since this is a learning project, you should try to make the code as good as possible, use classes (if they make sense), break down large functions into smaller functions (if you can give them good names), avoid using global variables, etc. If you allow yourself bad habits in a tiny program where it wouldn't be that much work to do things right, heaven knows what you will do in a large project where doing things right sometimes requires huge efforts.

 

I completely get that and agree 100%. im working on making my functions better, and ive come a LONG way, still got quite a bit left to learn though. This is a learning project in the sense that, i wanted to learn how to implement basic game fundamentals. i more than know the basics of C++ and SFML, and finished pong using classes and OOP, sprites ect. but it was lacking some of the things i wanted, (States, custom methods to make certain things simpler/better/faster, timing(which i didn't get to in the code you guys are looking at), im really OCD, so formatting is always on my mind, and this project probably looks vomit worthy outside of my IDE lol, but the way i have it set up works for what it is. the bad habits thing is definitely something i don't want to start/hang on too, i try not to use global's often if ever, and in this project the only ones i use are Width/Height, and the #includes. its going to take quite a bit of practice before id be able to keep a large project understandable. the variables i use in this that are super vague, i only do in projects where i don't expand into using classes. i gave up on trying to keep things short, and decided id rather know what something does rather than type X letters less, which im glad i started doing :).

Share this post


Link to post
Share on other sites

I don't like the switch or Alvaro's code ;)

 

The switch is better behaved since it wont do anything if passed a value not in the range 0 to 4. I'd put in a default case that asserts or something though. All switch statements should have a default case!

 

Alvaro's code doesn't do any range checking at all sad.png

You're right about the default case, i should of put those in. thinking back on it i decided not too because i didn't think it through fully. but now i will add them so i wont have to independently choose a variable outside of the function to give it a default value. its ok that he didn't do range checking, i do that already in my code and would of implemented it when needed.

Share this post


Link to post
Share on other sites

there are lots of things to take atention :

interesting int to string conversation :) there is functions for that,

several game loops with duplicating code,

all code that makes different things could be moved to functions draw initialize mousehandling and etc,

som drawing objects are initializing every loop - initialize them before main loop, 

magic numbers,

instead of switch with cases 0 1 2 3...   posible better to use array[]

maybe start using vectors sfVector2d  or similar

....

Share this post


Link to post
Share on other sites

there are lots of things to take atention :

interesting int to string conversation smile.png there is functions for that,

several game loops with duplicating code,

all code that makes different things could be moved to functions draw initialize mousehandling and etc,

som drawing objects are initializing every loop - initialize them before main loop, 

magic numbers,

instead of switch with cases 0 1 2 3...   posible better to use array[]

maybe start using vectors sfVector2d  or similar

....

Thanks! put your post in my ToDo file and will get working on it! :)

Share this post


Link to post
Share on other sites
Sign in to follow this