Skip to main content
added 1685 characters in body
Source Link
Loki Astari
  • 97.7k
  • 5
  • 126
  • 341

======

Logging

I would set up logging so that each action does its own logging. To do this it needs a logging object that knows what to do with the message. If you define your logging object as a class you can define different styles of logging.

class SimpleMessageLogger
{
    public:
        virtual ~SimpleMessageLogger() {}
        virtual void log(std::string const& message) = 0;
};

class SimpleMessageLoggerTOStdErr: public SimpleMessageLogger
{
    public:
        virtual void log(std::string const& message) override
        {
            std::cerr << time(nullptr) << ": " << message << "\n";
        }
}

class SimpleMessageLoggerTOSystem: public SimpleMessageLogger
{
    public:
        virtual void log(std::string const& message) override
        {
            // Call system logger
        }
}

Then your account needs to be set up to use a logger:

class Account
{
    SimpleMessageLogger& logger;
    int                  balance;

    public:
        Account(SimpleMessageLogger& logger)
            : logger(logger)
            , balance(0)
        {}

        void withdraw(int amount) {
            if (balance < amount) {
                logger.log("OverDrawn. We make money by charging fees");
            }
            balance -= amount;
            std::stringstream message;
            message << "Withdrawal: From account 1123 Amount: " << amount << " New Balance: " << balance;
            logger.log(message.str());
        }
}

int main()
{
    std::unique_ptr<SimpleMessageLogger> logger;
    // Decide what type of logger you want.
    logger = new SimpleMessageLoggerTOStdErr;

    Account. myAccount(*logger);

    myAccount.withdraw(1'000'000);
}

======

Logging

I would set up logging so that each action does its own logging. To do this it needs a logging object that knows what to do with the message. If you define your logging object as a class you can define different styles of logging.

class SimpleMessageLogger
{
    public:
        virtual ~SimpleMessageLogger() {}
        virtual void log(std::string const& message) = 0;
};

class SimpleMessageLoggerTOStdErr: public SimpleMessageLogger
{
    public:
        virtual void log(std::string const& message) override
        {
            std::cerr << time(nullptr) << ": " << message << "\n";
        }
}

class SimpleMessageLoggerTOSystem: public SimpleMessageLogger
{
    public:
        virtual void log(std::string const& message) override
        {
            // Call system logger
        }
}

Then your account needs to be set up to use a logger:

class Account
{
    SimpleMessageLogger& logger;
    int                  balance;

    public:
        Account(SimpleMessageLogger& logger)
            : logger(logger)
            , balance(0)
        {}

        void withdraw(int amount) {
            if (balance < amount) {
                logger.log("OverDrawn. We make money by charging fees");
            }
            balance -= amount;
            std::stringstream message;
            message << "Withdrawal: From account 1123 Amount: " << amount << " New Balance: " << balance;
            logger.log(message.str());
        }
}

int main()
{
    std::unique_ptr<SimpleMessageLogger> logger;
    // Decide what type of logger you want.
    logger = new SimpleMessageLoggerTOStdErr;

    Account. myAccount(*logger);

    myAccount.withdraw(1'000'000);
}
added 3440 characters in body
Source Link
Loki Astari
  • 97.7k
  • 5
  • 126
  • 341

All these methods on a class call Account.

A lot of methods don't seem to have anything to do with an account!

// Not sure what this is to do with an account?
void createFolder() const;
void createLimitFiles();
void deleteFile(std::string file_to_delete);
void settings();
void mainMenu();

The logging functions don't look like they belong in the account. But rather something that the account would use.

void logTransactions(const std::string& log_account_num, const std::string& transaction_type, const int& amount) const;
void log( bool note ) const;
void logInsideAccount(const std::string& in_account) const;

Do you call a method to withdraw information. Then call the logging functions to record that? When you withdraw someting from the account should the account not simply tell the logger about the action so it can be logged.

If that is the case then you need to pass a logging object to the account (probably in the constructor).


Modern Time formatting works with time and stream:

time_t now      = time(nullptr);
string time_now = ctime(&now);
tm* lock_time   = localtime(&now);


lock << lock_time->tm_mday << " " <<  lock_time->tm_hour << " " <<  lock_time->tm_min;

Can be replaced with:

auto now = std::chrono::system_clock::now();
lock << std::format("%e %I %M", now);

This is very verbose:

if ( ! file_in.good() )
{
    return;
}

A stream when used in a boolean context (like an if statement) will automatically convert itself to a bool based on its state (by calling good()).

if (!file_in) {
    return;
}

You are assuming that a line is less than 256 characters long.

char bufferLine[256];
file_in.getline(bufferLine, 256);

It may be true now. But in the future when your class is modified do you trust the next person to read all the code and make sure it conforms to all your current standards? If this is a limit you want the next programmer to inforce you should put it as a constant that is named in the class.

But alternatively I would use a resizable string so it can read any size of line.

std::string  line;
if (std::getline(file_in, line)) {
   // Correctly read a line of text from `file_in`
}

This is broken.

while (file_in.good() && ! file_in.eof())
{
    c = file_in.get();
    existent_content.push_back(c);
}

The situation. You have one character left in the stream and the file is good. The above condition is good and you enter the loop.

You will read the last character. The state of the stream is still good and the EOF will NOT be set. This is because the EOF flag is not set until you read past the end of file. So you enter the loop again. But this time when you try and read a character it fails (and sets the EOF flag). But you still unconditionally add it to existent_content.

You can write it like this but you need to test that the read worked:

while (file_in.good() && ! file_in.eof()) {
    int c;
    if ((c = file_in.get()) != EOF) {
        existent_content.push_back(c);
    }
}

But this is still considered bad practice. You shoudl loop on a read working.

int c;
while ((c = file_in.get()) != EOF) {
    existent_content.push_back(c);
}

All these methods on a class call Account.

A lot of methods don't seem to have anything to do with an account!

// Not sure what this is to do with an account?
void createFolder() const;
void createLimitFiles();
void deleteFile(std::string file_to_delete);
void settings();
void mainMenu();

The logging functions don't look like they belong in the account. But rather something that the account would use.

void logTransactions(const std::string& log_account_num, const std::string& transaction_type, const int& amount) const;
void log( bool note ) const;
void logInsideAccount(const std::string& in_account) const;

Do you call a method to withdraw information. Then call the logging functions to record that? When you withdraw someting from the account should the account not simply tell the logger about the action so it can be logged.

If that is the case then you need to pass a logging object to the account (probably in the constructor).


Modern Time formatting works with time and stream:

time_t now      = time(nullptr);
string time_now = ctime(&now);
tm* lock_time   = localtime(&now);


lock << lock_time->tm_mday << " " <<  lock_time->tm_hour << " " <<  lock_time->tm_min;

Can be replaced with:

auto now = std::chrono::system_clock::now();
lock << std::format("%e %I %M", now);

This is very verbose:

if ( ! file_in.good() )
{
    return;
}

A stream when used in a boolean context (like an if statement) will automatically convert itself to a bool based on its state (by calling good()).

if (!file_in) {
    return;
}

You are assuming that a line is less than 256 characters long.

char bufferLine[256];
file_in.getline(bufferLine, 256);

It may be true now. But in the future when your class is modified do you trust the next person to read all the code and make sure it conforms to all your current standards? If this is a limit you want the next programmer to inforce you should put it as a constant that is named in the class.

But alternatively I would use a resizable string so it can read any size of line.

std::string  line;
if (std::getline(file_in, line)) {
   // Correctly read a line of text from `file_in`
}

This is broken.

while (file_in.good() && ! file_in.eof())
{
    c = file_in.get();
    existent_content.push_back(c);
}

The situation. You have one character left in the stream and the file is good. The above condition is good and you enter the loop.

You will read the last character. The state of the stream is still good and the EOF will NOT be set. This is because the EOF flag is not set until you read past the end of file. So you enter the loop again. But this time when you try and read a character it fails (and sets the EOF flag). But you still unconditionally add it to existent_content.

You can write it like this but you need to test that the read worked:

while (file_in.good() && ! file_in.eof()) {
    int c;
    if ((c = file_in.get()) != EOF) {
        existent_content.push_back(c);
    }
}

But this is still considered bad practice. You shoudl loop on a read working.

int c;
while ((c = file_in.get()) != EOF) {
    existent_content.push_back(c);
}

added 674 characters in body
Source Link
Loki Astari
  • 97.7k
  • 5
  • 126
  • 341

Even Windows uses '/' as a path separator nowadays.

const string Paths::BASE_PATH = "C:\\ProgramData\\BankAcount";

The last time you needed to use '' as a path separator was 15 years ago.

const string Paths::BASE_PATH = "C:/ProgramData/BankAcount";

If you really want to us the '' then use the RAW character strings.

const string Paths::BASE_PATH = R"(C:\ProgramData\BankAcount)";

Member variables should always be private.

class Account
{
protected:
    std::string name;
    std::string new_name;
    long long int balance;
    std::string account_num;
    std::string file_path;

    std::string pin, new_pin;

    char buffer[256];

   // STUFF
};

Do you trust a stranger that derived his class from your class to main the invariants of the class?

Members should be private and then you provide methods to mutate the state in a way that is logically consistent.


OK. Have not read the whole class. But is this a string?

    char buffer[256];

Even Windows uses '/' as a path separator nowadays.

const string Paths::BASE_PATH = "C:\\ProgramData\\BankAcount";

The last time you needed to use '' as a path separator was 15 years ago.

const string Paths::BASE_PATH = "C:/ProgramData/BankAcount";

If you really want to us the '' then use the RAW character strings.

const string Paths::BASE_PATH = R"(C:\ProgramData\BankAcount)";

Even Windows uses '/' as a path separator nowadays.

const string Paths::BASE_PATH = "C:\\ProgramData\\BankAcount";

The last time you needed to use '' as a path separator was 15 years ago.

const string Paths::BASE_PATH = "C:/ProgramData/BankAcount";

If you really want to us the '' then use the RAW character strings.

const string Paths::BASE_PATH = R"(C:\ProgramData\BankAcount)";

Member variables should always be private.

class Account
{
protected:
    std::string name;
    std::string new_name;
    long long int balance;
    std::string account_num;
    std::string file_path;

    std::string pin, new_pin;

    char buffer[256];

   // STUFF
};

Do you trust a stranger that derived his class from your class to main the invariants of the class?

Members should be private and then you provide methods to mutate the state in a way that is logically consistent.


OK. Have not read the whole class. But is this a string?

    char buffer[256];

Source Link
Loki Astari
  • 97.7k
  • 5
  • 126
  • 341
Loading