6
\$\begingroup\$

So I'm working through a beginners C++ book and they had this example demonstrating class hierarchy, which was originally in a single file. I tried recreating it with multiple files (header files + source files) to test my understanding, but I wanted to make sure that I did everything in the "optimal" way. Specifically, I had to use more #includes than I thought I would need to, so I'm wondering if I can make improvements there. Here is what the project looks like:

main.cpp

#include "FDeepDishPizza.h"
#include <iostream>
using std::cout, std::endl;

int main()
{
    cout << "Hello world!" << endl;
    FDeepDishPizza deepPizza(12, 13, 14, 15);
    deepPizza.DumpDensity();
    return 0;
}

FrozenFood.h

#ifndef FROZENFOOD_H_INCLUDED
#define FROZENFOOD_H_INCLUDED

class FrozenFood {
private:
    int Price;

protected:
    int Weight;

public:
    FrozenFood(int APrice, int AWeight);
    int GetPrice();
    int GetWeight();
};


#endif // FROZENFOOD_H_INCLUDED

FrozenFood.cpp

#include "FrozenFood.h"
#include <iostream>
using std::cout, std::endl;

FrozenFood::FrozenFood(int APrice, int AWeight) {
    Price = APrice;
    Weight = AWeight;
}

int FrozenFood::GetPrice() {
    cout << Price << endl;
}

int FrozenFood::GetWeight() {
    cout << Weight << endl;
}

FrozenPizza.h

#ifndef FROZENPIZZA_H_INCLUDED
#define FROZENPIZZA_H_INCLUDED

#include "FrozenFood.h"

class FrozenPizza : public FrozenFood {                  
protected:
    int Diameter;
public:
    FrozenPizza(int APrice, int AWeight, int ADiameter);
    void DumpInfo();
};


#endif // FROZENPIZZA_H_INCLUDED

FrozenPizza.cpp

#include "FrozenPizza.h"
#include <iostream>
using std::cout, std::endl;

FrozenPizza::FrozenPizza(int APrice, int AWeight, int ADiameter) :
    FrozenFood(APrice, AWeight) {
    Diameter = ADiameter;
}

void FrozenPizza::DumpInfo() {
    cout << "\tFrozen Pizza Info:" << endl;
    cout << "\t\tWeight: " << Weight << " ounces" << endl;
    cout << "\t\tDiameter: " << Diameter << " inches" << endl;
}

FDeepDishPizza.h

#ifndef FDEEPDISHPIZZA_H_INCLUDED
#define FDEEPDISHPIZZA_H_INCLUDED

#include "FrozenPizza.h"

    class FDeepDishPizza : public FrozenPizza {         //public inherit from FrozenPizza, which itself inherits FrozenFood
    private:
        int Height;
    public:
        FDeepDishPizza(int APrice, int AWeight, int ADiameter, int AHeight);
        void DumpDensity();
    };
    
    #endif // FDEEPDISHPIZZA_H_INCLUDED

FDeepDishPizza.cpp

#include "FDeepDishPizza.h"
#include <iostream>
using std::cout, std::endl;

FDeepDishPizza::FDeepDishPizza(int APrice, int AWeight, int ADiameter, int AHeight) :
    FrozenPizza(APrice, AWeight, ADiameter) {
        Height = AHeight;
    }

void FDeepDishPizza::DumpDensity() {
    cout << "\tDensity: ";
    cout << Weight * 2 << " pounds per cubic foot" << endl; //doesn't actually give density just a test
}
\$\endgroup\$
8
\$\begingroup\$

Don't use std::endl

Use "\n" instead of std::endl. The latter is equivalent to "\n", but also forces a flush of the output stream, which can be bad for performance.

Don't confuse getting a value with printing a value

You have functions like int GetPrice(), that look like they return the price as an int. However, in your implementation, you actually print the price to the screen, and return nothing. Either don't print and return the price, or rename the function void PrintPrice() and return nothing.

It's usually better to just return the value instead of printing them. The caller can then decide if they want to print the value, and if so how.

Consider adding operator<< overloads for printing

It's always best to make classes behave like standard types. When printing something, wouldn't it be nice if you could write the following?

FrozenPizza fp(1, 2, 3);
std::cout << fp;

You can if you create a friend operator<<(), like so:

class FrozenPizza: public FrozenFood {
    ...
    public:
    friend std::ostream &operator<<(std::ostream &os, const FrozenPizza &fp);
};

std::ostream &operator<<(std::ostream &os, const FrozenPizza &fp) {
    os << "\tFrozen Pizza Info:\n"
          "\t\tWeight: " << fp.GetWeight() << " ounces\n"
          "\t\tDiameter: " << fp.GetDiameter() << " inches\n";
}

The advantage is that you can now also "print" to different streams beside std::cout, like to a std::stringstream or a std::ofstream, making it much more generic.

Mark constants with const

In your classes, you initialize the member variables with some value, after which they can never be changed. In that case, make those member variables const, so that this is also clear to the compiler, which can then better optimize your code, and give an error if you accidentily do write to those variables:

class Frozenfood {
    const int Price;

protected:
    const int Weight;

public:
    ...
};

To be able to set these const variables in the constructor, use member intialization lists:

FrozenFood::FrozenFood(int APrice, int AWeight):
        Price{APrice},
        Weight{AWeight} {
}

I see you already use member initializer lists when initializing the base class, which is good.

Mark member functions that do not modify any member variables as const

Apart from making member variables const, you can also make member functions const. This tells the compiler that these functions do not change any member variables, which allows it to generate better code, produce errors if you do accidentily make changes, and allows those functions to be called on const objects. So for example:

class FrozenFood {
    ...
    int GetPrice() const;
    int GetWeight() const;
};

int GetPrice() const {
    return Price;
}

int GetWeight() const {
    return Weight;
}

Consider using #pragma once

You are using header guards correctly. However, it can be a chore to add #ifndef..#define..#endif, and it is easy to accidentily copy&paste the header guard and forget to change the name. All major compilers support #pragma once. If you add that to the header files, the compiler will ensure it's only ever included once.

However, as pointed out by others, it is not standard C++ (yet). Also, C++20 introduces modules, which avoids the need for header guards.

| improve this answer | |
\$\endgroup\$
  • 7
    \$\begingroup\$ #pragma once isn't in the standard yet, so I don't recommend it over header guards. Most but not all compilers can use it which may affect portability. \$\endgroup\$ – pacmaninbw Jun 28 at 21:56
  • \$\begingroup\$ Wow a lot here for me to look into, thank you so much! I went ahead and marked your response as the answer, since it had a bit more to it and had good tips to improve performance as well as format. Y'all are awesome thank you so much!! \$\endgroup\$ – avghdev Jun 29 at 3:16
5
\$\begingroup\$

Code structure

  • The code in the file main.cpp only operates on FDeepDishPizza class, so the name sounds misleading to me. It feels like main.cpp would be working on the base class FrozenFood, instantiating its derived classes and calling their public functions, which it doesn't do.

  • A trivial constructor which only initialises the member variables can be in header file, instead of the implementation.

      # FDeepDishPizza.h
      FDeepDishPizza(int APrice, int AWeight, int ADiameter, int AHeight); #can be expanded here itself, instead of in FDeepDishPizza.cpp .  
    

Use of Caps

class FrozenFood {
private:
    int Price;
}
....
FDeepDishPizza(...)
  • Capitalised names indicate classes, while all small letters indicate variables and functions. The latter two are the ones you'd be typing a lot, so better keep shift key out of the way.

  • Also, private members can be prefixed with _, like _price to have them stand out if they're being modified.

#ifndef vs pragma once

Note Some implementations offer vendor extensions like #pragma once as alternative to include guards. It is not standard and it is not portable. It injects the hosting machine’s filesystem semantics into your program, in addition to locking you down to a vendor. Our recommendation is to write in ISO C++: See rule P.2.

http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rs-guards

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Great suggestions here, thank you! I copied the syntax more or less from the book, and I was wondering if there was a reason they kept capitalizing the member variables! I guess it was just overlooked. I also didn't know about the "pragma once" stuff so that'll be awesome to look into :) \$\endgroup\$ – avghdev Jun 29 at 3:14
  • \$\begingroup\$ "Capitalised names indicate classes, while all small letters indicate variables and functions" This is highly dependent on style. Some places use the style in the OP, with everything in PascalCase. Other places use the style in the standard library, with everything in lower_snake_case. It's incorrect to state anything definitive about casing other than that you should stick to a particular one, because there is such a large variance in style. \$\endgroup\$ – Justin Jun 29 at 16:40
  • \$\begingroup\$ @Justin I did put it as an individual preference: that's why I mentioned the shift key. But yes it's not a golden rule. It's what happening where I'm contributing to, and may not happen where you are. But OP, while starting out, should be typing comfortably. \$\endgroup\$ – anki Jun 29 at 17:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.