1
\$\begingroup\$

What is your opinion on this simple school project exercising OOP in C++?

class Person {
public:
    Person() = default;
    explicit Person(std::string const& name);
    Person(const std::string& name, int age);
    void setAge(int age);
    void setName(std::string const& name);
    int getAge() const;
    const std::string& getName() const;

private:
    int age{};
    std::string name;
};
Person::Person(const std::string &name, int age) : age(age), name(name) {}

int Person::getAge() const {
    return age;
}

const std::string &Person::getName() const {
    return name;
}
void Person::setAge(int age) {
    Person::age = age;
}
Person::Person(std::string const& name) : name(name) {}
void Person::setName(std::string const& name) {
    Person::name = name;
}

class Client : public Person {
public:
    static int const uuidLen = 37;
    explicit Client(std::string const& name);
    Client(Client const& orig);
    Client& operator=(Client const& rhs);
    ~Client();

    std::string const& getId() const;
private:
    void copyId(char *p);
    char *_id;
    std::string id;
};
void Client::copyId(char*p) {
    _id = new char[Client::uuidLen];
    memcpy(_id, p, Client::uuidLen);
    id = _id;
}

Client::Client(std::string const& name) : Person(name) {
    _id = new char[Client::uuidLen];

    uuid_t uid;
    uuid_generate(uid);
    uuid_unparse(uid, _id);

    id = _id;
}

Client::Client(Client const& orig) : Person(orig.getName()) {
    copyId(orig._id);
    setName(orig.getName());
    setAge(orig.getAge());
}

Client& Client::operator=(Client const& rhs) {
    if(this != &rhs){
        delete[] _id;
        copyId(rhs._id);
        setName(rhs.getName());
        setAge(rhs.getAge());
    }
    return *this;
}

Client::~Client() {
    delete[] _id;
}
std::string const& Client::getId() const {
    return id;
}

class Account {
public:
    static int newNumber;

    explicit Account(Client const& owner);
    void setInterestRate(double interestRate);

    int getNumber() const;
    double getBalance() const;
    double getInterestRate() const;
    Client const& getOwner() const;

    void deposit(double amount);
    bool canWithdraw(double amount) const;
    bool withdraw(double amount);
    void addInterest();

private:
    int number{};
    double balance{};
    double interestRate{};
    Client owner;
};
int Account::newNumber = 0;

Account::Account(Client const& owner) : number(newNumber++), owner(owner), balance(0.0) {}
int Account::getNumber() const {
    return number;
}
double Account::getBalance() const {
    return balance;
}
double Account::getInterestRate() const {
    return interestRate;
}
Client const& Account::getOwner() const {
    return owner;
}
void Account::setInterestRate(double interestRate) {
    //given in percentage
    Account::interestRate = interestRate / 100;
}
bool Account::canWithdraw(double amount) const {
    return (getBalance() - amount) >= 0;
}
void Account::deposit(double amount) {
    balance += amount;
}
bool Account::withdraw(double amount) {
    if(canWithdraw(amount)){
        balance -= amount;
        return true;
    }
    return false;
}
void Account::addInterest() {
    balance += (getBalance() * getInterestRate());
}

class Bank {
public:
    Account& createAccount(Client const& client);
    double totalDeposit(std::string const& uuid) const;
    double deposit(Account& account, double amount);
    double withdraw(Account &account, double amount);
    double setInerest(Account &account, double interest);
    void chargeInterest(Account &account);
    void printStats() const;

private:
    std::vector<Client> clients;
    std::vector<Account> accounts;
};
Account& Bank::createAccount(Client const& client) {
    accounts.emplace_back(client);
    clients.push_back(client);
    return accounts.back();
}

double Bank::totalDeposit(std::string const& uuid) const {
    std::vector<Account> allAccounts;
    std::copy_if(accounts.begin(), accounts.end(), std::back_inserter(allAccounts),
                 [&uuid](Account const& a) { return uuid == a.getOwner().getId(); });
    double total = 0;
    for (Account const& a: allAccounts) {
        total += a.getBalance();
    }
    return total;
}
double Bank::deposit(Account& account, double const amount) {
    account.deposit(amount);
    return amount;
}
double Bank::withdraw(Account& account, double amount) {
    if (!account.withdraw(amount)) {
        throw std::exception();
    }
    return amount;
}
void Bank::chargeInterest(Account& account) {
    account.addInterest();
}
double Bank::setInerest(Account& account, double interest) {
    account.setInterestRate(interest);
    return interest;
}
void Bank::printStats() const {
    for(Client  const& c : clients){
        double total = totalDeposit(c.getId());
        std::cout << "client '" << c.getName() << "' "
        << "has deposit of: " << total << '\n';
    }
    std::cout << std::endl;
}

int main(){
    Client j("Josef");
    Client p("Petr");

    Bank bank;

    Account& a1 = bank.createAccount(j);
    Account& a2 = bank.createAccount(p);

    bank.deposit(a1, 8500);
    bank.printStats();
}
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

setAge, getAge, setName, getName are trivial getter/setters. This pattern is an often repeated mistake, and has nothing better than declaring age and name as public:, because they don't provide encapsulation and sanity checks at all.

I don't see any good reason to use char* as type of _id.

The destructor of Person should be virtual.

Your design of relationship between Account and Client has a big problem, because each Account should hold a reference of Client, so that when some Client information is changed, all Accounts that have owner as that Client should reflect that change.

I suggest something like this:

class Client {
// ...
    int client_id;
// ...
};


class Bank {
// ...
unordered_map<int, Client> clients; // holds (client id, client object) pairs
// ...

    const Client& getClient(int id) const {
        return clients.find(id)->second;
    }
};

class Account {
    Bank& bank; // reference

    const Client& getOwner() const {
        return bank.getClient(client_id);
    }
    
    int client_id;
};

```
\$\endgroup\$
3
  • \$\begingroup\$ And there was even error I did not even notice. In the main function, I am using reference to account Account& a1 = bank.createAccount(j), but after that, I created another client which invalidates the reference to the first one (since the vector reallocates its internal storage by the accounts.emplace_back(client). So I think I should use std::deque instead, if I want to use references to the container elements (since the std::deque does not invalidate if new memory is needed). That was big mistake \$\endgroup\$ Commented Mar 21, 2022 at 18:53
  • \$\begingroup\$ And for your points: 1)Using getters/setters may not now use any encapsulation, but they can in the future, making them public would disable this. 2) I would not use char *_id if I have not to, but I am getting the UUID from uuid_generate, which I have to pass buffer to store the result, this is the only option (and from that buffer, I create std::string). I do not know about possibility in which I would give ownership to std::string directly (without any temporary buffer). 3) making virtual destructor for child classes is good point, thanks for this. \$\endgroup\$ Commented Mar 21, 2022 at 18:55
  • \$\begingroup\$ 4) And yes, the overall design of the bank is not ideal (having 2 vectors for cliens and accounts separately). But that is mandatory from the school specs (And I doubt the reasoning for this decision as well). If I were to make my own choice over the bank design, I would probably go for std::unordered_map<std::string, Client> clients (mapping between UUID and the client) and std::unordered_map<Client, std::vector<Accounts>> accounts, since one client can have multiple Accounts. \$\endgroup\$ Commented Mar 21, 2022 at 19:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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