«Игра в партию»

Я начал путешествие, чтобы стать программистом-самоучкой около месяца назад, и мой самый большой страх перед обучением - это развитие вредных привычек и не осознание этого.

Я ищу критику в этой игре «Угадай номер». Это мой первый настоящий «проект», в который я вложил серьезное время, поэтому я чувствую, что это моя первая возможность быть критикой.

package guessANumber;

import java.util.Random;

import javax.swing.JOptionPane;

/**
 * @author Mike Medina
 */

/**
 * The "Guess a Number" game
 * */
public class Guess {

static boolean playAgain;
static int max; // The objective will be between 1 and this number
static int objective; // Users are trying to guess this number
static int userGuess;

public Guess() {}

    /**
     * Runs the game while playAgain == true
     */ 
    public static void main(String[] args) {

        do {
        setMax();
        setObjective();
        userGuess();
        playAgain();
        } while (playAgain == true);

    }

    /**
     * Asks the user to set the max value they will guess between and validates it
     */
    public static void setMax() {

        boolean valid = false;

        // Asks user for "max"
        while(!valid) {
            try {
                max = Integer.parseInt(JOptionPane.showInputDialog("You will try to guess a number between 1 and \"max\". Set max: "));
                valid = true;
            }
            catch (NumberFormatException e) {}
        }

        // Ensures user input for max is greater than 1 and fits in an int
        while (max < 1 || max > Integer.MAX_VALUE) {
            valid = false;
            while (!valid) {
                try {
                    max = Integer.parseInt(JOptionPane.showInputDialog("Max must be between 1 and " + Integer.MAX_VALUE + ": "));
                    valid = true;
                }
                catch(NumberFormatException e) {}
            }
        }

    }

    /**
     * Sets the objective between 1 and "max"
     */
    public static void setObjective() {

        Random rand = new Random();

        if (max == 1)
            objective = 1;
        else
            objective = rand.nextInt(max - 1) + 1;

        // Prints objective for testing
        System.out.println(objective);

    }

    /**
     * Takes in the user's guess, validates it, and tells them when they win
     */
    public static void userGuess() {

        // Prompts user for guess and ensures it's an integer
        do {
            userGuess = 0;

            try {
                userGuess = Integer.parseInt(JOptionPane.showInputDialog("Guess a number between 1 and " + max + ": "));
            }
            catch (NumberFormatException e){}

            // Ensures user's guess is an integer greater than 0 and less than max
            while (userGuess > max || userGuess < 1) {
                try {
                    userGuess = Integer.parseInt(JOptionPane.showInputDialog(null, "Your guess must be between 1 and " + max));
                    }
                catch (NumberFormatException e) {}
            }
            userWinLose();
        } while (userGuess != objective);

    }

    /**
     * Tells the user whether their guess was high, low, or correct
     */
    public static void userWinLose() {

        // Tells the user their guess was too high, too low, or correct
        if (userGuess < objective)
            JOptionPane.showMessageDialog(null, "Too low!");
        else if (userGuess > objective)
            JOptionPane.showMessageDialog(null, "Too high!");
        else
            JOptionPane.showMessageDialog(null, "You win!");

    }

    /**
     * Asks the user if they want to play again or quit
     */
    public static void playAgain() {

        boolean valid = false;

        // Confirms user input is an integer
        while (!valid) {
            try {
                if (Integer.parseInt(JOptionPane.showInputDialog("Enter 0 to quit or 1 to play again: ")) == 0)
                    playAgain = false;
                else
                    playAgain = true;
                valid = true;
            }
            catch (NumberFormatException e) {}
        }

    }

}
27 голосов | спросил Mike Medina 17 Jpm1000000pmFri, 17 Jan 2014 15:19:22 +040014 2014, 15:19:22

6 ответов


27

В дополнение к уже указанным комментариям:

Держите вещи просто:

  • while (playAgain == true) можно записать while (playAgain)
  • if (A) variable=false; else variable=true; может быть записано variable = !A;
  • Не повторяйте себя: если вы много разбираетесь с пользовательскими вводами и проверяете значения, сделайте его функцией, которую вы можете повторно использовать.
  • Вам не нужны статические элементы. Локальная переменная может сделать трюк. Идея состоит в том, чтобы держать вещи в минимально возможном объеме, чтобы было легче понять, что изменит что.

Вот мой код с учетом моих комментариев:

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

/**
 * The "Guess a Number" game
 * */
public class Guess {
    public Guess() {}

    /**
     * Runs the game while playAgain == true
     */ 
    public static void main(String[] args) {
        do {
            int max = getMax();
            int obj = getObjective();
            userGuess(obj, max);
        } while (playAgain());
    }

    private static int getIntFromUser(String text, int min, int max)
    {
        while (true)
        {
            try
            {
                int val = Integer.parseInt(JOptionPane.showInputDialog(text));
                if (min <= val && val <= max)
                    return val;
            }
            catch (NumberFormatException e) { /* As pointed out by ChrisW, it might be worth having something here */}
        }
    }

    /**
     * Asks the user for the max value
     */
    public static void getMax() {
        return getIntFromUser("You will try to guess a number between 1 and \"max\". Set max: ", 1, Integer.MAX_VALUE);
    }

    /**
     * Gets the objective between 1 and "max"
     */
    public static int getObjective() {
        // Please read Simon André Forsberg's comment about this.
        Random rand = new Random();
        return (max == 1) ?
            1 :
            rand.nextInt(max - 1) + 1;
    }

    /**
     * Takes in the user's guess, validates it, and tells them when they win
     */
    public static void userGuess(int obj, int max) {
        int userGuess;
        do {
            userGuess = getIntFromUser("Guess a number between 1 and " + max + ": ", 1, max);
            userWinLose(userGuess, obj);
        } while (userGuess != obj);
    }

    /**
     * Tells the user whether their guess was high, low, or correct
     */
    public static void userWinLose(int guess, int objective) {
        if (guess < objective)
            JOptionPane.showMessageDialog(null, "Too low!");
        else if (guess > objective)
            JOptionPane.showMessageDialog(null, "Too high!");
        else
            JOptionPane.showMessageDialog(null, "You win!");
    }

    /**
     * Asks the user if they want to play again or quit
     */
    public static boolean playAgain() {
        return (getIntFromUser("Enter 0 to quit or 1 to play again", 0, 1) == 1);
    }
}
ответил Josay 17 Jpm1000000pmFri, 17 Jan 2014 16:08:52 +040014 2014, 16:08:52
17

В целом это выглядит довольно хорошо. Несколько указателей, хотя ...

  • Попробуйте правильно отложить свой код. Большинство из них выглядит хорошо, но на нескольких местах вы пропустили отступы. например.

    do {
    setMax();
    setObjective();
    userGuess();
    playAgain();
    } while (playAgain == true);
    
  • Не употребляйте и не прерывайте молчание на исключениях (за исключением некоторых редких случаев, но это вне сферы действия здесь)

    catch (NumberFormatException e) {} // don't do this
    
  • Оборудуйте свои утверждения соответствующими фигурными скобками

    if (max == 1)                   // no brackets!
        objective = 1;
    else                            // no brackets!
        objective = rand.nextInt(max - 1) + 1;
    

Некоторые люди не делают это из-за привычки, стиля кодирования и т. д. Это может быть не проблема, но по мере расширения вашего кода может возникнуть проблема, и рекомендации по языку стиля IIRC Java против отказа от фигурных скобок. Вот пример того, почему это может быть плохая идея https://stackoverflow.com/a/8020255/1021726

  • max > Integer.MAX_VALUE Никогда не произойдет

Потому что max объявляется как целое число и присваивает значение, введенное в JOptionPane. В случае, если пользователь вводит значение, превышающее Integer.MAX_VALUE, он выдает исключение. И поскольку вы ничего не делаете, за исключением этого, он просто терпит неудачу. См. Пункт № 2.

  • статический vs нестатический

Вы понимаете static? Если нет, то вы можете получить краткое описание здесь http://www.caveofprogramming.com/frontpage/articles/java/java-for-beginners-static-variables-what-are-they/

Если вы понимаете статичность, вы можете понять, что не хотите, чтобы два экземпляра вашего класса Guess использовали одну и ту же переменную.

  • Объявить модификатор доступа

    static boolean playAgain; // no access modifier
    

Пока ваши переменные не имеют модификаторов доступа (и я не думайте, что это по назначению), и это может быть не проблема, так как тогда он по умолчанию используется как package-private , но вы должны остерегаться того, что важно создать свой класс, его членов и переменных соответственно как они должны использоваться. Являются ли ваши переменные действительно package-private , или они должны быть объявлены как просто private переменные?

ответил Max 17 Jpm1000000pmFri, 17 Jan 2014 15:36:52 +040014 2014, 15:36:52
10

И Хосай, и Макс дали отличные ответы. Есть две вещи, которые я хотел бы добавить.

  1. Нет необходимости переносить логическую переменную playagain. Вы можете просто:

    do {
        ....
    } while (playAgain());
    
  2. В вашем методе getObjective() есть ошибка. Вы никогда не будете генерировать значение «max» (если оно не равно 1). Из API Java Документация для nextInt (n) :

      

    Возвращает псевдослучайное равномерно распределенное значение int между 0 (включительно) и заданным значением (эксклюзивным) `

    ... поэтому вы просите пользователя ввести max, скажем, например, введите 5. Теперь ваш getObjectiveMethod делает rand.nextInt(max - 1) + 1;, который будет генерировать случайное число из 0 через (5-1), который, так как (5 - 1) равен 4, а случайное значение будет эксклюзивным из 4, наибольшее генерируемое случайное число будет 3. Добавьте 1 к этому еще раз, и вы получите 4. Таким образом, ваш самый большой Objective никогда не будет 5 или максимальное значение.

Это случайное число поколение является общей проблемой, с которой можно столкнуться. Есть несколько сообщений и блогов об этом, но логический указатель, на который вы направляете, - это ответ StackOverflow: Генерация случайных чисел в диапазоне

Суть в том, что вы хотите, чтобы ваш номер сгенерировался как:

rand.nextInt(max) + 1;

В результате этого нет необходимости иметь специальный случай для max == 1. Вам нужно только убедиться, что max > 0, когда он введен (что вы должны сделать перед установкой valid = true

ответил rolfl 17 Jpm1000000pmFri, 17 Jan 2014 16:46:17 +040014 2014, 16:46:17
7

Чтобы бросить вас в глубоком конце, было бы неплохо начать больше думать о объектно-ориентированном дизайне для ваших классов и интерфейсов. Подумайте о повторном использовании своего кода и о том, как инкапсулировать достаточно полезное поведение в классе, сохраняя его как можно более общим.

Скажем, стало известно, что вы написали замечательный класс, чтобы играть в эту игру угадывания, а другие разработчики хотят использовать ее в своем собственном коде. Только один парень хочет использовать другой графический интерфейс, другой хочет, чтобы устройство издавало разные звуковые сигналы в зависимости от того, слишком ли высока или низкая догадка, а другая хочет использовать игру с очень маленькими детьми и всегда устанавливать максимум до 10. Поскольку это что они не могут использовать ваш код, потому что особенности реализации отображаются на каждом уровне.

Давайте сосредоточимся только на работе игры. Интерфейс понятен: клиент должен иметь возможность сообщать о догадках классу, а класс должен указывать, является ли догадка слишком высокой, слишком низкой или правильной. Поэтому определите IGuessHandler как интерфейс обработки угадывания, содержащий только это, и теперь наследуем его класс. Затем вы можете предоставить точно такие же реализации, как сейчас, но кто-то другой может реализовать разные.

Я думаю, вы также должны извлечь свой основной в класс GameRunner и, как правило, пытаться иметь более точные имена для классов и операций. В вашем основном цикле есть метод playAgain, который запускается, когда мы играем в первый раз. Это кажется тривиальным, но это небрежное мышление. У вас нет метода под названием playAgain, есть одна игра, которая фактически играет в игру. Вы называете это снова и снова, но сохраняйте имена как можно более ясными и описательными.

ответил TheMathemagician 17 Jpm1000000pmFri, 17 Jan 2014 19:41:23 +040014 2014, 19:41:23
5

Одна вещь, которую вы должны знать об исключениях, заключается в том, что вам не нужно сразу их поймать.

Причина, по которой код генерирует исключение в исключении в первую очередь, заключается в том, что он не знает, как обрабатывать исключение.

Аналогично, подпрограмма one-higher (например, метод getIntFromUser), которая сразу же вызвала этот код, также не знает, как обрабатывать исключение.

Это нормально, возможно, нормально, чтобы поймать исключение в другой подпрограмме намного выше стека вызовов.

Например, вот модифицированная версия ответа Жозе:

private static int getIntFromUser(String text, int min, int max)
{
    // parseInt might throw NumberFormatException
    int val = Integer.parseInt(JOptionPane.showInputDialog(text));
    if (min <= val && val <= max)
        return val;
    else
        throw new IllegalArgumentException("Not an integer between " + min + " and " max);
}

// display error message from caught exception
// return true if the user wants to play again
private static bool getIsContinueAfterError(String errorMessage)
{
    String message = errorMessage + "\n\nDo you want to continue playing?";
    int reply = JOptionPane.showConfirmDialog(null, message, "Error", JOptionPane.YES_NO_OPTION);
    return (reply == JOptionPane.YES_OPTION);
}

public static void main(String[] args) {
    boolean playAgain;
    do {
        try {
            int max = getMax();
            int obj = getObjective();
            userGuess(obj, max);
            playAgain = playAgain();
        }
        catch (NumberFormatException ex)
        {
            playAgain = getIsContinueAfterError("This is not an integer.");
        }
        catch (IllegalArgumentException ex)
        {
            playAgain = getIsContinueAfterError(ex.getMessage());
        }
    } while (playAgain);
}

Это не обязательно хороший пример (в вашем примере вы можете поймать и повторить попытку в getIntFromUser); Я имел в виду это как иллюстрацию, потому что вы сказали , что вы не знакомы с исключениями.

С вашим кодом, возможно, пользователь не вводил целое число, потому что все цифровые клавиши разбиты на клавиатуре; ваш код будет ловить и повторять навсегда, тогда как мой код дает пользователю возможность прекратить игру.

ответил ChrisW 17 Jpm1000000pmFri, 17 Jan 2014 18:52:07 +040014 2014, 18:52:07
3

с логической переменной playagain вы должны установить значение false при создании переменной, а затем цикл может быть простым while(playagain), который многие предпочитают a while за цикл do while, потому что вы видите, какое условие находится прямо из ворот.

EDIT: теперь, когда я прочитал код снова, я вижу, почему вы пошли с do while, и это полностью имеет смысл, за исключением следующего:

ваш метод PlayAgain должен возвращать логическое значение. что, как говорится, он должен быть пойман и передан переменной внутри вашего метода Main() что-то вроде keepPlaying и эта переменная должна быть создана внутри вашего Main(). логическое значение playAgain используется только в двух местах, Main() и playAgain(), что означает, что вы не должны делать его глобальным , он специфичен и должен быть конкретным по своему охвату, он не влияет ни на один из других методов в программе.

ответил Malachi 17 Jpm1000000pmFri, 17 Jan 2014 19:04:06 +040014 2014, 19:04:06

Похожие вопросы

Популярные теги

security × 330linux × 316macos × 2827 × 268performance × 244command-line × 241sql-server × 235joomla-3.x × 222java × 189c++ × 186windows × 180cisco × 168bash × 158c# × 142gmail × 139arduino-uno × 139javascript × 134ssh × 133seo × 132mysql × 132