Sprawdzenie kodu - czy kod jest poprawnie napisany?

1

Poprzez zalecenia osób z forum przestałem pisać nic nie wnoszące w naukę programy z "programowaniem obiektowym" i skupiłem się na napisaniu programu po prostu na pętlach metodach itd. Program ma symulować grę lotto. Jak oceniacie ten kod? Co można poprawić, aby był bardziej czytelny? Czy jest zgodny ze standardami języka na etapie mojej nauki?

import java.util.Random;
import java.util.Scanner;

public class Main {
    public static void main(String[] args) {
        System.out.println("Witaj w symulatorze gry lotto!");
        displayMenu();
    }

    static int[] userTicket = new int[6];
    static int[] lotteryTicket = new int[6];
    static int[] correctNumbers = new int[6];
    static long deposit = 20;
    static long betAmount;

    private static void displayMenu() {
        Scanner input = new Scanner(System.in);

        int choice;
        do {
            System.out.println(STR."\nStan konta: \{deposit}zł");
            System.out.println("Co chcesz teraz zrobić?");
            System.out.println("1. Stawianie kuponu");
            System.out.println("2. Sprawdzanie kuponu");
            System.out.println("3. Koniec gry");
            System.out.print("Twój wybór: ");
            choice = input.nextInt();

            switch (choice) {
                case 1:
                    createUserTicket();
                    System.out.print("Postawiony kupon: ");
                    displayTicket(userTicket);
                    break;
                case 2:
                    generateLotteryTicket();
                    System.out.print("Wylosowany kupon: ");
                    displayTicket(lotteryTicket);
                    checkLotteryTicket();
                    displayContinueMenu();
                    break;
                case 3:
                    System.out.println("Dziękuję za granie w lotto!");
                    System.exit(0);
                    break;
                default:
                    System.out.println("Wprowadzono zły znak!");
                    System.exit(0);
                    break;
            }
        } while (true);
    }

    private static void createUserTicket() {
        Scanner input = new Scanner(System.in);

        do {
            System.out.print("Za jaką stawkę chcesz zagrać? ");
            betAmount = input.nextLong();
            if (betAmount > deposit) {
                System.out.println("Zakład nie może być większy niż stan konta!");
            }

        } while (betAmount > deposit);
        deposit -= betAmount;

        for (int i = 0; i < userTicket.length; i++) {
            int singleNumber;
            System.out.print(STR."Podaj \{i + 1}. liczbę do kuponu (1 - 49): ");
            singleNumber = input.nextInt();
            if (validateTicket(userTicket, singleNumber)) {
                userTicket[i] = singleNumber;
            } else {
                System.out.println("Twoje liczby nie mogą sie powtarzać i być spoza zakresu!");
                i--;
            }
        }
    }

    private static boolean validateTicket(int[] tickets, int singleNumber) {
        if (singleNumber > 0 && singleNumber < 50) {
            for(int ticket: tickets) {
                if (singleNumber == ticket) {
                    return false;
                } else {
                    return true;
                }
            }
        } else {
            return false;
        }
        return true;
    }

    private static void displayTicket(int[] tickets) {
        for (int i = 0; i < tickets.length; i++) {
            if (tickets[i] != 0) {
                if (i == tickets.length - 1) {
                    System.out.print(STR."\{tickets[i]}.");
                } else {
                    System.out.print(STR."\{tickets[i]}, ");
                }
            }
        }
        System.out.println();
    }

    private static void generateLotteryTicket() {
        Random random = new Random();
        for (int i = 0; i < lotteryTicket.length; i++) {
            int singleNumber = random.nextInt(49) + 1;
            if (validateTicket(lotteryTicket, singleNumber)) {
                lotteryTicket[i] = singleNumber;
            } else {
                i--;
            }
        }
    }

    private static void checkLotteryTicket() {
        int counter = 0;
        for (int i = 0; i < userTicket.length; i++) {
            for (int k : lotteryTicket) {
                if (userTicket[i] == k) {
                    correctNumbers[i] = userTicket[i];
                    counter++;
                }
            }
        }

        if (counter == 0 || counter == 1 || counter == 2) {
            System.out.println("Niestety nic nie wygrałeś");
        } else if  (counter == 3) {
            deposit += betAmount * 2;
            System.out.println(STR."Gratulacje! Na twoje konto trafi \{betAmount * 2}zł");
        } else if  (counter == 4) {
            deposit += betAmount * 10;
            System.out.println(STR."Gratulacje! Na twoje konto trafi \{betAmount * 10}zł");
        } else if  (counter == 5) {
            deposit += betAmount * 100;
            System.out.println(STR."Gratulacje! Na twoje konto trafi \{betAmount * 100}zł");
        } else {
            deposit += betAmount * 100000;
            System.out.println(STR."Gratulacje! Na twoje konto trafi \{betAmount * 100000}zł");
        }

        System.out.print(STR."Liczba trafień: \{counter}\nTrafione liczby: ");
        displayTicket(correctNumbers);
    }

    private static void displayContinueMenu() {
        Scanner input = new Scanner(System.in);
        int choice;

        if (deposit > 0) {
            System.out.println("Czy chcesz grać dalej?");
            System.out.println("1. Tak");
            System.out.println("2. Nie");
            System.out.print("Twój wybór: ");
            choice = input.nextInt();

            switch (choice) {
                case 1:
                    userTicket = new int[userTicket.length];
                    lotteryTicket = new int[lotteryTicket.length];
                    displayMenu();
                    break;
                case 2:
                    System.out.println("Dziękuję za granie w lotto!");
                    System.exit(0);
                    break;
                default:
                    System.out.println("Wprowadzono zły znak!");
                    System.exit(0);
                    break;
            }
        } else {
            System.out.println("Nie masz już pieniędzy na koncie!");
            System.exit(0);
        }
    }
}

2
private static final int MAX_NUMBER = 49;

private static boolean validateTicket(int[] tickets, int singleNumber) {
    if (singleNumber <= 0 || singleNumber >= MAX_NUMBER + 1) {
        return false; // Number is outside the valid range
    }
    
    for (int ticket : tickets) {
        if (singleNumber == ticket) {
            return false; // Number is a duplicate
        }
    }
    
    return true; // Number is valid
}
1

Do takich rzeczy poprawiających czytelność to bym zaczął od przeniesienia całego programu do osobnej klasy a funkcje main wykorzystał tylko i wyłącznie jako metodę w której uruchamiasz grę. Lepiej mimo wszystko pielęgnować przyzwyczajenie aby logikę programu umieszczać w osobnej klasie. Java jest językiem obiektowym i warto się z obiektowym podejściem oswajać. Statyczne metody są głównie używane przy metodach wspierających jakieś procesy, przykładowo różne operacje na Stringach. Logika raczej powinna być przypisana do pojedynczego obiektu

    public class Lotto {
        // tutaj cała logika
    }
    public static void main(String[] args) {
        Lotto lotto = new Lotto();

        lotto.startNewLottoGame(); // ta metoda uruchamia tylko to co było w main
    }

Usunąć static z każdej metody oraz zmiennej a następnie do zmiennych dodać private. Scanner może być metodą globalną, polecam zrobić ją finalną. Final używasz ponieważ nie ma potrzeby później przypisywać nowego obiektu do zmiennej input, jest to coś w rodzaju oznaczenia - używaj tylko mnie. Scanner przeniosłem aby uniknąć ponownych inicjalizacji, jest ona niepotrzebna.

    private final Scanner input = new Scanner(System.in); // Scaner przeniesony tutaj i nie musisz go inicjalizować za każdym razem
    private int[] userTicket = new int[6];
    private int[] lotteryTicket = new int[6];
    private int[] correctNumbers = new int[6];
    private long deposit = 20;
    private long betAmount;

Dla czytelności też warto w switchu wywoływać pojedyńcze metody które będą uruchamiały jakieś konkretne akcje a jednocześnie będą opisywały to co robią. W taki sposób tylko i wyłącznie zerkając na switch jesteś w stanie określić co będzie uruchamiane podczas wybrania danej akcji zamiast patrzeć na menu wyżej.

    private void displayMenu() {
        int choice;
        do {
            System.out.println(STR."\nStan konta: \{deposit}zł");
            System.out.println("Co chcesz teraz zrobić?");
            System.out.println("1. Stawianie kuponu");
            System.out.println("2. Sprawdzanie kuponu");
            System.out.println("3. Koniec gry");
            System.out.print("Twój wybór: ");
            choice = input.nextInt();

            switch (choice) {
                case 1: placeNewTicket(); break;
                case 2: displayTickets(); break;
                case 3: completeGame(); break;
                default: handleIncorrectCharacter(); break;
            }
        } while (true);
    }

    private void placeNewTicket() {
        createUserTicket();
        System.out.print("Postawiony kupon: ");
        displayTicket(userTicket);
    }

    private void displayTickets() {
        generateLotteryTicket();
        System.out.print("Wylosowany kupon: ");
        displayTicket(lotteryTicket);
        checkLotteryTicket();
        displayContinueMenu();
    }

    private void completeGame() {
        System.out.println("Dziękuję za granie w lotto!");
        System.exit(0);
    }

    private void handleIncorrectCharacter() {
        System.out.println("Wprowadzono zły znak!");
        System.exit(0);
    }

To co wyżej można też zrobić w metodzie displayContinueMenu()

Wartości z tego if else mogą być w switchu

    private void checkLotteryTicket() {
        int counter = 0;
        for (int i = 0; i < userTicket.length; i++) {
            for (int k : lotteryTicket) {
                if (userTicket[i] == k) {
                    correctNumbers[i] = userTicket[i];
                    counter++;
                }
            }
        }

        if (counter == 0 || counter == 1 || counter == 2) {
            System.out.println("Niestety nic nie wygrałeś");
        } else if  (counter == 3) {
            deposit += betAmount * 2;
            System.out.println(STR."Gratulacje! Na twoje konto trafi \{betAmount * 2}zł");
        } else if  (counter == 4) {
            deposit += betAmount * 10;
            System.out.println(STR."Gratulacje! Na twoje konto trafi \{betAmount * 10}zł");
        } else if  (counter == 5) {
            deposit += betAmount * 100;
            System.out.println(STR."Gratulacje! Na twoje konto trafi \{betAmount * 100}zł");
        } else {
            deposit += betAmount * 100000;
            System.out.println(STR."Gratulacje! Na twoje konto trafi \{betAmount * 100000}zł");
        }

        System.out.print(STR."Liczba trafień: \{counter}\nTrafione liczby: ");
        displayTicket(correctNumbers);
    }

To takie kilka poprawek stylistycznych które mocno by poprawiły czytelność tego kodu. W przyszłości możesz się zainteresować również Listami zamiast tablic, są czytelniejsze i posiadają sporo przydatnych funkcji, ale to jak zrozumiesz dobrze działanie zwykłych tablic.

2
if (counter == 0 || counter == 1 || counter == 2) {

Zamiast rozpisywać się w ten sposób, możesz sprawdzić od razu czy coś jest mniejsze lub równe 2:

if (counter <= 2) {

Zarówno krócej, jak i bardziej pokazuje twoje intencje.

No bo spójrz:

int counter = 0;

Deklarujesz zmienną całkowitą, która może być dodatnia lub ujemna.
Co jeśli z jakichś powodów (np. przyszłej modyfikacji programu) counter będzie ujemny?

Wtedy mając te ify, które masz:

     if (counter == 0 || counter == 1 || counter == 2) {
        } else if  (counter == 3) {
              //...
        } else if  (counter == 4) {
              //...
        } else if  (counter == 5) {
              //...
        } else {
            deposit += betAmount * 100000;
            System.out.println(STR."Gratulacje! Na twoje konto trafi \{betAmount * 100000}zł");
        }

wpadniesz w ostatnie else, czyli counter będzie na minusie, a tobie się wyświetli tak jakby było np. 6 czy więcej.
modyfikacja pierwszego ifa na if (counter <= 2) by mogła temu zapobiec, bo przecież to chcesz naprawdę sprawdzić.

Pisząc kod, warto nie myśleć tylko o tym, że teraz działa, ale czy będzie działać w przyszłości, czy jest odporny na różne dziwne wartości brzegowe. Programista powinien być trochę pesymistą.

2

Co do user experience tej aplikacji:

  • Dlaczego użytkownik ma podawać 6 razy po kolei liczbę? Nie prościej byłoby go spytać żeby podał liczby po przecinku, i system wtedy mógłby je zwalidować?
  • Niektóre message'e są napisane z punktu widzenia programisty, a nie użytkownika:
    • "Zakład nie może być większy niż stan konta!" - słaba wiadomość, pisana z punktu widzenia kodu
    • "Nie masz tyle pieniędzy" - dobra wiadomość, pisana z punktu widzenia użytkownika
    • "Nie można kupić zakładu za 45zł (stan konta: 35zł). - jeszcze lepsza wiadomość, lepszy UX.
  • To samo jest niżej:
    • "Wprowadzono zły znak!" - wiadomość z perspektywy programisty (bo faktycznie wprowadzono)
    • "Znak "ł" jest niepoprawny (dozowolone znaki: a-z, 0-9)" - pomocna wiadomość z punktu widzenia UX.
  • Zamieniłbym też "Twoje liczby nie mogą sie powtarzać i być spoza zakresu!" na "Podaj nie powtarzające się liczby" oraz "Podaj liczby w zakresie 1-49", najlepiej jako dwie osobne wiadomości (które mogą się pokazać obie).

Co do samego kodu:

  1. Niepotrzebnie wszystko jest na staticach. Przenieś kod do zwykłej metody, np. run(), i uruchom ją w main(), np przez Main.run().
  2.  if (singleNumber == ticket) {
         return false;
     } else {
         return true;
     }
    
    to można zamienić na
    return singleNumber != ticket;
    
  3. Na prawdę STR."\{tickets[i]}." jest lepsze niż tickets[i] + "."?
  4. Całą funkcję displayTicket() można zaimplementować kodem:
    System.out.println(String.join(", ", tickets) + ".");
    
  5. Nie używaj System.exit(0);. Raczej skorzystaj z normalnych instrukcji sterujących, if, return, else do kontrolowania tego co program ma zrobić.
  6. validateTicket() jest trochę dziwny - dlatego że Twój kod teraz działa tak, żę w momencie w którym user wprowadza nową liczbę, Ty sprawdzasz czy ona już istnieje. To jest mega słaby design bo dodaje silny-coupling wprowadzania danych do walidacji danych. To co powinieneś zrobić, to osobno pobrać dane od usera, i osobno jest zwalidować - np najpierw pobrać wszystkie liczby, a potem wszystkie sprawdzić. I zamiast pisać własną funkcję, to wpisz w google "java check if array has duplicates".

No i moim zdaniem powinieneś napisać testy automatyczne pod ten program.

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