Advice on coding style

Started by
45 comments, last by Hodgman 8 years, 3 months ago

So, I've only been learning to program for the past couple weeks. I've been reading Sams Teach Yourself C++ in 21 Days while supplementing it with Object Oriented Programming in C++.

I'm finishing up the chapter on objects and classes and am doing an exercise. This is going to be VERY basic code and will compile. I'm just wondering if my coding is "intelligent" and organized making it easy to set and display values of multiple classes. Remember I'm VERY new and this code was all from scratch from the following question:

"Extend the employee class of Exercise 4 to include a date class and an etype enum. An object of the date class should be used to hold the date of first employment; that is, the date when the employee was hired. The etype variable should hold the employees type: (I used different ones). These two items will be private member data in the employee definition, just like the employee number and salary. You'll need to extend the getemploy() and putemploy() functions to obtain this new information from the user and display it. These functions will probably need switch statements to handle the etype variable. Write a main() program that allows the user to enter data for three employee variables and then displays this data."


// Exercise 6
#include <iostream>
using namespace std;
enum etype {manager, laborer, executive, driver		// define the types of employees
};

class date											// date the employee was hired
{
	private:
		int year, month, day;
	public:
		date():year(0), month(0), day(0)			// set default to 0,0,0 (no real purpose here but looks good)
		{}
		void getDate(int y, int m, int d);			// get and set the private variables
		void showDate() const;						// show the date of employment (Can't modify anything)
};

void date::getDate(int y, int m, int d)
{
	year = y; month = m; day = d;
}
void date::showDate() const
{
	cout << month << '/' << day << '/' << year;
}

class employee									// Employee class
{
	private:
		etype position;							// store position
		int number;
		float wage;
		date eDate;								// store date for employee as an object of date
	public:
		void getStat(int n, float w, etype type, int y, int m, int d);	// get all the stats in one call (simplicity?)
		void showEmp() const;											// show all the stats in one call
};

void employee::getStat(int n, float w, etype type, int y, int m, int d)
{
	number = n; wage = w; position = type;
	eDate.getDate(y, m, d);						// call the date class to store the dates
}
void employee::showEmp() const
{
	cout << " position is: ";		// figure out the position of the employee and display
	switch (position)
	{
		case manager:	cout << "manager"; break;
		case laborer:	cout << "laborer"; break;
		case executive: cout << "executive"; break;
		case driver: 	cout << "driver"; break;
	}
	cout << "\nEmployee number is: " << number;
	cout << "\nEmployee wage is: " << wage;
	cout << "\nEmployee start date is: "; eDate.showDate();		// call the member function of date to show employee date
}

int main()
{
	employee John, Joe, Jill;		// 3 employees
	int number, year, month, day;
	float wage;
	etype type;
	int temptype;					// to convert user input to enum etype
	char temp = ' ';				// for the '/' input from user
	
	cout << "Please enter the following details about John - ";
	cout << "\nPosition (0)manager, (1)laborer, (2)executive, (3)driver: "; 
	cin >> temptype;
	switch(temptype)				// turn the input into an enum etype
	{
		case 0:	type = manager;break;
		case 1: type = laborer; break;
		case 2: type = executive;break;
		case 3: type = driver;break;
	}
	cout << "\nEmployee number: "; cin >> number;
	cout << "\nDate of employment (mm/dd/yyyy) format: "; cin >> month >> temp >> day >> temp >> year;
	cout << "\nWage: "; cin >> wage;
	John.getStat(number, wage, type, year, month, day);
	
	cout << "Please enter the following details about Joe - ";
	cout << "\nPosition (0)manager, (1)laborer, (2)executive, (3)driver: "; cin >> temptype;
	switch(temptype)
	{
		case 0:	type = manager;break;
		case 1: type = laborer; break;
		case 2: type = executive;break;
		case 3: type = driver;break;
	}
	cout << "\nEmployee number: "; cin >> number;
	cout << "\nDate of employment (mm/dd/yyyy) format: "; cin >> month >> temp >> day >> temp >> year;
	cout << "\nWage: "; cin >> wage;
	Joe.getStat(number, wage, type, year, month, day);
	
	cout << "Please enter the following details about Jill - ";
	cout << "\nPosition (0)manager, (1)laborer, (2)executive, (3)driver: "; cin >> temptype;
	switch(temptype)
	{
		case 0:	type = manager;break;
		case 1: type = laborer; break;
		case 2: type = executive;break;
		case 3: type = driver;break;
	}
	cout << "\nEmployee number: "; cin >> number;
	cout << "\nDate of employment (mm/dd/yyyy) format: "; cin >> month >> temp >> day >> temp >> year;
	cout << "\nWage: "; cin >> wage;
	Jill.getStat(number,wage,type,year,month,day);
	
	cout << "Johns stats: ";
	John.showEmp() ;
	
	cout << endl;
	
	cout << "Joe's stats: ";
	Joe.showEmp() ;
	
	cout << endl;
	
	cout << "Jill's stats: ";
	Jill.showEmp() ;
	
	cout << endl;
	
	return 0;
}

Any advice on the code I wrote? Things to pay attention to or things I over complicated?

I hope to use your brains to help me learn properly and not get into bad habbits! Thanks in advance.

Advertisement

Please not that the following is based on the most common styles I've seen, it is not universal.

The following is discouraged:

  • Having multiple statements on one line

  • Having multiple variable declarations on one line

  • Abbreviated variable names (use the full name for a variable "year" instead of "y")

Most of your variables start with a lowercase letter (which is standard), but "John", "Joe" and "Jill" appear to be exceptions? Consistency is the most important part of having a code style, the actual details aren't too important (though it is good to stick with common ones unless you have a reason not to). Typically, constants are uppercase, e.g. enum entries would be "DRIVER". Types usually use "CamelCase", e.g. Employee, Date, EmployeeType.

Separate class or function declarations with a blank line. Separate function parameters with a space.

Personally, I prefer if a comment is before the statement it related to, and starts at the same indentation level.

Most people use "get" as a member function prefix to mean that the member function will only fetch data from the object and not modify it.

You repeated code in your main function, can you extract the logic for "John", "Joe" and "Jill" to be less specific, without affecting the program's functionality?

Consider renaming "eDate" to something meaningful, like "dateHired" or "hiredOn".

Thank you for the response! That's exactly what I was looking for. Being so green in programming, I don't want to start making mistakes early on and learn bad habits. I was thinking about the repetitive lines in the main function, and can definitely make a function declaration before and define it later to get all those variables and clean up main.

As for the multiple statements on one line, I just started doing this as the author was using it consistently. I thought it looked awkward but seemed to save space. I'll get rid of that, along with using multiple variable declarations on one line. I'm glad my gut instinct did not want to do this, but when reading code that consistently used it, I thought I was in the wrong.

This is my second "go" at learning programming. It was what I wanted to pursue after high school, but life circumstances changed and had to earn an income first. I tried about 6 years ago but didn't have the time once the second child came! I always thought Sams C++ in 21 days was the better book, but after doing the exercises, I'm learning a lot more from OOP in C++. After these books I was going to read accelerated c++ and start beginning c++ game programming <- the ultimate goal. Then move onto Tricks of the windows game programming gurus.... but I think books that assist with the object oriented part would be even more ideal before diving into windows programming? (I can't accept sentences that say: This code does this, don't ask how, but it works).

enum variables in the book are all upper case, I did not realize that it was standard. I'll adjust that. As for the objects John, Joe and Jill starting with an uppercase is probably bad mannerisms from the actual English language.

I hope I posted in the right forum, if not you can move this to wherever it belongs. I plan to use this site to aid in my development and to hopefully answer questions (and in time, I'll answer questions) I may have. Thanks again for the response, I'll change up the syntax to be more in style. I hope if I post some of the larger exercises for criticism people chime in! Again, thank you for the time to reply.

Regards,

Oh and what did you mean by this: "Separate class or function declarations with a blank line. Separate function parameters with a space.".

A quick example to explain?


Separate class or function declarations with a blank line

Instead of:

class A
{
    // A stuff...
};
class B
{
    // B stuff...
};
void foo()
{
    // foo stuff...
}
void bar()
{
    // bar stuff
}

Put a blank line between them:

class A
{
    // A stuff...
};
 
class B
{
    // B stuff...
};
 
void foo()
{
    // foo stuff...
}
 
void bar()
{
    // bar stuff
}

This helps to keep them visually distinct.


Separate function parameters with a space.

You had:

Jill.getStat(number,wage,type,year,month,day);

Use a space to separate the parameters:

Jill.getStat(number, wage, type, year, month, day);
Couple more points:
Many of the comments are unnecessary, in that they just restate what the code does. Maybe the book does that to help guide you through the program, and it is fine if you want to keep doing so in the short term to remind you what different parts mean, but remember that this is not how comments are usually used. Comments are best used to give context to the program, maybe an example in your program is why the "hire date" is important but not the "start date".

... I'm glad my gut instinct did not want to do this, but when reading code that consistently used it, I thought I was in the wrong.
Remember this is code style we're talking about, it is not objectively right or wrong.

... but I think books that assist with the object oriented part would be even more ideal before diving into windows programming?
I can't say I've read those exact books but strengthening the fundamentals first sounds like a good idea.

(I can't accept sentences that say: This code does this, don't ask how, but it works).
A good attitude for the most part, but don't let it turn into a "not invented here" syndrome!

enum variables in the book are all upper case, I did not realize that it was standard. I'll adjust that. As for the objects John, Joe and Jill starting with an uppercase is probably bad mannerisms from the actual English language.
Yes. Indeed most of the time you want your program to be agnostic to such details. Giving human friendly names to the data structures in your program is mostly accomplished using some kind of lookup system like a dictionary data structure (e.g. std::map<>) or a database of some kind. Don't worry about this too much for now, but I'd say that if you can, try make names data (even if only a string literal) and not code.

I hope I posted in the right forum, if not you can move this to wherever it belongs.
This is the right place!

Again, thank you for the time to reply.
No problem! :]

+1 re not writing comments that just repeat what the code already says.

Code should say "what"; comments should say "why".

I'm looking at this, though:


	switch(temptype)
	{
		case 0:	type = manager;break;
		case 1: type = laborer; break;
		case 2: type = executive;break;
		case 3: type = driver;break;
	}

Generally you want to give statements their own line so that they're more readable. If you want to join statements together visually then just put an empty line above and below them. However, in this specific case ("case n: singleSimpleStatement(); break;") I'm okay with this style (except put a space before the breaks) because it makes the switch easier to read. I use this myself in some cases.

However, this switch is - in and of itself - a DRY violation. You're performing the same action in each case with the only difference being a parameter. That's a signal that there's a better way to write the code. Check this out:


enum etype { manager, laborer, executive, driver };
etype etypeArray[] = { manager, laborer, executive, driver };
//...
type = etypeArray[temptype];

You could also just specify an enum base type and then just cast temptype to etype, but some people frown on that since enums should generally be thought of non-numerically.

I'd also point out that you're not sanitizing your user input. if the user enters "12" as a type here, for instance, then you're going to get undefined behavior. Can you try writing a function that prints out a message and then accepts a specified input value? I have a template function for this that I'll show you later, but it's a good exercise for you to try writing one.

Try writing defining this:


int getUserInt(const std::string& message, int minVal, int maxVal);

Such that you could say:


char question[] = "Please enter the following details about Jill - \nPosition (0)manager, (1)laborer, (2)executive, (3)driver: ";
temptype = getUserInt(question, 0, 3);

The function should print the message string, then wait for input. Once it gets input it should check to make sure that it's valid, and if it's not then it should state that the input was invalid and ask the question again. It should repeat this until it gets a valid response, then return that response.

Note that you should also reset cin after use because users can do strange things to you:


//seems simple enough... but danger lurks within!
std::cin >> someValue;

//fetch this now since it's about to be reset
bool ok = std::cin.good();

//this clears error flags from the stream (makes the stream state "good" again)
std::cin.clear();

//the user could enter something like "12 and a partidge in a pear tree"
//and your next attempts to stream from cin would get filled in with "and", "a", "partidge", "in", "a", "pear", "tree" respectively
//cin.ignore() purges the stream up to the first delimiter it finds (in this case '\n')
//see http://www.cplusplus.com/reference/istream/istream/ignore/
std::cin.ignore(std::numeric_limits<streamsize>::max(), '\n');

if(ok) { /* input didn't cause a stream or conversion error */ }
else { /* input could not be interpreted - someValue was not set by cin */ }

Users are dangerous!

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.

Wow thank you for all the replies. As for the use of arrays, I haven't delved that far yet but the example to use with enums looks good. As for the user protection, I had thought of it but deemed it not necessary for now as the exercise was just to get the program to function... that being said, I agree I should put it in regardless. Either way, it will strengthen my C++ skills as they develop.

Awesome notes from everyone and appreciate the feedback. Merry Christmas everyone! I hope to post some updated and more correct exercises in a few days. As always, all criticism welcome!

Wow thank you for all the replies. As for the use of arrays, I haven't delved that far yet but the example to use with enums looks good. As for the user protection, I had thought of it but deemed it not necessary for now as the exercise was just to get the program to function... that being said, I agree I should put it in regardless. Either way, it will strengthen my C++ skills as they develop.

Awesome notes from everyone and appreciate the feedback. Merry Christmas everyone! I hope to post some updated and more correct exercises in a few days. As always, all criticism welcome!

The most important thing is to not be afraid to try new things (style wise or otherwise). Learning to be flexible and work with more than one style is far more important than finding a 'perfect' style.

Seems like you're asking more for coding guidelines than coding style advices.

I think it's important to make the difference between guidelines (ie: good practice) and coding style. Guidelines can be objectively argued for or against and make your code better and usually make your code better. Coding style is more like a programmer's signature. Arguments on coding styles are basically made of fools arguing with other fools, sometimes very vigorously, which can be kind of funny.

Putting declarations on multiple lines is good practice because it can be error prone, especially in C and C++ because of the pointer and reference shenanigans. Systematically declaring a 1-parameter "explicit" unless you really mean to allow implicit construction is another example of a guideline.

Coding style is something like opening brace placement or naming conventions and in languages like C and C++ where no "official" style is suggested, you just pick one that suits your taste for your personal projects, and whatever your organization has decided on for work. No point in arguing on that. Personally, I hate it with the raging passion of a thousands supernovae exploding in unission when people use lowerCamelCasing to name public methods (use either snake_casing or UpperCamelCasing for your public symbols, IMHO) but I'm not going to tell you that it's better for humanity that you don't use that style. (Ok, maybe it's better for MY humanity, but that's a different thing. tongue.png )

And by the way don't ever think about using the Google coding guidelines for your C++ code, unless you have a parrot and are out of newspapers to put in the cage. It basically bans everything C++ brings on the table in the name of incompetent C++ developers that will use those wrong. I have no words for how much I hate the Google C++ coding guidelines. Look here for guidelines and coding style for modern C++: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md. This was written by the big names in the C++ community. I have a bit of reserves for their suggestions on actual coding style since they recommend a naming style that pretty much nobody uses, but again, arguments on coding style... lol, personally not gonna get started.

This topic is closed to new replies.

Advertisement