Locker Manager app c++

Question

good afternoon. I'm learning c++ and I made this app to "manage" lockers. I would like you to review it please, you can find the source code at my github: https://github.com/VitalZero/lockermanager

The app is made up by 3 main clases:

• CDocument: Takes care of file input/output
• CLockers: The lockers itself, stores it's info
• CLockManager: Takes care of the lockers and the document

Also, there's a "helper" class I made to show the functionality, SimpleMenu, that is not part of the main "program"

To use it as is, just create an instance, feed the file name and thats it:

#include "CLockManager.h"
#include "SimpleMenu.h"

int main()
{
  CLockManager manager("database.txt");
  SimpleMenu menu(manager);

  menu.ShowMenu();

  return 0;
}

File CDocument.h:

#ifndef CDOCUMENT_H_
#define CDOCUMENT_H_
/*
 * CDocument.h
 * It is used by CLockManager.
 * Just used to load and save data from a file using a vector of CLockers
 * CLockers overloads stream operators >> and <<
 * Just feed a vector of CLockers and it's all good!
 */
#include <fstream>
#include <string>
#include <vector>
#include "CLockers.h"

class CDocument{
public:
    CDocument() = delete;
    CDocument(const std::string& path_in)
        :
        path(path_in)
    {
    }
    ~CDocument()
    {
        if(inFile.is_open())
            inFile.close();
    }
    bool SaveData(std::vector<CLockers>& lockers);
    bool LoadData(std::vector<CLockers>& lockers);

private:
    std::fstream inFile;
    std::string path;
};

#endif /* CDOCUMENT_H_ */

File CDocument.cpp

#include "CDocument.h"

bool CDocument::SaveData(std::vector<CLockers>& lockers)
{
    inFile.open(path.c_str(), std::ios_base::out | std::ios_base::trunc);

    if(inFile.is_open())
    {
        for(unsigned int i = 0; i < lockers.size(); ++i)
        {
            inFile << lockers.at(i);
        }
        inFile.close();
        return true;
    }

    return false;
}

bool CDocument::LoadData(std::vector<CLockers>& lockers)
{
    inFile.open(path.c_str(), std::ios_base::in);

    if(inFile.is_open())
    {
        CLockers tmp;
        lockers.clear();

        while( (inFile.good()) && (inFile >> tmp) ) // here is the problem from CLockers, even though is being checked in here
        {                                           // it gives another extra loop after the fail bit is set
            lockers.push_back(tmp);
        }

        inFile.close();

        return true;
    }
    else
    {
        // Probably move this out to menu.cpp
        // interaction only should occur outside the class
        std::cout << "\tThe file "
                << path << " doesn't exist or can't be opened.\n";
        std::cout << "Do you want to create it? y/n: ";
        std::string stringAnswer;
        std::getline(std::cin, stringAnswer);

        if( (stringAnswer == "y") || (stringAnswer == "Y") )
        {
            inFile.open(path, std::ios_base::out | std::ios_base::trunc);
            inFile.close();
            return true;
        }
    }
    return false;
}

Thank you in advance! and any suggestion/correction is highly appreciated!. Greetings.

Answer 1

Close on destruction

You shouldn't need to explicitly close your file member on destruction. Since it's a safe variable and not an unsafe pointer, fstream's own destructor will be called and the file will be closed.

Const members

You should make path const, since it doesn't change over the course of the object's lifetime.

File object scope

inFile is not opened in the constructor, and it's always used in open/close pairs in your methods. That means that it shouldn't be a member at all; just use local variables. One benefit is that you won't need to explicitly close those files; when the variable goes out of scope the file will be closed.

Error handling

If you want to drink more of the OOP kool-aid, you shouldn't be using boolean return values, which is a pattern inherited from C code. Instead, your methods would be void-typed, and you'd throw an exception if something goes wrong.