¿Este código sigue las convenciones estándar, no es redundante? ¿Cómo puedo hacerlo más comprensible para otras personas?
Es un ejercicio del libro introductorio de Java:
Escriba un juego del ahorcado que genera aleatoriamente una palabra y le pide al usuario que adivine una letra a la vez. Cada letra de la palabra se muestra como un asterisco. Cuando el usuario adivina correctamente, se muestra la letra real. Cuando el usuario termina un palabra, muestra el número de errores.
Mi programa:
import java.util.Arrays; import java.util.Scanner; public class Hangman{ public static void main(String[] args) { String[] words = {"writer", "that", "program"}; // Pick random index of words array int randomWordNumber = (int) (Math.random() * words.length); // Create an array to store already entered letters char[] enteredLetters = new char[words[randomWordNumber].length()]; int triesCount = 0; boolean wordIsGuessed = false; do { // infinitely iterate through cycle as long as enterLetter returns true // if enterLetter returns false that means user guessed all the letters // in the word e. g. no asterisks were printed by printWord switch (enterLetter(words[randomWordNumber], enteredLetters)) { case 0: triesCount++; break; case 1: triesCount++; break; case 2: break; case 3: wordIsGuessed = true; break; } } while (! wordIsGuessed); System.out.println("\nThe word is " + words[randomWordNumber] + " You missed " + (triesCount -findEmptyPosition(enteredLetters)) + " time(s)"); } /* Hint user to enter a guess letter, returns 0 if letter entered is not in the word (counts as try), returns 1 if letter were entered 1st time (counts as try), returns 2 if already guessed letter was REentered, returns 3 if all letters were guessed */ public static int enterLetter(String word, char[] enteredLetters) { System.out.print("(Guess) Enter a letter in word "); // If-clause is true if no asterisks were printed so // word is successfully guessed if (! printWord(word, enteredLetters)) return 3; System.out.print(" > "); Scanner input = new Scanner(System.in); int emptyPosition = findEmptyPosition(enteredLetters); char userInput = input.nextLine().charAt(0); if (inEnteredLetters(userInput, enteredLetters)) { System.out.println(userInput + " is already in the word"); return 2; } else if (word.contains(String.valueOf(userInput))) { enteredLetters[emptyPosition] = userInput; return 1; } else { System.out.println(userInput + " is not in the word"); return 0; } } /* Print word with asterisks for hidden letters, returns true if asterisks were printed, otherwise return false */ public static boolean printWord(String word, char[] enteredLetters) { // Iterate through all letters in word boolean asteriskPrinted = false; for (int i = 0; i < word.length(); i++) { char letter = word.charAt(i); // Check if letter already have been entered bu user before if (inEnteredLetters(letter, enteredLetters)) System.out.print(letter); // If yes - print it else { System.out.print("*"); asteriskPrinted = true; } } return asteriskPrinted; } /* Check if letter is in enteredLetters array */ public static boolean inEnteredLetters(char letter, char[] enteredLetters) { return new String(enteredLetters).contains(String.valueOf(letter)); } /* Find first empty position in array of entered letters (one with code \u0000) */ public static int findEmptyPosition(char[] enteredLetters) { int i = 0; while (enteredLetters[i] != "\u0000") i++; return i; } }
Registro de su trabajo:
(Guess) Enter a letter in word **** > a (Guess) Enter a letter in word **a* > t (Guess) Enter a letter in word t*at > q q is not in the word (Guess) Enter a letter in word t*at > t t is already in the word (Guess) Enter a letter in word t*at > b b is not in the word (Guess) Enter a letter in word t*at > h (Guess) Enter a letter in word that The word is that You missed 2 time(s)
Comentarios
- Sería bueno si adjunto un enlace en el alojamiento de imágenes gratuitas con una imagen del esquema de bloques que ilustra lógica de mi programa, porque ' no es obvio cómo funciona o ' no es necesario?
- Los diagramas de flujo y similares aumentarían th La comprensión de lo que ' está haciendo. Eso ' siempre es bueno.
- Creo que ' has hecho un buen trabajo eliminando redundancias y creando métodos para hacerlo más conciso. Sin embargo, es un poco difícil de seguir. Quizás los nombres de métodos más descriptivos ayudarían (¡la mejor documentación es el código legible!)
- su código es demasiado complicado, necesita explicar lo que está sucediendo, ya que es difícil para un estudiante leer y saber exactamente qué ' está sucediendo. Gracias.
Respuesta
Números mágicos: sería mejor mover sus definiciones para 0,1, 2,3 en un campo enum
o final
para que pueda hacer referencia a ellos con un nombre significativo. Por ejemplo, final int LETTER_NOT_IN_WORD = 0
o enum HangmanGuess { LETTER_NOT_IN_WORD }
Redundancia: la lógica central entre findEmptyLetters
y inEmptyLetters
es lo mismo, estás buscando un char
en un char[]
. Podrías refactorizar así:
/* Check if letter is in enteredLetters array */ public static boolean inEnteredLetters(char letter, char[] enteredLetters) { return indexOf(letter, enteredLetters) >= 0; } /* Find first empty position in array of entered letters (one with code \u0000) */ public static int findEmptyPosition(char[] enteredLetters) { return indexOf("\u0000", enteredLetters) } /* Determine the index in {@code vals} where {@code ch} exists. Returns -1 if {@code ch} is not in {@code vals}. */ public static int indexOf(char ch, char[] vals) { return Arrays.asList(vals).indexOf(Character.valueOf(ch)); }
Una cosa a destacar también es que en la implementación actual, findEmptyPosition
devolverá n
donde n
es el enteredLetters.length + 1
si no hay posiciones vacías. No estoy seguro de si esta es la funcionalidad deseada.
Respuesta
Lo primero (pequeño): parece más común use Random
que Math.random()
. Consulte esta publicación sobre prácticas recomendadas .
Los comentarios y el cambio dentro de do {} while () es muy confuso para mí personalmente. enterLetter()
nunca devuelve true
o false
, e incluso la mayoría de los programadores nuevos saben que un do / while se repite hasta que se cumple la condición. Sería más útil describir qué representa 0-3 fuera código de producción o más complejo, una enumeración lo haría considerablemente más comprensible).
Dentro de enterLetter()
, sería útil mover el int emptyPosition = findEmptyPosition(enteredLetters);
llamada a la else if
donde se usa. No solo evita la asignación de memoria hasta que se usa, sino que también es más legible (y puede eliminar la variable emptyPosition
si lo desea y llamar a la función dentro de la referencia , pero eso es semántico).