Jump to content

  • Log In with Google      Sign In   
  • Create Account

FREE SOFTWARE GIVEAWAY

We have 4 x Pro Licences (valued at $59 each) for 2d modular animation software Spriter to give away in this Thursday's GDNet Direct email newsletter.


Read more in this forum topic or make sure you're signed up (from the right-hand sidebar on the homepage) and read Thursday's newsletter to get in the running!


C++ SFML Code Review


Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.

  • You cannot reply to this topic
12 replies to this topic

#1 Naughtyusername   Members   -  Reputation: 482

Like
0Likes
Like

Posted 13 November 2013 - 12:52 AM

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!



Sponsor:

#2 wintertime   Members   -  Reputation: 1881

Like
0Likes
Like

Posted 13 November 2013 - 04:52 AM

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



#3 Álvaro   Crossbones+   -  Reputation: 13933

Like
1Likes
Like

Posted 13 November 2013 - 05:01 AM

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.

#4 Paradigm Shifter   Crossbones+   -  Reputation: 5439

Like
0Likes
Like

Posted 13 November 2013 - 05:09 AM

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 :(


"Most people think, great God will come from the sky, take away everything, and make everybody feel high" - Bob Marley

#5 Naughtyusername   Members   -  Reputation: 482

Like
0Likes
Like

Posted 13 November 2013 - 05:23 AM

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.



#6 Álvaro   Crossbones+   -  Reputation: 13933

Like
3Likes
Like

Posted 13 November 2013 - 05:25 AM

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.

#7 Naughtyusername   Members   -  Reputation: 482

Like
0Likes
Like

Posted 13 November 2013 - 05:26 AM

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!



#8 Álvaro   Crossbones+   -  Reputation: 13933

Like
1Likes
Like

Posted 13 November 2013 - 05:30 AM

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.

#9 Naughtyusername   Members   -  Reputation: 482

Like
0Likes
Like

Posted 13 November 2013 - 05:31 AM

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 :)



#10 Naughtyusername   Members   -  Reputation: 482

Like
0Likes
Like

Posted 13 November 2013 - 05:50 AM

 

 

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 :).



#11 Naughtyusername   Members   -  Reputation: 482

Like
0Likes
Like

Posted 13 November 2013 - 05:53 AM

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.



#12 serumas   Members   -  Reputation: 730

Like
2Likes
Like

Posted 13 November 2013 - 06:46 AM

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

....



#13 Naughtyusername   Members   -  Reputation: 482

Like
1Likes
Like

Posted 13 November 2013 - 07:26 AM

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! :)






Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.



PARTNERS