Prośba o ocenę kodu (zaawansowanego?) kalkulatora opartego na funkcjach i strukturach

pytanie zadane 3 października 2016 w C i C++ przez Konrad Nabożny Stary wyjadacz (13,460 p.)

Witam. W ramach samokształcenia i poćwiczenia struktur, napisałem według mnie dosyć ciekawy kalkulator w konsoli, działający nie na zasadzie podaj a podaj b co chcesz zrobić 1. dodaj 2 odejmij, tylko już przyjaznego sposobu typu: 16*4, 15+25 itp. Nie jestem pewien czy wszystko co robiłem było koniecznie wiec moją prośbę kieruję do ekspertów. Czy wykonałem to poprawnie i czy kod jest w miarę dopracowany i zoptymalizowany. Pozdrawiam serdecznie.

Kod: https://wklej.to/GUDho

1	komentarz 4 października 2016 przez manjaro Nałogowiec (37,390 p.) Jak widzę conio.h to od razu sobie odpuszczam taki kod...

komentarz 4 października 2016 przez Szahid Pasjonat (20,930 p.)

Możesz wyjaśnić dlaczego?

2	komentarz 4 października 2016 przez DragonCoder Nałogowiec (36,500 p.) Bo osoba piszaca na Linuxie, lub Macu nie moze odpalic kodu... conio.h jest nie przenosne, lepiej uzywac bibliotek multiplatformowych.... Kod da sie napisac krocej i w main wygladaloby to lepiej....

1	komentarz 4 października 2016 przez manjaro Nałogowiec (37,390 p.) Dokładnie tak.

komentarz 4 października 2016 przez DragonCoder Nałogowiec (36,500 p.)

A teraz prosba odemnie, rozbij to na 3 pliki, klasa.h klasa.cpp i main.cpp, mozesz na wiecej jesli chcesz, uzyj klas a zmienne zrob prywatne, nazywaj wszystko po angielsku, a zmienne musza miec sens

komentarz 4 października 2016 przez Konrad Nabożny Stary wyjadacz (13,460 p.)

Myślałem właśnie o rozbiórce każdego działania arytmetycznego na osobny plik, ale ostatecznie stwierdziłem że nie ma to sensu, bo kalkulator nie policzy dłuższych działań np. 8*4+2-6 itp, więc nawet się nie fatygowałem, tylko zacząłem myśleć nad rozwiązaniem tych rozszerzonych działań.

komentarz 4 października 2016 przez DragonCoder Nałogowiec (36,500 p.)

Po kij rozbijac kazde dzialanie na osobny plik? Co prawda mozna to zrobic jako funkcje... Ja tak zrobilem dla obliczenia twierdzenia sinusow i kosinusow, ale w kladie da sie wszystko zrobic.... Jak chcesz to Ci to moge podeslac

1 odpowiedź

odpowiedź 4 października 2016 przez criss Mędrzec (172,590 p.)
wybrane 4 października 2016 przez Konrad Nabożny

Najlepsza

Troche bez sensu dla każdego rodzaju działania są po 3 pola w strukturze. Bo w zasadzie wynika to tylko z nazw zależnie od działania. Wystaczyłoby czynnik1, czynnik2 i wynik.

Poza tym - na siłe próbujesz w strukturze upchnąć jakieś zmienne potrzebne w programie, ale nie związane bezpośrednio z Obliczeniami (nazwa powinna do czegoś zobowiązywać). Mówie o powtorzenie_petli. W efekcie tworzysz osobny obiekt Obliczenia tylko po to, żeby korzystać ze wspomnianego boola.

Podobna sytuacja: jeśli w strukturze już jest pole na wynik operacji, to po co tworzyć osobno poczatkowe_wartosci i dodawania, odejmowanie (...) ? To samo ze znakiem arytmetycznym. Osobny obiekt bez powodu. Niby obiekt typu Obliczenia ma skupiać w sobie wszystkie te dane, a ty i tak jedno działanie rozbijasz na kilka takich obiektów (z którego każdy wykorzystujesz tylko częściowo) i tym samym tracisz sens tej struktury.

Także ze względu na to bardzo pokrętnie napisany program. Niewiele więcej jest do oceniania, ale jeszcze pare słów nt. funkcji funkcja_... (swoją drogą nie bardzo jest sens tego "funkcja_" w nazwie funkcji - wiadomo, że to funkcja):

Nigdzie nie doszukałem się, żebyś skorzystał z wartości zwracanej przez te funkcje (poprzez return). Błąd jakotaki to nie jest, ale przejrzyściej jest przypisywać zmiennym wartości "normalnie" poprzez zmienna = f(); . Tym bardziej, że raczej powinieneś się zdecydować czy przypisujesz do zmiennej przekazanej przez ref czy zwracasz coś z funkcji i to przypisujesz do zmiennej.

Poza tym - nie bardzo jest sens przekazywać prymitywy przez referencje - ich kopiowanie nic nie kosztuje. A jeśli już bardzo chcesz, to niech to będzie const referencja. Zwykła referencja nie przyjmie rvalue, a const ref. przyjmie obie lvalue i rvalue. Tzn. w sytuacji jaka jest teraz u ciebie w kodzie:

To zadziała:

float a, b, c;
funkcja_dodawanie(a, b, c);

Ale to już nie:

float wynik;
funkcja_dodawanie(wynik, 2, 5);

Oczywiście mówie o const tylko dla 2. i 3. argumentu. Zmienna do której wynik ma zostać przypisany ofc const być nie może.

No i na koniec:

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

Nigdzie nie widze, żebyś korzystał gdzieś z którejś z tych bibliotek. Btw, skoro C++ to <cmath>, nie <math.h>.

komentarz 4 października 2016 przez Szahid Pasjonat (20,930 p.)

Zgadzam się. Ponad to gdzie w programie jest ustawiona początkowa wartość bool?

komentarz 4 października 2016 przez Konrad Nabożny Stary wyjadacz (13,460 p.)

1. Chciałem żeby wszystko było tak uporządkowane jak tylko mogło, dlatego do każdego działania użyłem trzech różnych zmiennych w strukturze.

2. Masz rację. Tak "logika" programu powinna być w innym miejscu niż struktura Obliczenia.

3. Te początkowe wartości tworzę dlatego że nie wiem jakie działanie poda użytkownik i dlatego później przypisuję je do odpowiednich obiektów w strukturze. Wiem, że mogłem zrobić wszystko używając trzech zmiennych, ale odsyłam do punktu 1.

4. Nazwałem tak funkcje, ponieważ miałem mętlik w głowie od samych nazw dodawanie, odejmowanie więc dodałem przed tym funkcja_. Wiem że nie było to potrzebne.

5. Jak to nie używam niczego co jest zwracane przez return? Jak inaczej wypisywałbym prawidłowy wynik na ekranie?

6. Użyłem tych referencji bo myślałem że będą konieczne, ale później zapomniałem ich już usunąć.

Dziękuję za konstruktywną krytykę, dało mi to wiele do myślenia. Pozdrawiam.

komentarz 4 października 2016 przez criss Mędrzec (172,590 p.)

1. Chciałem żeby wszystko było tak uporządkowane jak tylko mogło, dlatego do każdego działania użyłem trzech różnych zmiennych w strukturze.

Może i coś tam porządkuje, ale kosztem 4-krotnego zwiększenia rozmiaru, więcej kodu. Nie ma szans, żeby to się opłacało. Anyway - są na to sposoby. Chodzi ci tylko o nazwe, prawda? Więc użyj unii:

struct Obliczenia
{
   union
   {
      float wartosc1, skladnik1, odjemna, dzielna;
   };
   // itd. (kolejne unie)
};

Teraz wartosc1 = x jest rownoznaczne z skladnik1 = x , odjemna = x i dzielna = x. (tzn. dostajesz się do tej samej pamięci) Masz 4 różne nazwy i nie zajmujesz 4 razy tyle pamięci. Poczytaj jeszcze o uniach, żebyś zrozumiał jak to działa.

3. Te początkowe wartości tworzę dlatego że nie wiem jakie działanie poda użytkownik i dlatego później przypisuję je do odpowiednich obiektów w strukturze.

No to sam widzisz, że to tylko komplikuje sprawe...

5. Jak to nie używam niczego co jest zwracane przez return? Jak inaczej wypisywałbym prawidłowy wynik na ekranie?

Tak jak to robisz teraz. Przekazujesz do funkcji zmienną do której ma być wynik zapisany i wewnątrz funkcji się to dzieje. Ale przypisania w stylu wynik = f(); (przypisuje wartość zwracaną przez funkcje poprzez słowo kluczowe return do zmiennej) nie widze.

Ponad to:

Zrobił bym metode wynik() która zwraca wynik odpowiedniego działania zależnie od aktualnego znaku arytm. Wtedy faktycznie obliczenia byłyby zamknięte w strukturze, bo póki co to jest jakiś dziwny wrapper. Przetrzymuje zmienne (i to większość niepotrzebnych), ale i tak sami musimy wszystko robić.
Przydałby się konstruktor w którym będziesz inicjalizował wszystkie pola do jakiejś startowej wartości (czynniki do 0, znak arytm. np. do '+').
Wszystkie pola wynikowe wywalić (suma, roznica, iloczyn, iloraz), bo są niepotrzebne skoro mamy metode wynik().
Raczej przyzwyczajaj się do nazywania wszystkiego po angielsku. Wszyscy zrozumieją twój kod, a w przyszłości i tak będziesz musiał tak pisać. No i jest ładniej (po prostu prosciej sie nazywa po ang., a niektórych nawet się nie da po polsku).