Advertisement

Advice on coding style

Started by December 23, 2015 11:45 PM
45 comments, last by Hodgman 8 years, 11 months ago


My Dev C++

I hope for your sake you're using one of the updated versions of Dev C++ and not the incredibly out-of-date Bloodshed version.

C++: A Dialog | C++0x Features: Part1 (lambdas, auto, static_assert) , Part 2 (rvalue references) , Part 3 (decltype) | Write Games | Fix Your Timestep!

Hint: binary operators should never be member functions.

Stephen M. Webb
Professional Free Software Developer

Advertisement

Sorry late to the party here, all of this is meant in the learning spirit so don't take it the wrong way.

So since you asked about coding style, automate the coding style with something like clang-format, etc and let it deal with "style".

Minor nitpicks with your code,

  • If your compiler supports C++11 or greater then you should too. Replace that enum with enum class, future code readers will thank you later
  • Just say no to pulling in the std namespace, being a game programmer, you will probably end up writing a vector class at some point
  • Probably want to just "inline" the date class's functions.
  • Same with Employee
  • FUTURE BUG: employee::showEmp() you can get that switch statement into a bad way. Think about adding a default case to avoid people from doing heinous things later on.
  • Float suck, don't use them unless you have a good reason
  • Switch statements in main, default case....just waiting for bad user input
  • "\nDate of employment (mm/dd/yyyy) format: ", There are several ways to do this, 1 of many, http://www.cplusplus.com/reference/ctime/strftime/
  • Apply the above to date::showDate()
  • int number, year, month, day; There exists a data structure to represent time already, no need to reinvent
  • date::getDate doesn't actually get anything, I would have personally expected time_t getDate() as the signature
  • Lots of issues between pass by value vs reference
  • employee::getStat doesn't actually get anything

I could probably find some more if I spent some more time on it. If you want to hack on the code sometime, ping me and we can hack on it online sometime if that would help out in learning.

Sorry late to the party here, all of this is meant in the learning spirit so don't take it the wrong way.

So since you asked about coding style, automate the coding style with something like clang-format, etc and let it deal with "style".

Minor nitpicks with your code,

  • If your compiler supports C++11 or greater then you should too. Replace that enum with enum class, future code readers will thank you later
  • Just say no to pulling in the std namespace, being a game programmer, you will probably end up writing a vector class at some point
  • Probably want to just "inline" the date class's functions.
  • Same with Employee
  • FUTURE BUG: employee::showEmp() you can get that switch statement into a bad way. Think about adding a default case to avoid people from doing heinous things later on.
  • Float suck, don't use them unless you have a good reason
  • Switch statements in main, default case....just waiting for bad user input
  • "\nDate of employment (mm/dd/yyyy) format: ", There are several ways to do this, 1 of many, http://www.cplusplus.com/reference/ctime/strftime/
  • Apply the above to date::showDate()
  • int number, year, month, day; There exists a data structure to represent time already, no need to reinvent
  • date::getDate doesn't actually get anything, I would have personally expected time_t getDate() as the signature
  • Lots of issues between pass by value vs reference
  • employee::getStat doesn't actually get anything

I could probably find some more if I spent some more time on it. If you want to hack on the code sometime, ping me and we can hack on it online sometime if that would help out in learning.

"floats suck"

Why?

What will you make?

Floats suck for beginners (and others) because they require documents for all the potential edge cases that look like this:

http://docs.oracle.com/cd/E19957-01/806-3568/ncg_goldberg.html

A link with some fun pitfalls, https://randomascii.wordpress.com/2012/06/26/doubles-are-not-floats-so-dont-compare-them/

I'm not saying to avoid them in general btw just not the most beginner friendly item here.

Also, I noticed the downvotes....what exactly are you not liking here?

Advertisement

Sorry late to the party here, all of this is meant in the learning spirit so don't take it the wrong way.

So since you asked about coding style, automate the coding style with something like clang-format, etc and let it deal with "style".

Minor nitpicks with your code,

  • If your compiler supports C++11 or greater then you should too. Replace that enum with enum class, future code readers will thank you later
  • Just say no to pulling in the std namespace, being a game programmer, you will probably end up writing a vector class at some point
  • Probably want to just "inline" the date class's functions.
  • Same with Employee
  • FUTURE BUG: employee::showEmp() you can get that switch statement into a bad way. Think about adding a default case to avoid people from doing heinous things later on.
  • Float suck, don't use them unless you have a good reason
  • Switch statements in main, default case....just waiting for bad user input
  • "\nDate of employment (mm/dd/yyyy) format: ", There are several ways to do this, 1 of many, http://www.cplusplus.com/reference/ctime/strftime/
  • Apply the above to date::showDate()
  • int number, year, month, day; There exists a data structure to represent time already, no need to reinvent
  • date::getDate doesn't actually get anything, I would have personally expected time_t getDate() as the signature
  • Lots of issues between pass by value vs reference
  • employee::getStat doesn't actually get anything

I could probably find some more if I spent some more time on it. If you want to hack on the code sometime, ping me and we can hack on it online sometime if that would help out in learning.

"floats suck"

Why?

To elaborate a bit on Henderson's post: it appears that OP is using a float to store employee wages. There are some reasons not to use floating-point numbers to store currency values. It's not that floats suck in general, or that they should always be avoided (Henderson making that claim why s/he's getting down-voted) - it's that using them to store money values can lead to bad things happening.


When doing any kind of calculation with currency, accuracy is extremely important. And floating point numbers (floats and doubles) don’t have an accurate enough representation to prevent rounding errors from accumulating when doing arithmetic with monetary values.

Well put on the currency issue. That was the road I was heading down but didn't put it quite so elegantly (edge cases rant).

Prohibitions without adequate explanation are counter-productive. Some people will react by rejecting your advice and others will accept it pathologically and develop (and often spread) alternative bad habits. I'm guilty of doing the same thing sometimes though.

If your compiler supports C++11 or greater then you should too. Replace that enum with enum class, future code readers will thank you later

Can you make (or link) a compelling case for this?

Just say no to pulling in the std namespace, being a game programmer, you will probably end up writing a vector class at some point

I think I understand what you mean, but could you explain it so that someone who doesn't can get why it's a problem?

Probably want to just "inline" the date class's functions.
Same with Employee

?

FUTURE BUG: employee::showEmp() you can get that switch statement into a bad way. Think about adding a default case to avoid people from doing heinous things later on.

Such as?

Float suck, don't use them unless you have a good reason

-.-

Switch statements in main, default case....just waiting for bad user input

What input is "bad" and why? What will happen and how can it be avoided?

"\nDate of employment (mm/dd/yyyy) format: ", There are several ways to do this, 1 of many, http://www.cplusplus.com/reference/ctime/strftime/
Apply the above to date::showDate()

Avoid stdlib in C++. If you want to promote a standardized method then C++ uses <chrono>.

int number, year, month, day; There exists a data structure to represent time already, no need to reinvent

What data structure?

Lots of issues between pass by value vs reference

This probably implies a lack of understanding. Can you explain the correct usage and why it's superior?

void hurrrrrrrr() {__asm sub [ebp+4],5;}

There are ten kinds of people in this world: those who understand binary and those who don't.

Good point, here are references for all them. I just walked thru your list in order and provided references, etc where needed.

This topic is closed to new replies.

Advertisement