Kalkulator z wykorzystaniem funkcji - prośba o komentarz

Question

Mój pierwszy kalkulator ewoluował. W obecnej wersji wykorzystałam funkcje.

Bardzo proszę o uwagi.

#include <iostream>
#include <cstdlib>
#include<conio.h>
#include<windows.h>
#include<limits>

using namespace std;

double number1, number2;
int operation;

double testNumber1(double n1), testNumber2(double n2);
int test3(int choice);
void addition(double n1,double n2),subtraction(double n1,double n2);
void multiplication(double n1,double n2),division(double n1, double n2);


int main()
{
    cout << "Witaj w kalkulatorze :)\n\n";
    do
    {
        cout<<"Podaj pierwsza liczbe: ";
        while(!(cin>>number1))
        {
            testNumber1(number1);
        }

        cout<<"Podaj  druga   liczbe: ";
        while (!(cin>>number2))
        {
            testNumber2(number2);
        }

        cout<<endl<<"Wybierz dzialanie:\n"
            <<"1.Dodawanie "<<"\t"<<"2.Odejmowanie "<<"\t"<<"3.Mnozenie "<<"\t"<<"4.Dzielenie "<<"\t"<<"5.Wyjscie \n\n";

        cout<<"Wybrano dzialanie nr:";
        cin.ignore(numeric_limits<streamsize>::max(), '\n');

        while ((!(cin>>operation)) ||operation < 1 || operation > 5)
        {
            test3(operation);

        }

        cout << '\n';

        switch(operation)
        {
        case 1:
        {
            addition(number1,number2);
            break;
        }
        case 2:
        {
            subtraction(number1,number2);
            break;
        }
        case 3:
        {
            multiplication(number1,number2);
            break;
        }
        case 4:
        {
            division(number1,number2);
            break;
        }
        case 5:
        {
            cout<<"Koniec programu";
        }
        }
    }
    while(operation!=5);
    return 0;
}


double testNumber1(double n1)
{
    cout<<"Blad \n";
    cin.clear();
    cin.ignore(numeric_limits<streamsize>::max(), '\n');
    cout<<"Podaj pierwsza liczbe: ";
    return n1;
}

double testNumber2(double n2)
{
    cout<<"Blad \n";
    cin.clear();
    cin.ignore(numeric_limits<streamsize>::max(), '\n');
    cout<<"Podaj  druga   liczbe: ";
    return n2;
}

int test3(int choice)
{
    cout << "Blad\nWybierz dzialanie: ";
    cin.clear();
    cin.ignore(numeric_limits<streamsize>::max(), '\n');

    return choice;
}

void addition(double n1,double n2)
{
    double result=n1+n2;
    cout<<n1<<" + "<<n2<<" = "<<result<<"\n\n";
}

void subtraction(double n1,double n2)
{
    double result=n1-n2;
    cout<<n1<<" - "<<n2<<" = "<<result<<"\n\n";
}

void multiplication(double n1,double n2)
{
    double result=n1*n2;
    cout<<n1<<" * "<<n2<<" = "<<result<<"\n\n";
}

void division(double n1, double n2)
{
    if (n2==0)
    {
        cout<<"Blad. Nie dzielimy przez zero \n"
            <<"Podaj druga liczbe:";
        cin.clear();
        cin.ignore(numeric_limits<streamsize>::max(), '\n');
        cin>>n2;
    }
    double result=n1/n2;
    cout<<n1<<" / "<<n2<<" = "<<result<<"\n\n";
}

 

Answer 1

Pytasz o uwagi. To prosty program ale wystarczająco złożony by zauważyć kilka rzeczy. Możesz się nie zgadzać z sugestiami. Przemyśl je jednak (nawet jeśli się z nimi nie zgadzasz :-) ).

Rzeczy do poprawienia:

1. Użycie zmiennych globalnych. Zmienne globalne to zło. W tym prostym programie przenieś je do funkcji main().

2. Zła maniera deklarowania kilku zmiennych i funkcji w jednej linii. Nie rób tak. Nawet w prostych programach.

3. Powtórzenie kodu funkcji testNumber1 i testNumber2. One różnią się tylko komunikatem. Komunikat przekaż jako argument funkcji.

4. Główna logika aplikacji wygląda raczej tak: 

• Pobierz 1 wartość
• Pobierz 2 wartość
• Pobierz operacje
• Wykonaj operację
• Wróć do początku.

Dopiero w przypadku szczególnym wyboru opcji zakończenia programu, następuje opuszczenie głównej pętli.

W związku z tym:

a) Przenieś kompletne pobranie 1 czy 2 wartości do funkcji. Niech samodzielnie poprawnie ją pobierze i zwróci do głównej części programu.

b) Wydziel funkcję pobrania działania. Dopiero w przypadku pobrania działania kończącego w main() sprawdzaj warunek opuszczenia pętli głównej.

c) Wydziel wykonanie operacji do oddzielnej funkcji z przekazaniem argumentu wyboru.

5. Zwróć uwagę że w funkcji division, masz podobny kod do wykrywania błędu jak w poprzednich. Wydziel to do funkcji.

6. Nieuzasadnione użycie nagłówków nieprzenośnych z MS Windows.

7. Nie będę się upierał. Być może jeszcze nie wiesz jak to zrobić. switch/case można łatwo uniknąć stosując tablicę funkcji. Instrukcje switch/case powodują w dłuższej perspektywie utrzymania programu zaciemnienie kodu i jego komplikację. No ale zakładam że się chcesz ich nauczyć :-)

Comments

Bardzo dziękuję za wszystkie uwagi.

Zabieram się do ich analizy i wprowadzania zmian w kodzie, co zapewne będzie cenną nauką.

C++ uczę się od kilku tygodni z materiałów dostępnych w sieci i mam pełną świadomość, że mój kod daleki jest od ideału. Dlatego każda uwaga i wskazówka są bardzo cenne.

Punkt 1, 2 ,3-, 5 -  bezdyskusyjne, pozostaje tylko zastosować i stosować na przyszłość.

Punkt 4 a  -  Próbowałam wprowadzić takie rozwiązanie, ale pojawił się problem. W przypadku wprowadzenia złej danej (np. litery) i poprawieniu jej na cyfrę, do funkcji głównym zwracana była wartość zero. Nie potrafiłam niestety go rozwiązać.                      

Punkt 4 b - Nie do końca rozumiem jak się do tego zabrać, ale powalczę z tematem.        

Punkt 4 c - Czy takie działanie spowoduje przeniesienie switch/case z funkcji głównej do osobnej funkcji? Tak właśnie to zrozumiałam, ale nie jestem pewna.                                

Punkt  6 - Już spotkałam się z tą informacją, ale wyszłam z założenia, że nie wszystko na raz. Samodzielna nauka jest bardzo czasochłonna i żmudna. Temat właściwego stosowania bibliotek jeszcze przede mną,ale mam świadomość jakie są w tej chwili ograniczenia mojego kodu.                                                                                          

Punkt 7 - Nie rozważałam takiej możliwości, ale widzę, że to bardzo logiczne. Zadanie miało na celu m.in. zastosowanie switch/case i póki co w tej wersji tak zostanie. Ale w następnym "projekciku" spróbuję zastosować tablicę funkcji.

Jeszcze raz bardzo dziękuję za uwagi.

Pozdrawiam 

 

---

Nie cyzelowałem już wszystkiego ale jest chyba porządniej. Zerknij jak... podejmiesz próbę poprawienia tego co wyżej:

#include <iostream>
#include <limits>

using namespace std;

double getNumber(const char* msg);
void inputError(const char* msg);
int getOperation();
void executeOperation(double number1, double number2, int operation);
void showOperation(double number1, double number2, double result, char operChar);
double addition(double n1, double n2);
double subtraction(double n1, double n2);
double multiplication(double n1, double n2);
double division(double n1, double n2);


int main() {
    double number1;
    double number2;
    int operation;
    cout << "Witaj w kalkulatorze :)\n\n";

    for(;;) {
        number1 = getNumber("Podaj pierwszą liczbę: ");
        number2 = getNumber("Podaj drugą liczbę: ");
        operation = getOperation();

        if(operation == 5) {
            break;
        }

        executeOperation(number1, number2, operation);
    }

    cout << "Koniec programu\n";
}

double getNumber(const char* msg) {
    std::cout << msg;
    double number;

    while(!(cin >> number)) {
        inputError(msg);
    }

    return number;
}

void inputError(const char* msg) {
    cout << "Bład \n";
    cin.clear();
    cin.ignore(numeric_limits<streamsize>::max(), '\n');
    cout << msg;
}

int getOperation() {
    int operation;
    cout << endl << "Wybierz działanie:\n"
         << "1.Dodawanie " << "\t" << "2.Odejmowanie " << "\t" << "3.Mnożenie "
         << "\t" << "4.Dzielenie " << "\t" << "5.Wyjście \n\n";
    cout << "Wybrano działanie nr: ";

    while((!(cin >> operation)) || operation < 1 || operation > 5) {
        inputError("Wybierz działanie: ");
    }

    cout << '\n';
    return operation;
}

void executeOperation(double number1, double number2, int operation) {
    double result;
    char operationChar;

    if(operation == 5) {
        return;
    }

    switch(operation) {
        case 1: 
                result = addition(number1, number2);
                operationChar = '+';
                break;
        case 2: 
                result = subtraction(number1, number2);
                operationChar = '-';
                break;
        case 3: 
                result = multiplication(number1, number2);
                operationChar = '*';
                break;
        case 4:
                result = division(number1, number2);
                operationChar = '/';
                break;
        default:
                ;
    }
    showOperation(number1, number2, result, operationChar);
}

void showOperation(double number1, double number2, double result, char operationChar) {
    cout << number1 << ' ' << operationChar << ' '
        << number2 << " = " << result << "\n\n";
}

double addition(double n1, double n2) {
    return n1 + n2;
}

double subtraction(double n1, double n2) {
    return n1 - n2;
}

double multiplication(double n1, double n2) {
    return n1 * n2;
}

double division(double n1, double n2) {
    while(n2 == 0) {
        inputError("Nie dzielimy przez zero \nPodaj drugą liczbę: ");
        cin >> n2;
    }

    return n1 / n2;
}

 

---

Już podejrzałam. Dużo zupełnie nowych dla mnie rzeczy. Teraz wiem przynajmniej w jakim kierunku zmierzać.

Jeszcze raz wielkie dzięki.

Answer 2

Bez zagłębiania się w szczegóły, odniosę się tylko do prototypów funkcji: Stosuj zasadę "jeden wiersz jedna funkcja". Zyskasz na przejrzystości. Będzie wyraźnie widać, która funkcja zwraca jaką wartość.

Answer 3

Wady:

-biblioteka windows

-przestrzen nazw

-nazwy zmiennych i funckji, poczytaj o czystym kodzie

-zmienne globalne

-sposob zapisu prototypu funckji

-uzywanie bibliotek na sile, owiele lepiej by to wygladalo gdybys uzyl minimalnej iloisci dodatkow

Plusy:

-rodzielenie funckji na cialo i prototyp

jak cos jeszcze znajde to dopisze

Comments

Dlaczego?

Answer 4

Perfect (poza drobną uwagą kolegi).

Chyba przyjdę na korepetycje!

Comments

Co tu jest perfect?

---

Nie rozumiem co uwazasz za perfekt? Widze 3 biblioteki:

#include<conio.h>
#include<windows.h>
#include<limits>

Z czego conio.h jest biblioteka nie przenosna, tak samo jak i windows.h. Nie widze sensu zycia biblioteki limits... Mozna samemu zainpletowac cos wlasnego, np. z notacja naukowa.

Nie uzywaj nawiasow przy case, bo nie jest to konieczne i pozniej mozna sie nawet zaplatac. 

---

Nie widze sensu zycia biblioteki limits...

 

cin.ignore(numeric_limits<streamsize>::max(), '\n');