2
\$\begingroup\$

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.

\$\endgroup\$
  • 2
    \$\begingroup\$ While it's reasonable to add other code in a link to provide more context to tell us more about the usage of the code under review, you really need to put the code to be reviewed into the question. The question needs to contain enough that it remains a complete and meaningful question even when any external links die. \$\endgroup\$ – Jerry Coffin Sep 10 at 21:15
  • \$\begingroup\$ Thank you!, I added some more relevant code. I don't know of I can include the rest as is a lot of code (for me :) ). \$\endgroup\$ – Vital Zero Sep 10 at 21:32
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Hi!, thank you for your comments, I'll take into accout what you say, specially for fstream's objects. Now, as for the error handling. Most likely I'm using boolean values so I can return to some menu instead of exiting the program. Havent practiced exceptions that much, I'm not really sure if I can use that as handling boolean values in the functions. \$\endgroup\$ – Vital Zero Sep 11 at 17:28

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.