Följer den här koden standardkonventioner, inte överflödig? Hur kan jag göra det mer begripligt för andra människor?

Det är en övning från Java-introduktionsboken:

Skriv en hangman-spel som slumpmässigt genererar ett ord och uppmanar användaren att gissa en bokstav i taget. Varje bokstav i ordet visas som en asterisk. När användaren gör en korrekt gissning visas den faktiska bokstaven. När användaren avslutar en word, visa antalet missningar.

Mitt program:

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; } } 

Logg av sitt arbete:

 (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)  

Kommentarer

  • Skulle det vara bra om jag bifogar länk till fri bildhosting med bild av blockschemat som illustrerar logik för mitt program, eftersom det ' inte är uppenbart hur det fungerar eller att det ' inte är nödvändigt?
  • Flödesscheman och liknande skulle öka th förståelsen av vad du ' gör. Att ' alltid är en bra sak.
  • Jag tror att du ' har gjort ett bra jobb med att ta bort uppsägningar och skapa metoder för att göra det mer kortfattat. Det är dock lite svårt att följa. Kanske mer beskrivande metodnamn skulle hjälpa (den bästa dokumentationen är läsbar kod!)
  • din kod är för komplicerad, du behöver förklara vad som händer, eftersom det är svårt för en student att läsa och veta exakt vad ' händer. Tack.

Svar

Magiska siffror: Det vore bättre att flytta dina definitioner till 0,1, 2,3 i ett enum eller final fält så att du kan referera till dem med ett meningsfullt namn. Till exempel final int LETTER_NOT_IN_WORD = 0 eller enum HangmanGuess { LETTER_NOT_IN_WORD }

Redundans: Kärnlogiken mellan findEmptyLetters och inEmptyLetters är samma, du letar efter en char i en char[] . Du kan refaktorera så här:

/* 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)); } 

En sak att påpeka är också att i den nuvarande implementeringen findEmptyPosition returnerar n där n är enteredLetters.length + 1 om det inte finns några tomma positioner. Jag är inte säker på om detta är den önskade funktionaliteten.

Svar

Första (små) sak: det verkar vanligare att använd Random än Math.random(). Se det här inlägget med bästa metoder .

Kommentarerna och byt först ut i do {} medan () är mycket förvirrande för mig personligen. enterLetter() returnerar aldrig true eller false, och även de flesta nya programmerare känner till en do / medan iterates tills villkoret är uppfyllt. Det skulle vara mer användbart att beskriva vad 0-3 representerar (och om detta var produktionskod eller mer komplex, skulle Enum göra det betydligt mer förståeligt).

Inne i enterLetter(), skulle det vara till hjälp att flytta int emptyPosition = findEmptyPosition(enteredLetters); kallar till else if där den används. Det förhindrar inte bara att allokera minnet tills det används, det är också mer läsbart (och du kan ta bort variabeln emptyPosition om du vill och ringa funktionen inuti referensen , men det är semantiskt.

Lämna ett svar

Din e-postadress kommer inte publiceras. Obligatoriska fält är märkta *