• Najnowsze pytania
  • Bez odpowiedzi
  • Zadaj pytanie
  • Kategorie
  • Tagi
  • Zdobyte punkty
  • Ekipa ninja
  • IRC
  • FAQ
  • Regulamin
  • Książki warte uwagi

Sprawdzenie jakości kodu c++

Object Storage Arubacloud
+2 głosów
370 wizyt
pytanie zadane 26 listopada 2020 w C i C++ przez Gutekv0 Nowicjusz (170 p.)

Witam,

Od jakiegoś czasu małymi krokami przerabiam sobie książkę od Alex'a Allain o podstawach c++. Właśnie jestem po wykonaniu zadania, tylko nieco je urozmaiciłem. Chciałbym poprosić Was o wskazówki w przejrzystości kodu oraz o jakiekolwiek błędy. Dodam, iż styczności ze switchem jeszcze nie miałem, jest to następny rozdział ;)

Weź program z menu, który napisałeś wcześniej, i rozbij go na serię wywołań funkcji, z których każda odpowiada za jedną pozycję menu. Jako dwie nowe pozycje menu dodaj kalkulator oraz 99 Bottles of Beer.

Dzięki!

#include <iostream>
#include <string>

std::string imie;
char option;
int calculator();
int txt();
int yesorno();
int yesornotxt();
int menu();
int user();
int main();
int end();

int end()
{
	system("cls");
	std::cout << "Dziekuje ze skorzystania z programu.";

	return 0;
}

int yesornotxt()
{
	std::string kontynuacja;

	while (kontynuacja != "Y" && kontynuacja != "N" && kontynuacja != "y" && kontynuacja != "n")
	{
		std::cout << "Chcesz kontynuowac? (Y/N): ";
		std::cin >> kontynuacja;

		if (kontynuacja == "y" || kontynuacja == "Y")
		{
			txt();
		}
		else if (kontynuacja == "n" || kontynuacja == "N")
		{
			menu();
		}
	}
	return 0;
}

int yesorno()
{
	std::string kontynuacja;

	while (kontynuacja != "Y" && kontynuacja != "N" && kontynuacja != "y" && kontynuacja != "n")
	{
		std::cout << "Chcesz kontynuowac? (Y/N): ";
		std::cin >> kontynuacja;

		if (kontynuacja == "y" || kontynuacja == "Y")
		{
			calculator();
		}
		else if (kontynuacja == "n" || kontynuacja == "N")
		{
			menu();
		}
	}
	return 0;
}

int user()
{
	std::cout << "Podaj swoje imie: ";
	std::cin >> imie;
	return 0;
}

int menu()
{
	system("cls");
	std::cout << "Witaj " << imie << ". Sposrod opcji, wybierz ta ktora Cie interesuje" << std::endl;
	std::cout << "[1]Otworz kalkulator" << std::endl;
	std::cout << "[2]Tekst piosenki 99 Bottles of beer" << std::endl;
	std::cout << "[3]Wyjscie" << std::endl;

	std::cin >> option;

	while (option != '1' && option != '2' && option != '3')
	{
		menu();
	}

	if (option == '1')
	{
		calculator();
	}
	else if (option == '2')
	{
		txt();
	}
	else if (option == '3')
	{
		end();
	}

	return 0;
}

int txt()
{
	system("cls");
	for (int i = 99; i >= 2; i--)
	{
		std::cout << i << " bottles of beer on the wall, " << i << " bottles of beer." << std::endl;
		std::cout << "Take one down and pass it around - " << i - 1 << " bottles of beer on the wall." << std::endl;
	}
	std::cout << "1 bottle of beer on the wall, 1 bottle of beer.\nTake it down and pass it around - no more bottles of beer on the wall." <<std::endl;
	std::cout << "\n" << "";

	yesornotxt();

	return 0;
}

int calculator()
{
	system("cls");
	std::string choose;
	double a;
	double b;
	

	while (true)
	{
		std::cout << "Kalkulator" << std::endl;
		std::cout << "\n" << "Wprowadz znak dzialania (+, -, *, / ): ";
		std::cin >> choose;
		while (choose != "+" && choose != "-" && choose != "*" && choose != "/")
		{
			calculator();
		}
		std::cout << "Wprowadz pierwsza liczbe: ";
		std::cin >> a;
		std::cout << "Wprowadz druga liczbe: ";
		std::cin >> b;
		if (choose == "+")
		{
			std::cout << a << " " << choose << " " << b << " " << "=" << " " << a + b << std::endl;
			std::cout << "\n" << std::endl;
		}
		else if (choose == "-")
		{
			std::cout << a << " " << choose << " " << b << " " << "=" << " " << a - b << std::endl;
			std::cout << "\n" << std::endl;
		}

		else if (choose == "*")
		{
			std::cout << a << " " << choose << " " << b << " " << "=" << " " << a * b << std::endl;
			std::cout << "\n" << std::endl;
		}

		else if (choose == "/")
		{
			std::cout << a << " " << choose << " " << b << " " << "=" << " " << a / b << std::endl;
			std::cout << "\n" << std::endl;
		}
		{
			break;
		}

	}
	yesorno();
	return 0;
}

int main()
{
	user();
	system("cls");
	menu();

	while (option != '1' && option != '2' && option != '3')
	{
		menu();
	}

	if (option == '1')
	{
		calculator();
	}
	else if (option == '2')
	{
		txt();
	}
	else if (option == '3')
	{
		end();
	}
	return 0;

}

 

2 odpowiedzi

+1 głos
odpowiedź 26 listopada 2020 przez j23 Mędrzec (194,920 p.)
wybrane 27 listopada 2020 przez Gutekv0
 
Najlepsza
  • Pozbądź się zmiennych globalnych. Jeśli jakaś funkcja chce mieć wartość option lub imie, to po prostu przekaż je w parametrach funkcji.
  • Zdecyduj się, albo piszesz po angielsku, albo po polsku. Standardem jest angielski.
  • Jeśli funkcja nie musi zwracać wartości, niech będzie void. U Ciebie wszystkie zwracają wartości typu int, choć nic z tego nie wynika - zawsze zwracają 0.
  • while (true) to nie jest dobry pomysł. Dla zwykłej przyzwoitości daj while (std::cin)
  • to zagnieżdżenie menu w menu to zły pomysł. Funkcja powinna zwrócić numer wyboru lub błąd. Zmienna globalna jest zbędna.
  • zamiast drabinki if..else if zastosuj switch.
komentarz 27 listopada 2020 przez Gutekv0 Nowicjusz (170 p.)

Witam, oto co udało mi się zmienić. 

  • Pozbądź się zmiennych globalnych. Jeśli jakaś funkcja chce mieć wartość option lub imie, to po prostu przekaż je w parametrach funkcji.

Mam problem z przekazaniem je w parametrach funkcji. W jaki sposób to zrobić? Dałem je jako zmienne globalne ze względu, że w przypadku zmiennej  imie występuje ona w funkcji  void menu() oraz void user().

  • Zdecyduj się, albo piszesz po angielsku, albo po polsku. Standardem jest angielski. 

 Jeśli chodzi o Imie oraz kontynuacja zostało to zmienione.

  • Jeśli funkcja nie musi zwracać wartości, niech będzie void. U Ciebie wszystkie zwracają wartości typu int, choć nic z tego nie wynika - zawsze zwracają 0.

Zrobione! ;) 

  • while (true) to nie jest dobry pomysł. Dla zwykłej przyzwoitości daj while (std::cin)

Zrobione, lecz nie rozumiem różnicy pomiędzy dwoma metodami. 

  • to zagnieżdżenie menu w menu to zły pomysł. Funkcja powinna zwrócić numer wyboru lub błąd. Zmienna globalna jest zbędna.

Zmienna globalna została usunięta, zagnieżdżone menu zamieniłem w instrukcję break. 

  • zamiast drabinki if..else if zastosuj switch.

To jest mój następny rozdział, ale jak do tego dojdę, to zamierzam tego używać ze względu na to, że słyszałem, że jest wygodniejsza i praktyczna.

Wysyłam aktualny kod, który udało mi się przerobić. Dziękuję Ci bardzo za zainteresowanie się tematem ;)
 

#include <iostream>
#include <string>

std::string name;
char option;

void end()
{
	system("cls");
	std::cout << "Dziekuje ze skorzystania z programu.";

}

void yesOrNoTxt()
{
	void txt();
	void menu();
	std::string again;

	while (again != "Y" && again != "N" && again != "y" && again != "n")
	{
		std::cout << "Chcesz kontynuowac? (Y/N): ";
		std::cin >> again;

		if (again == "y" || again == "Y")
		{
			txt();
		}
		else if (again == "n" || again == "N")
		{
			menu();
		}
	}

}

void yesOrNo()
{
	void calculator();
	void menu();
	std::string again;

	while (again != "Y" && again != "N" && again != "y" && again != "n")
	{
		std::cout << "Chcesz kontynuowac? (Y/N): ";
		std::cin >> again;

		if (again == "y" || again == "Y")
		{
			calculator();
		}
		else if (again == "n" || again == "N")
		{
			menu();
		}
	}

}

void user()
{
	std::cout << "Podaj swoje imie: ";
	std::cin >> name;

}

void menu()
{
	void txt();
	void calculator();
	system("cls");
	std::cout << "Witaj " << name << ". Sposrod opcji, wybierz ta ktora Cie interesuje" << std::endl;
	std::cout << "[1]Otworz kalkulator" << std::endl;
	std::cout << "[2]Tekst piosenki 99 Bottles of beer" << std::endl;
	std::cout << "[3]Wyjscie" << std::endl;

	std::cin >> option;

	while (option != '1' && option != '2' && option != '3')
	{
		{
			break;
		}
	}

	if (option == '1')
	{
		calculator();
	}
	else if (option == '2')
	{
		txt();
	}
	else if (option == '3')
	{
		end();
	}

}

void txt()
{
	system("cls");
	for (int i = 99; i >= 2; i--)
	{
		std::cout << i << " bottles of beer on the wall, " << i << " bottles of beer." << std::endl;
		std::cout << "Take one down and pass it around - " << i - 1 << " bottles of beer on the wall." << std::endl;
	}
	std::cout << "1 bottle of beer on the wall, 1 bottle of beer.\nTake it down and pass it around - no more bottles of beer on the wall." <<std::endl;
	std::cout << "\n" << "";

	yesOrNoTxt();

}

void calculator()
{
	system("cls");
	std::string choose;
	double a;
	double b;
	

	while (std::cin)
	{
		std::cout << "Kalkulator" << std::endl;
		std::cout << "\n" << "Wprowadz znak dzialania (+, -, *, / ): ";
		std::cin >> choose;
		while (choose != "+" && choose != "-" && choose != "*" && choose != "/")
		{
			calculator();
		}
		std::cout << "Wprowadz pierwsza liczbe: ";
		std::cin >> a;
		std::cout << "Wprowadz druga liczbe: ";
		std::cin >> b;
		if (choose == "+")
		{
			std::cout << a << " " << choose << " " << b << " " << "=" << " " << a + b << std::endl;
			std::cout << "\n" << std::endl;
		}
		else if (choose == "-")
		{
			std::cout << a << " " << choose << " " << b << " " << "=" << " " << a - b << std::endl;
			std::cout << "\n" << std::endl;
		}

		else if (choose == "*")
		{
			std::cout << a << " " << choose << " " << b << " " << "=" << " " << a * b << std::endl;
			std::cout << "\n" << std::endl;
		}

		else if (choose == "/")
		{
			std::cout << a << " " << choose << " " << b << " " << "=" << " " << a / b << std::endl;
			std::cout << "\n" << std::endl;
		}
		{
			break;
		}

	}
	yesOrNo();
}

int main()
{
	user();
	system("cls");
	menu();

	while (option != '1' && option != '2' && option != '3')
	{
		menu();
	}

	if (option == '1')
	{
		calculator();
	}
	else if (option == '2')
	{
		txt();
	}
	else if (option == '3')
	{
		end();
	}
	return 0;

}

 

komentarz 28 listopada 2020 przez j23 Mędrzec (194,920 p.)

W jaki sposób to zrobić?

Jeśli menu oczekuje name, wystarczy zrobić tak:

void menu (const std::string &name)
{
    ...
}

Zrobione, lecz nie rozumiem różnicy pomiędzy dwoma metodami. 

Różnica jest taka, że jak strumień wejściowy wejdzie w stan błędu, to pętla zostanie przerwana (wprawdzie jest tam break, ale zakładam, że to błąd).

Linie 69, 70: po co te deklaracje tam? Daj je na początku.

void user()
{
    std::cout << "Podaj swoje imie: ";
    std::cin >> name;
 
}
std::string getUserName()
{
    std::string name;

    std::cout << "Podaj swoje imie: ";
    std::cin >> name;
    return name;
}

Jak pisałem, zmienne globalne nie są koniecznością. Trzeba ich w miarę możliwości unikać. Nazwy funkcji powinny być bardziej opisowe (na ogół zawierają czasownik).

void calculator()
{
    ...     
        while (choose != "+" && choose != "-" && choose != "*" && choose != "/")
        {
            calculator();
        }

To samo co z menu. Po co wywołujesz calculator w calculator? Przecież możesz tak:

std::cout << "\n" << "Wprowadz znak dzialania (+, -, *, / ): ";
while(std::cin >> choose && 
    choose != "+" && 
    choose != "-" && 
    choose != "*" && 
    choose != "/") {
    std::cout << "Jeszcze raz\n";
}

 

–1 głos
odpowiedź 26 listopada 2020 przez Wiciorny Ekspert (270,190 p.)

Przede wszystkim zastosuj namespace. 

using namespace std;

Aby uniknąć 

std::string kontynuacja;

https://docs.microsoft.com/pl-pl/cpp/cpp/namespaces-cpp?view=msvc-160


Nie znam się na Cpp na tyle, ale warto uwzględnić paradygmaty dla każdego rodzaju programów, które świadczą o programiście i tego, że trzyma się pewnych konwencji, zasad:

  • - METODA nazwa- powinna informować o tym, co metoda wykonuje, np getAllUsers() - pobiera wszystkich użytkowników, metody to zachowania więc tak powinno się je tworzyć
  • - zmienne- jako że przechowują danie powinny być przymiotnikami głównie powinny tyczyć się tego co przechowują 
  • gdzie to możliwe staraj się stosować konwencje np camelCase 
    https://pl.wikipedia.org/wiki/CamelCase
  • jasno ustalaj, że piszesz po angielsku żeby nie było sytuacji że metody masz po angielsku a zmienne nnp kontynuacja  po polsku

 

komentarz 27 listopada 2020 przez tkz Nałogowiec (42,000 p.)

Przede wszystkim zastosuj namespace. 

using namespace std;

Dlaczego?

komentarz 27 listopada 2020 przez Wiciorny Ekspert (270,190 p.)
kod programistyczny ma być imperatywny.
komentarz 27 listopada 2020 przez tkz Nałogowiec (42,000 p.)
Nie wiem co użycie przestrzeni nazw ma do imperatywności, ale to zła praktyka, szczególnie w tym przypadku, gdzie przestrzeń nazw musiałby zostać użyta globalnie.
komentarz 27 listopada 2020 przez Wiciorny Ekspert (270,190 p.)
no przecież nie lokalnie, ty w końcu się zdecyduj czy jesteś pasjonat czy specjalista, bo błądzisz.
komentarz 27 listopada 2020 przez Gutekv0 Nowicjusz (170 p.)

@Wiciorny,  Uzyłem konwencji camelCase oraz zmieniłem kontynuacje ;) 
Co do std::, to tak jak kolega powyżej mówi.  Mi to tam nie wadzi, a w późniejszym etapie, gdy już nabiorę rozpędu, będzie to przydatne ;d 
Dzięki wielkie za porady! 

komentarz 28 listopada 2020 przez tkz Nałogowiec (42,000 p.)

@Wiciorny, Nie wiem, czy teraz trolujesz, czy piszesz na poważnie. Wiem tyle, że Twoja wiedza na temat C++ to "hello world". Globalna przestrzeń nazw to strzał w kolano. Z tym stwierdzeniem zgodzi się każdy, kto pisze w C++. Jeszcze dopytam. Co ma imperatywność do przestrzeni nazw?

Podobne pytania

0 głosów
1 odpowiedź 439 wizyt
pytanie zadane 15 lipca 2016 w Java przez michalxi1410 Początkujący (480 p.)
0 głosów
1 odpowiedź 126 wizyt
+1 głos
1 odpowiedź 409 wizyt
pytanie zadane 20 lutego 2020 w Python przez Piotrovsky Nowicjusz (170 p.)

92,576 zapytań

141,426 odpowiedzi

319,652 komentarzy

61,961 pasjonatów

Motyw:

Akcja Pajacyk

Pajacyk od wielu lat dożywia dzieci. Pomóż klikając w zielony brzuszek na stronie. Dziękujemy! ♡

Oto polecana książka warta uwagi.
Pełną listę książek znajdziesz tutaj.

Akademia Sekuraka

Kolejna edycja największej imprezy hakerskiej w Polsce, czyli Mega Sekurak Hacking Party odbędzie się już 20 maja 2024r. Z tej okazji mamy dla Was kod: pasjamshp - jeżeli wpiszecie go w koszyku, to wówczas otrzymacie 40% zniżki na bilet w wersji standard!

Więcej informacji na temat imprezy znajdziecie tutaj. Dziękujemy ekipie Sekuraka za taką fajną zniżkę dla wszystkich Pasjonatów!

Akademia Sekuraka

Niedawno wystartował dodruk tej świetnej, rozchwytywanej książki (około 940 stron). Mamy dla Was kod: pasja (wpiszcie go w koszyku), dzięki któremu otrzymujemy 10% zniżki - dziękujemy zaprzyjaźnionej ekipie Sekuraka za taki bonus dla Pasjonatów! Książka to pierwszy tom z serii o ITsec, który łagodnie wprowadzi w świat bezpieczeństwa IT każdą osobę - warto, polecamy!

...