GameState, is this a good practice?

Started by
11 comments, last by nunobarao 9 years, 3 months ago

I feel you better first learn the programming and its quirks, that will help you more than trying to find the best object oriented design.

- If you provide a "{}" block for the function you dont put ";".

- Never add a destructor if you dont provide copy and move constructors and operator= (follow rule of zero, three or five).

- Follow Dont Repeat Yourself principle. If you feel the need to add numbers to variable names use an array. If you need to repeatedly add the same data, use a loop over the array.

- Delete those useless comments you clutter the code with, anyone can see from the code what a constructor is or that getTitle gets the title. Learn to add useful comments about the big picture goal and why you choose to do something.

Advertisement
It would be more typical to model such types in terms of collections of other types, here is an overview of how that might look:

// Class book much like you had it

std::ostream &operator<<(std::ostream &out, const Book &book) {
    return out << '\"' << book.getTitle() << '\"' << ", by " << book.getAuthor();
}

#include "book.h"

class Shelf
{
public:
    Shelf(const std::string &label) : label(label)
    {
    }

    const std::string &getLabel() const;

    const std::vector<Book> &getBooks() const;

    void placeBook(const Book &book);

private:
    std::string label;
    std::vector<Book> books;
};

std::ostream &operator<<(std::ostream &out, const Shelf &shelf) {
    out << "Shelf labelled " << shelf.getLabel() << " contains:\n";
    for (const Book &book : shelf.getBooks()) {
        out << " * " << book << '\n';
    }
    return out;
}

#include "shelf.h"

class Furniture
{
public:
    Furniture(const std::string &description, const std::vector<Shelf> &shelves)
    : 
        description(description),
        shelves(shelves)
    {
    }

    const std::string &getDescription() const;

    const std::vector<Shelf> &getShelves() const;

private:
    std::string description;
    std::vector<Shelf> shelves;
};

std::ostream &operator<<(std::ostream &out, const Furniture &item) {
    out << "Furniture " << item.getDescription() << '\n';
    for (const Shelf &shelf : item.getShelves()) {
        out << " * " << shelf << '\n';
    }
    return out;
}

#include "furniture.h"

class Room
{
public:
    Room(const std::string &name) : name(name)
    {
    }

    const std::string &getName() const;

    const std::vector<Furniture> &getFurniture() const;

    void furnishWith(const Furniture &furniture);

private:
    std::string name;
    std::vector<Furniture> furniture;
};

std::ostream &operator<<(std::ostream &out, const Room &room) {
    out << "Room " << room.getDescription() << '\n';
    for (const Furniture &item : item.getFurniture()) {
        out << shelf << '\n';
    }
    return out;
}

Furniture buildBookShelf() {
    Shelf math("Math");
    for(int i = 0 ; i < 3 ; ++i) {
        math.placeBook(Book("The joy of math, volume " + std::to_string(i + 1), "Matty Mathews"));
    }

    Shelf informatics("Informatics");
    for(int i = 0 ; i < 5; ++i) {
        informatics.placeBook(Book("The joy of information, volume " + std::to_string(i + 1), "Ivy Inverness"));
    }

    std::vector<Shelf> shelves;
    shelves.push_back(math);
    shelves.push_back(informatics);
    return Furniture("Antique oak bookshelf", shelves);
}

Room prepareLibrary() {
    Room library("West facing library");
    Furniture bookshelf = buildBookShelf()
    library.furnishWith(bookshelf);
    return library;
}

int main() {
    Room room = prepareLibrary();
    std::cout << library << "\n";
}

I hope the implementations of the member functions is more or less obvious to you. I've not compiled that, it is possible that there are typos or other errors in the listing.

Couple of points to highlight:

First is that I didn't expose "setters" for all of the members. A common mistake many beginners make is assuming that every field must be exposed for reading and writing. Good OO code is often the opposite, the class exposes enough to be useful, but no more. I took a short cut above, and exposed the collections. Others would encapsulate this detail in various ways. Note how the operations to add to the collection were written in terms of the domain, you place books on a shelf, you furnish a room.

Also, see how we didn't need create different variables for the individual book objects, we create anonymous instances inside the loops and add them directly to the shelf.

Notice how an item of furniture does not have an operation to add a shelf - most pieces of furniture comes as they are and don't change over time. So to create a piece of furniture, you must first describe the shelves, then create the furniture from this description. It would be possible to retain the ability to modify the contents of the shelves, but the example was getting long enough as it is.

You can see already how you pass the objects to and from functions. I've used the C++ stream overloads for this, but that isn't necessary, and might not be a good idea - you might see why by attempting to print the shelves indented once, and the books on the shelf indented twice, while still retaining the ability to print an unindented book directly.

Many thanks too both of you.

This topic is closed to new replies.

Advertisement