PL-C++ Bieda kod, sposoby jego usprawnienia i czemu nie działa tak jak powinien

Cześć!

Jeżeli ktoś znajdzie czas i chciałoby mu się pomóc w nauce kodowania początkującemu, a przede wszystkim sprawdzić ten bieda kod to byłbym niezmiernie wdzięczny. Jeżeli odwołałem się do błędnego forum/wątku etc. to przepraszam i proszę o reprymendę.

==============================================================================================

Odnośnie kodu(na ten moment wy edytowałem kod,aby te klamry wyglądały lepiej- i chyba wyglądają. Zmieniłem funkcje. Program nie działa, lecz pracuje nad nim. Jestem głąbem bo dopiero teraz zauważyłem, iż program miał dwa początki :D) :

#include <iostream> 
#include <stdio.h>
#include <cstdlib>
#include <conio.h>
#include <windows.h>
 
using namespace std;
 
string PIN;
 
char wybor;
 
string GetUser(string pin)
 
{
    if (pin == "4057")
    {
        return "Pawel"; 
    }
    else if (pin == "1111")
    {
        return "Krzysztof";
    }
    else if (pin == "4053")
    {
        return "Adrian";
    }
    else if (pin == "2222")
    {
        return "Marcin";
    }
    else if (pin == "1234")
    {
        return "ADMIN";
    }
    else
    {
        return "";
    }
}
 
int main()
 
{
    cout << "Witaj w naszym banku"<<endl; //pierwszy napis
    cout << "podaj PIN:"; //prosba o podanie danych
    cin >> PIN; //wczytywanie danych
 
{
	
        bool isAdmin = false;
        bool correctPin = false;
 
        string user = GetUser(PIN);
            if (user.length() > 0)
            {
                cout << "Poprawny PIN" << endl;
                cout << "Witaj " << user << "! Jak dzis mozemy Ci pomoc?" << endl;
                correctPin = true;
 
                if(user == "ADMIN")
                {
                    isAdmin = true;
                }
            }
                else
                {
                    cout << "Nieprawidlowy PIN";
                }
          
for(;;)
	{
						     
 
            if(isAdmin)
            {
                cout<<endl;
                cout<<"MENU BANKOMATU" <<endl; //cale menu
                cout<<"==========================" <<endl;
                cout<<"1.STAN KASY"<<endl;
                cout<<"2.OSTATNIE LOGOWANIA"<<endl;
                cout<<"3.LOG BLEDOW"<<endl;
                cout<<"4.Wyjscie"<<endl;
 
                cout<<endl;
                wybor=getch();
            }
 
            else if(correctPin) // Nie Admin, ale użytkownik z poprawnym PINem
            {
                cout<<endl;
                cout<<"MENU BANKOMATU" <<endl;
                cout<<"==========================" <<endl;
                cout<<"1.Stan konta"<<endl;
                cout<<"2.Zmiana PIN"<<endl;
                cout<<"3.Wyplac pieniadze"<<endl;
                cout<<"4.Wyjscie"<<endl;
 
                cout<<endl;
                wybor=getch();
            }
                        switch(wybor) // dalej jestem adminem i mam funkcje wielokrotnego wyboru
                        {
                            case '1':
                                cout<<"STAN KASY="<<endl;
                                break;
 
                            case '2':
                                cout<<"OSTATNIE LOGOWANIA"<<endl;
                                break;
 
                            case '3':
                                cout<<"LOG BLEDÓW:"<<endl;
                                break;
 
                            case '4':
                                cout<<"Do widzenia"<<endl;
                                exit(0);
                                break;
 
                            default: cout<<"Nie ma takiej opcji w menu sprobuj jeszcze raz";
 
                        }
 
                            getchar();getchar();
                            system("cls");  //czyszczenie okna po wybraniu opcji
 	}
}
 
        return 0;
 
}


Niestety jedna funkcja nie działa poprawnie i jeszcze nie odkryłem jak z tym kodem zawalczyć. Chciałbym, aby jeżeli ktoś wpisuje inny PIN (który jest w kodzie), inny niż admina to, aby wyświetlało mu się użytkownikowi inne menu.

Jeżeli ktoś chce mi powiedzieć jak usprawnić ten kod etc to też będę bardoz wdzięczny.
W razie pytań zapraszam, sory za poziom kodu i dziękuję z góry.

Pozdrawiam

95% komentarzy jest typu:

int dayOfMonth; // Dzień miesiąca

Na co to komu?
Lub też zmylanie:

char wybor; // Tablica

A ja tu widzę zmienną typu char, nie tablicę.
Nie chcę być tu złośliwy, po prostu chodzi o to, żeby nie komentować oczywistości, bo tak robisz tylko bałagan. Twoja zmienna powinna mówić czym jest, nie komentarz.

Zostawiając komentarze na bok:
Pomyśl jak program sprawdza PIN. Na początku sprawdza czy jest równy "4057". Załóżmy że jest, więc wypisujemy powitanie, a następnie? Porównujemy inne PIN... Po co, jeżeli wiemy, że pierwszy był poprawny? Co innego użycie IF-ELSE IF, bo wtedy kiedy jeden jest poprawny nie sprawdzany bezsensu reszty, ale można usprawnić to dalej - napis jest taki sam dla każdego użytkownika (za wyjątkiem imienia), czemu by nie zrobić funkcji, która porównywałaby PINy i zwracała imię, a następnie skonstruować sobie uniwersalną metodę witania użytkownika?

string GetUser(string pin)
{
	if (pin == "4057")
	{
		return "Pawel";
	}
	else if (pin == "1111")
	{
		return "Krzysztof";
	}
	else if (pin == "4053") 
	{
		return "Adrian";
	}
	else if (pin == "2222")
	{
		return "Marcin";
	}
	else if (pin == "1234")
	{
		return "ADMIN";
	}
	else
	{
		return "";
	}
}


int main()
{
	string user = GetUser(PIN);
	if (user.length() > 0)
	{
		cout << "Poprawny PIN" << endl;
		cout << "Witaj " << user << "! Jak dzis mozemy Ci pomoc?" << endl;
	}
	else
	{
		cout << "Nieprawidlowy PIN";
	}
	...
}

Poza tym, pozbądź się zmiennych globalnych bo w większości przypadków to zło. Powyższe rozwiązanie daje Ci możliwość przekazania PIN jako parametr.

Co do ustalania czy użytkownik jest adminem czy nie, możesz to zrobić za pomocą zmiennej-flagi:

bool isAdmin = false;
bool correctPin = false;

string user = GetUser(PIN);
if (user.length() > 0)
{
	cout << "Poprawny PIN" << endl;
	cout << "Witaj " << user << "! Jak dzis mozemy Ci pomoc?" << endl;

	correctPin = true;

	if(user == "ADMIN")
	{
		isAdmin = true;
	}
}
else
{
	cout << "Nieprawidlowy PIN";
}

if(isAdmin)
{
	//Wyswietl Menu Admin'a
}
else if(correctPin) // Nie Admin, ale użytkownik z poprawnym PINem
{
	//Wyswietl Menu Użytkownika
}

Powyższe rozwiązanie daje Ci również możliwość czy użytkownik wpisał poprawny pin. Możesz jednak dać użytkownikowi możliwość ponowienia próby, wtedy pętla while Twoim przyjacielem.

Jeżeli formatowanie kodu jest takie jak powyżej to je również należy poprawić, W żadnym wypadku nie powinieneś mieć kilku klamer zamykających na tej samej linii.

Wywal poszczególne fragmentu kodu w osobne funkcje, narób sobie dobrych nawyków a w przyszłości, w większych projektach, łatwiej Ci będzie do nich wrócić po jakimś czasie i rozumieć co się gdzie robi..

Jeśli do pinu zamiast std::string użyłbyś int to funkcja zwracająca nazwę użytkownika na podstawie pinu wyglądałaby jeszcze ładniej

std::string GetUser( int pin )
{
  std::string name;
  switch( pin )
  {
    case 4057: name  = "Pawel"; break;
    case 1111: name = "Krzysztof"; break;
    case 4053: name = "Adrian"; break;
    case 2222: name = "Marcin"; break;
    case 1234: name = "ADMIN"; break;
    default: break;
  }
  return name;
}

@Mastyma:

hej! Wyjustowałem kod czy teraz te klamry wyglądają lepiej? Resztę zmian aktualnie implementuje.

1. Klamry powinieneś mieć zaraz za instrukcją i z takim samym wcięciem, to jest:

// To
if(...)
{
}
// Zamiast
	if(...)
{
}

2. Rzeczy w case'ach powinny być wcięte:

switch(...)
{
	case 1:
		// Instrukcja
		break;
	case 2: // Pusta linia nie jest konieczna bo case'y są na innym wcięciu niż instrukcje w nich
		// I tak dalej
}

3)Ta "drabinka" ifów ze sprawdzaniem PIN wygląda jakby te ify były zagnieżdżone, a nie są.

4. Można przyjąć zasadę, że klamry oznaczają wcięcie, czyli nigdy nie powinno być żadnej instrukcji na tym samym wcięciu co klamry (jeżeli klamry ją obejmują oczywiście).

if(...)
{
	// Instrukcja n
	// Instrukcja n+1 itd...
	for(...) //Tu wcięcie jest OK
	{
		//Instrukcja x
	DoSomething(); // To powinno być wcięte
	}
	DoSomethingElse(); // To jest ok, bo nie należy do 'for' ale do 'if'
}

Dodatkowo:

default: cout<<"Nie ma takiej opcji w menu sproboj jeszcze raz";

Spróbuj ;)

@Mastyma:

BTW kod jeszcze bardziej wyjustowałem. Jeżeli mogę Cię prosić zweryfikuj tera.

Opiszę w poście bo łatwiej mi będzie pokazać o co chodzi:

1. Masz dwa razy int main():

int main() // <-- Tu jeden
{
    cout << "Witaj w naszym banku"<<endl; //pierwszy napis
    cout << "podaj PIN:"; //prosba o podanie danych
    cin >> PIN; //wczytywanie danych
 
        int main() // <-- Tu dwa
{

2. Źle wcięty kod:

 string user = GetUser(PIN);
            if (user.length() > 0)
            {

3. switch(wybor) też jest wcięte, jak i kod za nim, a te linie powinny być na tym samym poziomie co klamra zamykająca else if(...):

else if(correctPin) // Nie Admin, ale użytkownik z poprawnym PINem
{
	//...

}
switch(wybor) // dalej jestem adminem i mam funkcje wielokrotnego wyboru
{
	//... 
} 
getchar(); getchar();
system("cls");  //czyszczenie okna po wybraniu opcji

Dodając jeszcze do poprzedniej listy, przy przypisywaniu wartości użyj spacji przed i za znakiem równania (i innymi), dla przykładu:

wynik=5+3; // Jest mniej czytelne niż:
wynik = 5 + 3;

//To samo możesz zastosować do coutów itd.
cout << "Tekst" << endl;
cin >> zmienna;

Jak już to poprawisz powinno być OK ;)

@Mastyma:
W dużym uproszczeniu:
Wyobraź sobie, że kompilator czyta Twój kod linia po linii, z góry na dół.
Załóżmy, że mamy prosty kod:

#include <iostream>
using namespace std;

int main()
{
	SayHi();
	// Linię wyżej kompilator wywali błąd, bo myśli - osoba chce wywołać jakąś funkcję, ale ja jej nie znam, nie wiem o niej nic.
	return 0;
}
void SayHi()
{
	cout << "Hello World" << endl;
}

Więc możemy mieć:

#include <iostream>
using namespace std;
void SayHi() // [Kompilator]: Aha, tak będzie wyglądała ta funkcja.
{
	cout << "Hello World" << endl; 
}
int main()
{
	SayHi(); // Kompilator w tym momencie widział już taką funkcję i wie co ma robić.
	return 0;
}

Można zrobić to jeszcze inaczej, czyli zadeklarować funkcję przed main, ale zdefiniować za nim:

#include <iostream>
using namespace std;
void SayHi(); // Deklaracja funkcji (nagłówek)
int main()
{
	SayHi(); // Kompilator wie, że taka funkcja jest zadeklarowana i wie jak się do niej odwołać.
	return 0;
}
void SayHi() // Definicja
{
	cout << "Hello World" << endl;
}

To samo dzieje się w plikach nagłówkowych (*.h), tam wrzucasz tylko deklaracje zmiennych, funkcji itd. ale definicje są już w pliku *.cpp

@atmal odnośnie Twojego komentarza kilka postów wyżej. Nie wprowadzaj kolegi w błąd. Zera z przodu liczb w C++ nie będą ignorowane. Na dowód masz tu kawałek kodu https://ideone.com/WVOdTe

Poprzedzenie liczby zerem powoduje traktowanie takiej liczby jako zapisanej w systemie ósemkowym.

Ale w czym problem? Pin 0123 traktujemy jako 123 i tyle.
Zero nie jest żadnym problemem.

#include <iostream>
#include <cassert>
#include <string>
#include <sstream>
using namespace std;

int main()
{
	istringstream iss;
	iss.str("0123");
	int i;
	iss >> i;
	assert(i == 123); // ok!
	assert(i == 0123); // nie ok.
}

https://ideone.com/f3qYel

@Azarien ja to wiem, że można traktować 0123 jako 123, ale chodzi mi o tą subtelną różnicę. Nawet w Twoim kodzie napisałeś "nie ok". Tylko o to mi chodziło ponieważ kolega napisał w komentarzu, że program pominie zero na początku liczby. Chciałem zaznaczyć, że należy z tym uważać i w kodzie takie liczby traktowane są jak liczby zapisane w systemie ósemkowym.

Tak przy okazji, to w realnym programie trzymanie takich rzeczy jak kody PIN czy PESEL jako liczba jest średnim pomysłem.