Prośba o sprawdzenie i ocenę kodu

0

Witam, proszę o sprawdzenie i konstruktywną krytykę mojego kodu. Program ma być prostą symulacją banku. Mają się na niego składać dwa podstawowe bloki: tworzenie konta (w końcowej fazie pisania) i operacje wykonywane na koncie (metody są już zadeklarowane - chciałbym się ograniczyć do kilku podstawowych). W planach mam modyfikację ustawienia pinu, tak żeby wpisywane liczby były niewidoczne. Nie potrafię sobie poradzić z wywołaniem metody generującej numer konta (nrK cannot be resolve to a variable). Z góry przepraszam jeśli poniższy kod jest zbyt długi.

public class Konto {
	
	private String imie;
	private String nazwisko;
	private double stanKonta;
	private String login;
	private String haslo;
	private String pin;
	private String numerKonta;
	
	//konstruktor bezparametrowy tworzy konto
	public Konto(){
		this.imie = "";
		this.nazwisko = "";
		this.stanKonta = 0.0;
		this.login = "";
		this.haslo = "";
		this.pin = "";
		this.numerKonta = null;
	}

	//konstruktor tworzy konto
	public Konto(String imie, String nazwisko, double stanKonta, String login, String haslo, String pin){
		this.imie = imie;
		this.nazwisko = nazwisko;
		this.stanKonta = stanKonta;
		this.login = login;
		this.haslo = haslo;
		this.pin = pin;
		this.numerKonta = "09 2490 1044 0000 3200 9400 7370";
	}
	
///////////////////////////////////////////////////////	
//  metody:  
	
	//ustaw imię
	public void setImie(String imie1){
		imie = imie1;
	}	
	//pokaż imię
	public String getImie(){
		return imie;
	}
///////////////////////////////////////////////////////	
	
	//ustaw nazwisko
	public void setNazwisko(String nazwisko1){
		nazwisko = nazwisko1;
	}
	//pokaż nazwisko
	public String getNazwisko(){
		return nazwisko;
	}
///////////////////////////////////////////////////////	
	
	//ustaw login
	public void setLogin(String login1){
		login = login1;
	}	
	//pokaż login
	public String getLogin(){
		return login;
	}
///////////////////////////////////////////////////////		
	
	//ustaw hasło
	public void setHaslo(String haslo1){
		haslo = haslo1;
	}
	//pokaż haslo
	public String getHaslo(){
		return haslo;
	}
///////////////////////////////////////////////////////		
	
	//ustaw PIN
	public void setPIN(String pin1){
		pin = pin1;
	}
	//pokaż PIN
	public String getPIN(){
		return pin;
	}	
///////////////////////////////////////////////////////		
	
	//przydzielanie nuberu konta (losowy)
	public void setNumerKonta(String nrK){
		String nr = "";
		String czworki = "";
		String para = "";
		
		int c = (int) Math.round(Math.random()*99 );
			if(c<10){
				para = "0" + c;
			}else{
				para = c + "";
			}
		
		for(int i = 0; i < 6; i++){
			int a = (int) Math.round(Math.random()*9999 );
			
			if(a <= 999 && a > 99){
				czworki = " "+a;
			}else if(a <= 99 && a > 9){
				czworki = "  "+a;
			}else if(a <= 9 && a >= 0){
				czworki = "   "+a;
			}
			nr +=" " + czworki;
		}
		nrK = para + nr;
		numerKonta = nrK;
	}
///////////////////////////////////////////////////////
	
	//pokaż nr konta
	public String getnrK(){
		return numerKonta;
	}
///////////////////////////////////////////////////////
	
	//sprawdzenie stanu konta
	public double getStanKonta(){
		return stanKonta;
	}
///////////////////////////////////////////////////////	
	
	//wpłata:
	public void wplata(double w){
		stanKonta += w;
	}	
	//wypłata:
	public void wyplata(double w){
		stanKonta -= w;
	}
}

klasa testująca:

import java.util.Scanner;
import javax.swing.JOptionPane;

public class Test {

	public static void main(String[] args) {
		
		String decyzja = "";
		String czyTak = "";
		
		while(decyzja.isEmpty() || !"yn".contains(decyzja)){
			czyTak = JOptionPane.showInputDialog("Witaj w naszym banku! Czy jesteś już naszym klientem? \n Jeśli tak - wpisz: y \n Jeśli nie - wpisz: n");
			decyzja = new Scanner(czyTak).next();
		}
		
		if(decyzja.equals("n") ){
			decyzja = "";
			czyTak = "";
			
			while(decyzja.isEmpty() || !"yn".contains(decyzja)){
				czyTak = JOptionPane.showInputDialog("Czy chcesz się zarejestrować w naszym banku i stworzyć konto? \n Jeśli tak - wpisz: y \n Jeśli nie - wpisz: n");
				decyzja = new Scanner(czyTak).next();
			}
			
			if(decyzja.equals("n")){
				JOptionPane.showMessageDialog(null, "Do widzenia");
				System.exit(0);
			}
			
			//tworzenie nowego konta
			else{
				Konto nowe = new Konto();
				
				while(nowe.getImie().equals("") ){
					nowe.setImie(JOptionPane.showInputDialog("Rejestracja \nPodaj imię: ") );
				}
				
				while(nowe.getNazwisko().equals("") ){
					nowe.setNazwisko(JOptionPane.showInputDialog("Rejestracja \nPodaj nazwisko: ") );
				}
				
				while(nowe.getLogin().equals("") ){
					nowe.setLogin(JOptionPane.showInputDialog("Rejestracja \nPodaj login: ") );
				}
				
				String potwierdz ="default";
				while( !(nowe.getHaslo().equals(potwierdz)) ){
					nowe.setHaslo(JOptionPane.showInputDialog("Podaj hasło: ") );
					potwierdz = JOptionPane.showInputDialog("Potwierdź hasło: ");
					
					if( !(nowe.getHaslo().equals(potwierdz) ) ){
						JOptionPane.showMessageDialog(null, "Błąd! Hasło niepotwierdzone!");
					}
				}

				potwierdz ="default";
				while(    !(nowe.getPIN().equals(potwierdz))   ||   !(   (Integer.valueOf(nowe.getPIN()) > 0)  &&  (Integer.valueOf(nowe.getPIN()) < 10000)   )    ){
					nowe.setPIN(JOptionPane.showInputDialog("Podaj PIN (4 cyfry): "));
					
					if(Integer.valueOf(nowe.getPIN()) > 10000){
						JOptionPane.showMessageDialog(null, "Błąd! PIN zbyt długi!"); continue;
					}else if(Integer.valueOf(nowe.getPIN()) < 1000){
						JOptionPane.showMessageDialog(null, "Błąd! PIN zbyt krótki!"); continue;
					}
					
					potwierdz = JOptionPane.showInputDialog("Potwierdź PIN: ");
				
					if( !(nowe.getPIN().equals(potwierdz) ) ){
						JOptionPane.showMessageDialog(null, "Błąd! PIN niepotwierdzony!");
					}
				}
				//przydzielenie numeru konta wygenerowanego losowo
				nowe.setNumerKonta(nrK);
				
				//spr
				System.out.println(nowe.getImie() + "\n" +nowe.getNazwisko() + "\n" + nowe.getLogin() + "\n" + nowe.getHaslo() + "\n" + nowe.getPIN() + "\n" + nowe.getnrK() );
			}
		}
	}
}
0

To co zwróciło szczególnie moją uwagę, to fakt że walidację (np. poprawności pinu) realizujesz dopiero w klasie testującej. Takie sprawdzenia powinny być wykonywane podczas implementacji metod w pierwszej klasie. Wówczas można by obsługiwać patologiczne przypadki poprzez wyrzucanie wyjątków. A co do numeru konta to można moim zdaniem użyć typu Integer.

0

Uwagi do klasy "Konto"

  1. "09 2490 1044 0000 3200 9400 7370" - magic number, zdefiniuj jako stałą
  2. Nie rozumiem. Metoda public void setNumerKonta(String nrK) przyjmuje nrk tylko po to by nigdy tej wartości nie użyć?
  3. Warunki typu:
if(a <= 999 && a > 99)

można by napisać jakoś lepiej (i wyrzucić ify) np.

 String.format("%04d", a);

Ta linijka przyjmuje liczbę całkowitą, zwracając String według reguły:
dla 1 - 0001
dla 12 - 0012
dla 1234 - 1234

0

Poprawiona metoda generowania losowego nr konta:

	//przydzielanie nuberu konta (losowy)
	public void setNumerKonta(){
		String nrK = "";
		String nr = "";
		String para = "";
		
		int p = (int) Math.round(Math.random() * 99);
		para =  "" + String.format("%02d", p);
		
		for(int i = 0; i < 6; i++){
			int a = (int) Math.round(Math.random() * 8999);

			nr +=" " + String.format("%04d", a);
		}
		nrK = para + nr;
		numerKonta = nrK;
	}
1
public void setNumerKonta(){
        String nr = "";
 
        int c = (int) Math.round(Math.random()*99 );
        nr += String.format("%02d", c);
 
        for(int i = 0; i < 6; i++){
            int a = (int) Math.round(Math.random()*9999 );
            nr +=" " + String.format("%04d", a);
        }

        numerKonta = nr;
}
 

Chociaż jakby się tak zastanowić, to po co losować pary, czwórki, można wylosować raz tylo-cyfrową liczbę ile cyferek potrzebujemy i sformatować.

0

Przy próbie stworzenia nowego obiektu klasy Konto w klasie Test pojawił się komunikat, że to co jest w nawiasie "cannot be resolved to a variable". Ma ktoś pomysł, co może być źle? Konstruktor:

	//konstruktor tworzy konto
	public Konto(String imie, String nazwisko, double stanKonta, String login, String haslo, String pin, String NRK){
		this.imie = imie;
		this.nazwisko = nazwisko;
		this.stanKonta = stanKonta;
		this.login = login;
		this.haslo = haslo;
		this.pin = pin;
		this.numerKonta = NRK;
	}

komenda tworząca nowe konto w klasie Test:

Konto domyslne = new Konto(Jan, Kowalski, 2000, login, haslo, 1234, NRK);
0

Wydaje mi się, ze warto abyś od początku nauki podchodził do tematu bardziej analitycznie, programowanie to nie tylko klepanie kodu. W Twoim programie jest trochę punktów, które można jeszcze raz przemyśleć, np:

* czy konto jest bezpośrednio powiązane z imieniem, nazwiskiem, loginem, hasłem..? Ja bym wydzieliła jeszcze jedna klasę, np Customer, Profile, etc. opisującą osobę (klienta, który jest właścicielem danego konta). W takie sposób będziesz miał oddzielne obiekty: Klient banku i konto. Takie podejście sprawi, ze Twój program będzie bardziej elastyczny, i np. jeden klient banku będzie bez problemu mógł miec wiele kont ;) 

* następnie, czy może istnieć profil użytkownika bez imienia, nazwiska, loginu? Raczej nie, wiec nie udostępniłabym konstruktora, który pozwoli na utworzenie obiektu bez tych danych. 

* Zastanów się tez, czy numer konta nie powinien być automatycznie ustawiony przy tworzeniu konta? Obstawiam, ze tak, wiec po co udostępniać publiczny setter? Czy login może się zmienić? Nie powinien, wiec tez nie potrzebujesz publicznego settera, jeśli przekażesz login w konstruktorze. Pamiętaj, ze nie o to chodzi, żeby generować gettery i settery dla każdej zmiennej, udostępniaj te metody z głowa :)

* Jeśli wydzielisz oddzielna klasę opisująca klienta, potrzebujesz czegoś, co wskaże do kogo dane konto należy. Można wiec utworzyć taki konstruktor, który przy tworzeniu konta wymusi podanie identyfikatora użytkownika, i automatycznie wygeneruje numer, np.:
 public Konto(int userId){
		this.user = userId;
        this.stanKonta = 0.0;
        this.numerKonta = this.setNumerKonta(); //private method
    }

Jeśli chodzi o sam kod, jest przyjęte uzywanie angielskich nazw zmiennych, metod, klas, itd.

0

Dziękuję za odpowiedź @shagrin. Powyższy kod miał być jedynie przećwiczeniem wszystkiego, czego w nim użyłem. Kiedy planowałem ten program, założyłem że będzie on znacznie mniejszy. W miarę kodowania dodawałem nowe pomysły (stąd też momentami brak konsekwencji w kodzie), które sprawiły że niepotrzebnie i zdecydowanie za daleko wyszedłem poza wcześniej ustalone ramy. Dowiodło to tego, że powinienem się skupić na mniejszych projektach, pozwalających szybciej przećwiczyć nowo poznane zagadnienia. Pierwotnie miała być tylko rejestracja, jedno konto + metody wpłaty/wypłaty i wyświetlanie danych.

  • następnie, czy może istnieć profil użytkownika bez imienia, nazwiska, loginu? Raczej nie, wiec nie udostępniłabym konstruktora, który pozwoli na utworzenie obiektu bez tych danych.

Dodałem ten konstruktor na potrzeby szybkiego stwarzania dodatkowych kont, pomiędzy którymi miałyby być wykonywane operacje.

Wszystkie Twoje uwagi i każdego kto wypowiadał się w tym temacie, są dla mnie bardzo cenne i postaram się do nich stosować w kolejnych projektach.

1 użytkowników online, w tym zalogowanych: 0, gości: 1