Kalkulator z zabezpieczeniem strumienia wejścia - prośba o komentarz

pytanie zadane 24 marca 2017 w C i C++ przez Justyna33 Nowicjusz (220 p.)

Kalkulator to mój pierwszy "poważny" projekt. Uczę się programowania od 2 tygodni z materiałów dostępnych online.

Jeśli ktoś z Państwa zechciałby zagłębić się w napisany przeze mnie kod i skomentować jego jakość, będę wdzięczna.

Kalkulator wykonuje podstawowe działania i ma zabezpieczone dane wejściowe.

Z góry dziękuję :)

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

using namespace std;
double liczba, liczba2, wynik;
int dzialanie;
int main()
{
cout << "Witaj w kalkulatorze :)" <<endl<<endl; Sleep(500);
do
{ cout<<"Podaj pierwsza liczbe: "; cin>>liczba; bool a;
    if ((a=cin.fail())==true)
    {   do
        {
            system( "cls" ); cout<<"Blad "<<endl;
            cin.clear(); cin.sync();
            cout<<"Podaj pierwsza liczbe: "; cin>>liczba;
        }
        while  ((a=cin.fail())==true);
    }

 cout<<"Podaj  druga   liczbe: "; cin>>liczba2; bool b;
    if ((b=cin.fail())==true)
    {   do
        {
            cout<<"Blad "<<endl;
            cin.clear(); cin.sync();
            cout<<"Podaj  druga   liczbe: "; cin>>liczba2;
        }
         while  ((b=cin.fail())==true);
    }

cout<<endl<<"Wybierz dzialanie"<<endl;
cout<<"1. Dodawanie"<<endl<<"2. Odejmowanie"<<endl<<"3. Mnozenie"<<endl<<"4. Dzielenie"<<endl<<"5. Koniec programu"<<endl<<endl;
cout<<"Wybrano dzialanie nr:";
cin>>dzialanie; bool c;

if ((c=cin.fail())==true|| dzialanie>=6)
    {do
        {
            cout<<"Blad "; cin.clear(); cin.sync(); cout<<endl<<"Wybierz dzialanie:";
            cin>>dzialanie;
        }
        while((c=cin.fail())==true|| dzialanie>=6);
    }

cout<<endl;
switch(dzialanie)
{
case 1:
    {
    wynik=liczba+liczba2;
    cout<<liczba<<" + "<<liczba2<<" = "<<wynik;
    getch();system ("cls");
    break;
    }
case 2:
    {
    wynik=liczba-liczba2;
    cout<<liczba<<" - "<<liczba2<<" = "<<wynik;
    getch();system ("cls");
    break;
    }
case 3:
    {
    wynik=liczba*liczba2;
    cout<<liczba<<" * "<<liczba2<<" = "<<wynik;
    getch();system ("cls");
    break;
    }
case 4:
    {
        if ((b=cin.fail())==true || liczba2==0)
            { do
                {
                    cout<<"Blad "<<endl; cin.clear(); cin.sync(); cout<<"Podaj druga liczbe:";cin>>liczba2;
                }
              while((b=cin.fail()==true)|| liczba2==0);
            }
    wynik=liczba/liczba2;
    cout<<liczba<<" / "<<liczba2<<" = "<<wynik;
    getch();system ("cls");
    break;
    }
case 5:
    {
    cout<<"Koniec programu";
    break;
    }
}
}
while(dzialanie!=5);
    return 0;
}

3 odpowiedzi

odpowiedź 24 marca 2017 przez Sedi Stary wyjadacz (10,200 p.)

Fajnie, że chciało Ci się pisać kalkulator. To co mógłbym w nim poprawić, to :

-zmienne globalne

-liczby mógłbyś zastąpić znakami, przez co kod byłby nieco bardziej przyjazny

- Zamiast cin.fail(), dobrym pomysłem byłoby użycie użycie (!cin), który sprawdza, czy w strumieniu wszystko jest w porządku

- cin.sync(), można zastąpić lepszą std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n'), kóry ignoruje dane aż do długości linii

- Masz bardzo dużą redundację i to czasami normalne. Równocześnie w Twoim przypadku duża część instrukcji można zamknąć w funkcji, ot np. wczytywanie danych, lub ich przypisywanie

- Twój kod zadziała tylko dla Windowsa(windows.h,conio.h)

system("cls");

- Nie wiem, czy jest to dobrym pomysłem. System jest wolne, wymaga spawnowania innego procesu, uruchomiania shella, wykonania skryptu startowego shella, po czym wywołania faktycznej komendy, deinicjalizacji wszystkiego i zamknięciu procesu. To co jest istotne, to fakt że antywirusy mogą traktować to jako wirusa, gdyż znane są przypadku błędów bezpieczeństwa związanych z użyciem system(). Poza tym czyszczenie ekranu moim zdaniem jest nie w porządku, a zwłaszcza w kalkulatorze. Wyobraź sobie, że liczysz coś. To coś jest na górze ekranu i nagle znika. W moim odczuciu nie jest to do końca ciekawe.

Sleep(500);

-W moim odczuciu przy dłuższym użytkowaniu, będzie to raczej męczyło koniecznością czekania nawet tej pół sekundy za każdym razem

- Ostatnią kwestią, którą chciałbym poruszyć, to używanie przestrzeni nazw standardowych, czyli std. Oczywiście to tylko wspomnienie, gdyż w przypadku tak prostych programów, problem nie występuje.

Podsumowując: Mimo tej krytyki, napisanie takiego programu po 2 tygodniach jest naprawdę bardzo dobrym wynikiem, nazywasz zmienne po imieniu(a nie np, a,b,c), co jest naprawdę bardzo dobrym rezultatem. Tak trzymać!

komentarz 24 marca 2017 przez Justyna33 Nowicjusz (220 p.)

Bardzo dziękuję za obszerny komentarz.

A ponieważ wielu zagadnień w nim zawartych nie rozumiem, będzie doskonałą motywacją do dalszej pracy :)

komentarz 24 marca 2017 przez Sedi Stary wyjadacz (10,200 p.)

Jeśli chcesz, żebym coś wyjaśnił, z przyjemnością to zrobię. Zwłaszcza dla osoby, która umie przyjąć krytykę własnego programu(co jest rzadkością) :)

komentarz 24 marca 2017 przez Justyna33 Nowicjusz (220 p.)

Dziękuję

odpowiedź 24 marca 2017 przez Knayder Nałogowiec (37,640 p.)

Oprócz tego co padło wyżej, nadmieniłbym jeszcze:

Najlepiej od początku pisać po Angielsku, wejdzie to już w nawyk i nie będzie problemów z późniejszym przejściem z Polskiego na Angielski (Co jest wymagane w większości, jeżeli nie wszystkich miejscach pracy).
Musisz też pamiętać o tym, że kod nie jest lepszy, jeżeli zajmuje możliwie najmniej linijek. Jedna linijka, jedno działanie.
Przykład:
```
cout<<"Blad "; cin.clear(); cin.sync(); cout<<endl<<"Wybierz dzialanie:";
```
To jest bardzo niechlujne.
Lepiej to rozdzielić na pojedyncze linie:
```
cout<<"Blad ";
cin.clear();
cin.sync();
cout<<endl<<"Wybierz dzialanie:";
```
Tak samo, staraj się nie "Zasłaniać" klamer.
Przykład:
```
if ((a=cin.fail())==true)
    {   do
```
W twoim przypadku, pętla do...while zaczyna się w tej samej linijce, w której znajduje się klamra if'a.
Staraj się trzymać przyjętej konwencji, jeżeli chodzi o klamry itp.

Pozdrawiam.

Pokaż 8 poprzednich komentarzy

komentarz 24 marca 2017 przez j23 Mędrzec (194,920 p.)

Zmiana typu na char dla zmiennej operation spowodowała, że w przypadku błędnej danej nie wczytuje się pętla (56 linijka kodu).

Tak to widzę:

cout<<"Wybrano dzialanie nr:";

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

while(operation < '1' || operation > '5')
{
	cout << "Blad\nWybierz dzialanie: ";
	cin >> operation;
	cin.ignore(numeric_limits<streamsize>::max(), '\n');
}

cout << '\n';

switch(operation)
...

Jeśli chodzi o warunek w case'ie czwartym, to nie do końca o to mi chodziło. On tam powinien być, tak jak powinna być możliwość wprowadzania wartości zero dla obu operandów. Chodziło mi o bezsensowną konstrukcję warunku !number2 || number2 == 0, przecież wystarczy number2 == 0. I zamiast w warunku dobierać nowy dzielnik, wypisz po prostu komunikat o dzieleniu przez zero.

komentarz 24 marca 2017 przez Justyna33 Nowicjusz (220 p.)

Dziękuję za podpowiedzi. Problem rozwiązany.

komentarz 24 marca 2017 przez mokrowski Mędrzec (155,460 p.)
edycja 24 marca 2017 przez mokrowski

Na obecnym etapie bez znajomości zagadnień typu funkcja oraz zmienna globalna, raczej niewiele więcej zrobisz. Masz kandydatów na wydzielenie do funkcji w postaci operacji (w case), podawania liczb oraz prezentacji opcji do wyboru. Wprawdzie zawsze da się coś jeszcze "wycisnąć" ale to już naprawdę będą szczegóły np. można tak:

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

Z tego co jeszcze mogę podpowiedzieć, w swoim IDE znajdź opcję automatycznego formatowania kodu. Wybierz jedną z proponowanych konwencji (która Ci będzie odpowiadała) i ją stosuj.

komentarz 24 marca 2017 przez Justyna33 Nowicjusz (220 p.)

Dziękuję za "słowo" komentarza.

Myślę, że mogę już skupić się na kolejnych zagadnień (w planie mam teraz podstawy funkcji i tablice). Do kalkulatora z pewnością wrócę, kiedy nauczę się czegoś, co wyraźnie poprawi mój kod. Chyba nie ma teraz sensu tracić czasu na " zabiegi kosmetycznetyczne".

komentarz 24 marca 2017 przez mokrowski Mędrzec (155,460 p.)

Kod jest wystarczająco dobry na tym etapie nauki. Idź dalej :-)

odpowiedź 27 marca 2017 przez Justyna33 Nowicjusz (220 p.)

W kodzie jest jeszcze jeden błąd, z którym nie potrafię sobie poradzić. Liczba "0" jest uznawana za błąd.

Bardzo proszę pomoc.

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

using namespace std;
double number, number2, score;
char operation;
int main()
{
    cout << "Witaj w kalkulatorze :)\n\n";
    do
        { cout<<"Podaj pierwsza liczbe: ";
          cin>>number;
            while(!number)
                {
                            cout<<"Blad \n";
                            cin.clear();
                            cin.ignore(numeric_limits<streamsize>::max(), '\n');
                            cout<<"Podaj pierwsza liczbe: ";
                            cin>>number;
                }

            cout<<"Podaj  druga   liczbe: ";
            cin>>number2;

            if (!number2)
                {
                    do
                        {
                            cout<<"Blad \n";
                            cin.clear();
                            cin.ignore(numeric_limits<streamsize>::max(), '\n');
                            cout<<"Podaj  druga   liczbe: "; cin>>number2;
                        }
                    while  (!number2);
                }

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

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

                while(operation < '1' || operation > '5')
                    {
                        cout << "Blad\nWybierz dzialanie: ";
                        cin >> operation;
                        cin.ignore(numeric_limits<streamsize>::max(), '\n');
                    }

            cout << '\n';
            switch(operation)
            {
                case '1':
                        {
                            score=number+number2;
                            cout<<number<<" + "<<number2<<" = "<<score<<"\n\n";
                            break;
                        }
                case '2':
                        {
                            score=number-number2;
                            cout<<number<<" - "<<number2<<" = "<<score<<"\n\n";
                            break;
                        }
                case '3':
                        {
                            score=number*number2;
                            cout<<number<<" * "<<number2<<" = "<<score<<"\n\n";
                            break;
                        }
                case '4':
                        {
                            score=number/number2;
                            cout<<number<<" / "<<number2<<" = "<<score<<"\n\n";;
                            break;
                        }
                case '5':
                        {
                            cout<<"Koniec programu";
                            break;
                        }
            }
        }
    while(operation!=5);
    return 0;
}

komentarz 27 marca 2017 przez Molester Bywalec (2,920 p.)

Zrób tak, przy okazji pare rzeczy poprawiłem sama się zapoznaj z nimi.

#include <iostream>
#include <cstdlib>
#include<limits>
 
using namespace std;
double number, number2, score;
char operation;
int main()
{
    cout << "Witaj w kalkulatorze :)\n\n";
    do
        {
         cout<<"Podaj pierwsza liczbe: ";
            
          while(!(cin>>number))
                {
                            cout<<"Blad \n";
                            cin.clear();
                            cin.ignore(numeric_limits<streamsize>::max());
                            cout<<"Podaj pierwsza liczbe: ";
                            cin>>number;
                }
 
            cout<<"Podaj  druga   liczbe: ";
            
            while (!(cin>>number2))
            {
                
            cout<<"Blad \n";
            cin.clear();
            cin.ignore(numeric_limits<streamsize>::max(), '\n');
            cout<<"Podaj  druga   liczbe: "; cin>>number2;    
            }
            
            
            cout<<endl<<"Wybierz dzialanie:\n";
            cout<<"1.Dodawanie "<<"\t";
            cout<<"2.Odejmowanie "<<"\t";
            cout<<"3.Mnozenie "<<"\t";
            cout<<"4.Dzielenie "<<"\t";
            cout<<"5.Wyjscie \n\n";
 
            cout<<"Wybrano dzialanie nr:";
            cin>>operation;
            cin.ignore(numeric_limits<streamsize>::max(), '\n');
 
                while(operation < '1' || operation > '5')
                    {
                        cout << "Blad\nWybierz dzialanie: ";
                        cin >> operation;
                        cin.ignore(numeric_limits<streamsize>::max(), '\n');
                    }
 
            cout << '\n';
            switch(operation)
            {
                case '1':
                        {
                            score=number+number2;
                            cout<<number<<" + "<<number2<<" = "<<score<<"\n\n";
                            break;
                        }
                case '2':
                        {
                            score=number-number2;
                            cout<<number<<" - "<<number2<<" = "<<score<<"\n\n";
                            break;
                        }
                case '3':
                        {
                            score=number*number2;
                            cout<<number<<" * "<<number2<<" = "<<score<<"\n\n";
                            break;
                        }
                case '4':
                        {
                            if (number==0 || number2==0)
                                exit (0);
                                
                            score=number/number2;
                            cout<<number<<" / "<<number2<<" = "<<score<<"\n\n";;
                            break;
                        }
                case '5':
                        {
                            cout<<"Koniec programu";
                            break;
                        }
            }
        }
    while(operation!=5);
    return 0;
}