My first OO C++ app

Can anybody comment on this code for me? I'm not new to OOP, but I am new to C++ and I'm sure I didn't exactly do this correctly from a C++ programmer's point of view. I was very concerned about learning the proper use of header files. Did I do it correctly? I think next I'll add some menu driven stuff and create a collection of house objects.

logic.h

Code:

class Building
{
public:
	Building();
	~Building();

	setRooms(int num);
	int getRooms();
	setFloors(int num);
	int getFloors();

private:
	int rooms, floors;
};

class House : public Building
{
public:
	House();
	~House();

	setBedrooms(int num);
	int getBedrooms();
	setBaths(int num);
	int getBaths();

private:
	int bedrooms, baths;
};

logic.cpp

Code:

#include <iostream>
#include "logic.h"

using namespace std;

/* Building functions */
Building::Building()
{
	std::cout << "New building object created." << endl;
}

Building::~Building()
{
	std::cout << "Building object being destroyed." << endl;
}

Building::setFloors(int num)
{
	floors = num;
}

Building::setRooms(int num)
{
	rooms = num;
}

int Building::getFloors() { return floors; }
int Building::getRooms() { return rooms; }

/* House functions */
House::House()
{
	std::cout << "New house object created." << endl;
}

House::~House()
{
	std::cout << "House object being destroyed." << endl;
}

House::setBaths(int num)
{
	baths = num;
}

House::setBedrooms(int num)
{
	bedrooms = num;
}

int House::getBaths() { return baths; }
int House::getBedrooms() { return bedrooms; }

main.cpp

Code:

#include <iostream>
#include "logic.h"

using namespace std;

int main()
{
	House h;
	h.setBaths(12);
	h.setBedrooms(40);
	h.setFloors(5);
	h.setRooms(30);

	std::cout << "Current house" << endl;
	std::cout << "Baths: " << h.getBaths() << endl;
	std::cout << "Bedrooms: " << h.getBedrooms() << endl;
	std::cout << "Floors: " << h.getFloors() << endl;
	std::cout << "Rooms: " << h.getRooms() << endl;

	return 0;
}

structurally 2 questions....
1) why the virtual destructor ? these classes dont seem to be polymorphic so thats unnecessary.
2) why are the data members protected. data should be private unless proven otherwise. With the get/set interfaces you have these data members will never need to be protected.

Biggest error....

void main().... wtf is that? the standard says that main always returns an int.

As for file layout.... seems perfect.

Originally posted by 7stud
You didn't get any errors when you compiled that?

Nope and it runs fine. :dunno:

Stoned, thanks for the suggestions. I'll make the changes.

lol just noticed that some funcs missing return types. missed that on first scan.
Probably didnt error as return types not specified default to int.

Probably didnt error as return types not specified default to int.

Ahhhh. That's why there weren't any errors when I tried compiling the program(and then deleted my post). I didn't know the return type defaulted to int.

I posted about the return types in your other thread.

another small point is that amount of unnecessary House:: and Building:: there are. Inside a class member function you can refer to the privates without the scope resolution operator being used.

i.e.

Code:

int Building::get_rooms() { return rooms;}

and not

Code:

int Building::get_rooms() { return Building::rooms;}

but thats purely a style issue.

Awesome feedback, thanks! I've edited the original post with the changes. Keep it coming! There are more differences between Java and C++ than I had imagined.

Typically you would set the state of your object when you create it with either some default value(s) or something specific. In this case, you would reasonably expect a building to be created with a number of (specific) rooms, so ideally your building constructor should set the number of rooms and floors. At the moment, someone can create a building and then get the number of rooms before it has been set to a sensible value.

Some of these function should return a value indicating success or failure, or throw an exception, for instance, you wouldn't expect to be able to set the number of rooms in a building to -100.

It doesn't look as though you're familiar with the use of const in C++ yet but that's something to look up and consider before long.

By the way, there is no default return type of int in C++.

One more minor point:

Code:

Building::setFloors(int num)
{
	floors = num;
}

I would break the habit of repeatedly using "num" as a parameter name, even for such short functions. Give it a meaningful name. This will become far more obvious when you pass multiple parameters or use longer functions, but the good habit should start now.

If you don't have it already, I would add an include guard to your logic.h file like this:

Code:

#ifndef LOGIC_H
#define LOGIC_H

// ...
// code
// ...

#endif // LOGIC_H

That is a good habit to get into for all you header files.

Originally posted by Stoned_Coder
1) why the virtual destructor ? these classes dont seem to be polymorphic so thats unnecessary.

I might disagree here. The classes aren't being used polymorphicly yet, but you can't base the decision on whether to make the destructor virtual or not on how its being used in your sample code (otherwise, you would have no need to make Building a separate class).

Obviously, you are attempting to design a class hierarchy where a House is a Building. Therefore, it is possible that in the future you will want to have operations on Buildings that can work with your Houses, too. It looks like your design is leading to polymorphic usage, so I would suggest putting in the virtual destructor now. Even if you don't plan to use the classes polymorphicly, many C++ people still advise making sure base classes have virtual destructors. I seem to be getting into a lot of discussions about this around here lately, but I haven't seen any good reason not to follow that advice.

I like Omnius's points about using the constructor and also using const. Also, if you have using namespace std;, you don't need to put std::cout. I prefer std:: over a using directive, but you don't need both.

Code:

#ifndef LOGIC_H
#define LOGIC_H

// ...
// code
// ...

#endif // LOGIC_H

What does this code do exactly?

I'll have to look into the const keyword.

Originally posted by CompiledMonkey

Code:

#ifndef LOGIC_H
#define LOGIC_H

// ...
// code
// ...

#endif // LOGIC_H

What does this code do exactly?

I'll have to look into the const keyword.

It just basically says that "if logic.h hasn't already been included, include it. Else, don't bother."

I personally thing #pragma once works better, or if you use the above code you just #include the header in the StdAfx.h file....(if you don't have a StdAfx.h file, you should do it next time).

Those are include guards. In your simple case, they aren't necessary. But assume you had another header file that included logic.h. Maybe family.h that had the declarations for a family class that had a House member. This requires a logic.h include:

family.h

Code:

#ifndef FAMILY_H
#define FAMILY_H

#include "logic.h"

class Family
{
    // ....
    House currentHome;
};
#endif

Now assume you used a Family object in your main.cpp. You would want to include "family.h". Since you are also separately using a House, you would keep the "logic.h" like this:

main.cpp

Code:

#include <iostream>
#include "logic.h"
#include "family.h"

// ...

When you include a header file (or any file, but you should really only ever include header files) with the #include "logic.h", all you are really doing is adding all the code in that file to the .cpp file where you put the include. This is done by the preprocessor before the code is actually compiled for that particular .cpp file. So when you compile main.cpp, the prepocesser places all the stuff from iostream and all the stuff from logic.h and all the stuff from family.h into main.cpp. It reads through the combined files and comes across another preprocessor directive:

#ifndef LOGIC_H

This checks to see if the symbol LOGIC_H is defined anywhere at this point in the compilation. Since this is a preprocessor directive, it does not correspond to a C++ variable, class, or type. Basically, you can define a symbol in your makefile or project settings, or you can define it using the #define directive. You can also use whatever symbol name you want for an include guard, but an all uppercase and underscore name that includes the file name is typical. MSVC++ adds a GUID (unique identifier) since it is important that the symbol is unique.

In this case, if the symbol is not defined (#ifndef), then the code after the #ifndef will execute all the way up until the matching #endif. If the symbol is defined, that code will be ignored. In the case of compiling main.cpp, the symbol will not have been defined at that point, so the preprocessor does not exclude the rest of the code until the #endif. It continues on to:

#define LOGIC_H

Since at this point the symbol has not been defined, the line above is not ignored. So it defines the symbol. The symbol is now defined for the rest of the preprocessing of this .cpp file.

All the code after the #define is also not excluded from the .cpp file. Then the preprocessor finds the following line:

#endif // LOGIC_H

This ends the #ifndef block above. The // comment isn't necessary, I just added it to show what the #endif corresponded to.

Now the rest of the code in the .cpp file is then searched by the preprocessor. The code inside family.h is found. But family.h includes logic.h, so the everything in logic.h will be added to the cpp code being compiled again. If you don't have the include guards, then you will get redefinition errors, since Building and House will have been defined twice.

If you do have include guards, the preprocesser will get to the #ifndef LOGIC_H and it will see that that particular symbol was already defined earlier, so it will ignore (and tell the compiler to ignore) everything up until the corresponding #endif.


So basically, it protects against defining stuff multiple times due to a file being included multiple times. You only do these in header files, since you only ever include a header file (if for some reason you needed to include a cpp file, you would have to consider adding the include guards there, too).

.... Geez, that was long. I guess you could sum it up with:

Originally posted by Quantrizi
It just basically says that "if logic.h hasn't already been included, include it. Else, don't bother."

Wow, best explaination ever. Thanks!

By the way, there is no default return type of int in C++.

I never actually said that there was. In fact, this is a kind of hold-over from c89. I think implicit ints in c were stopped in c99 and they certainly do not exist in c++ however, almost every compiler in existence seems to take a default return type of int with nothing more than a warning. I presume this is only allowed for backwards compatability and I never suggested it should be used, I just offered an explanation for why this code will compile even tho it looks flawed. I think gcc with -pedantic would have errored and comeau with /strict would have errored but most other compilers will happily compile this.