Ocena kodu - waluty

Question

Cześć wszystkim!

Prosiłabym o ocene poniższego kodu. Konwertuje on podaną kwotę w PLN na daną walute i odwrotnie.

Rady i przykłady lepszego kodu mile widziane 

 

#include <iostream>
#include "waluty.h"
#include "funkcje.h"

using namespace funkcje;


int main()
{
	while(true)
	{
		switch (menu())
		{
		case 'a':
		{
			waluty* dolar = new USD;
			wybierz_czynnosc(dolar);
			delete dolar;
			break;
		}
		case 'b':
		{
			waluty* funt = new GBP;
			wybierz_czynnosc(funt);
			delete funt;
			break;
		}
		case 'c':
		{
			waluty* euro = new EUR;
			wybierz_czynnosc(euro);
			delete euro;
			break;
		}
		case 'd':
		{
			std::cout << "\nDo widzenia!\n";
			exit(0);
			break;
		}
		}
	} 

	return 0;
}

#include <iostream>
#include <ctype.h> 
#include <iomanip>

namespace funkcje
{
	char akcja()
	{
		using namespace std;
		cout << "Wybierz akcje: \n";
		cout << "a) konwertuj na walute\n";
		cout << "b) konwertuj na PLN\n";
		char ch;
		cout << "==> ";
		cin >> ch;
		return tolower(ch);
	}

	char menu()
	{
		using namespace std;
		cout << "Wybierz walute:\n";
		cout << "---------------------------\n";
		cout << "a) USD\n";
		cout << "b) GBP\n";
		cout << "c) EUR\n";
		cout << "d) zakoncz\n";
		char ch;
		cout << "==> ";
		cin >> ch;
		return tolower(ch);
	}

	void convert(waluty*object)
	{
			std::cout << "Podaj kwote w PLN: ";
			std::cin >> object->money;
			std::cout << std::setprecision(2) << object->konwertuj() << std::endl << std::endl;
			object->money = 0;
	}

	void to_PLN(waluty*object)
	{
			std::cout << "Podaj kwote w walucie: ";
			std::cin >> object->money;
			std::cout << std::setprecision(3)<<object->konwertuj_na_pln()<< std::endl<<std::endl;
			object->money = 0;
	}

	void wybierz_czynnosc(waluty* object)
	{
		switch (akcja()) {
		case 'a':
		{
			convert(object);
			break;
		}
		case 'b':
		{
			to_PLN(object);
			break;
		}
		}
	}
}

#pragma once
class waluty
{
public:
	double money = 0;
	virtual double konwertuj() = 0;
	virtual double konwertuj_na_pln() = 0;
};

class USD : public waluty
{
public:
	double konwertuj()
	{
		return money / 3.81;
	}
	double konwertuj_na_pln()
	{
		return money * 3.78;
	}
};

class GBP :public waluty
{
public:
	double konwertuj()
	{
		return money / 4.80;
	}
	double konwertuj_na_pln()
	{
		return money * 4.76;
	}
};

class EUR :public waluty
{
public:
	double konwertuj()
	{
		return money / 4.28;
	}
	double konwertuj_na_pln()
	{
		return money * 4.25;
	}
};

 

Answer 1

Tak na szybko:

- nie ma potrzeby, żeby alokować obiekty walut dynamicznie

- (w zasadzie nie ma potrzeby żeby te klasy trzymały liczby, równie dobrze to mogłyby być metody konwertuj(double))

- trochę dziwne, że wiedza o opcjach a/b/c/d jest podzielona między funkcją `menu()` a main'em.

- nie używaj exit(), wystarczyło `return 0`.

- trochę przemieszany język tego kodu między polskim i angielskim

- jeśli metody nie mutują obiektu, fajnie gdyby były `const`.

- rekomenduję dodać override i final do definicji klas dziedziczących po `waluty`

- to bardziej ciekawostka niż uwaga: w "prawdziwym" sofcie działającym na pieniądzach, zazwyczaj unika się przechowywania wartości pieniężnych w zmiennych typu zmiennoprzecinkowego (float, double), bo łatwo wtedy o przypadkowe błędy precyzji.

Comments

	void wybierz_czynnosc(/*const/* waluty* object)
	{
		switch (akcja()) {
		case 'a':
		{
			convert(object);    //jeżeli odkomentujemy const tu jest
                                //błąd        
			break;
		}
		case 'b':
		{
			to_PLN(object);     //tutaj również
			break;
		}
		}
	}
}

Da się tu dać const?

---

Miałem na myśli, od samej klasy:

class waluty
{
public:
    double money = 0;
    virtual double konwertuj() const = 0;
    virtual double konwertuj_na_pln() const = 0;
};
 
class USD final : public waluty
{
public:
    double konwertuj() override const
    {
        return money / 3.81;
    }

etc etc.

Tak gdzie Ty wpisałeś, obecnie nie można, bo `convert` zmienia pole klasy.