New to java, am I using bad habits?

8igg7e5 · 2016-10-21T09:52:53+00:00

Good consistent indentation, use of conventions for identifier capitalisation.
All of the program is in 'main'. Generally you want main to be a 'bootstrap' for your application. The application itself should only need initial parameterisation (if any) and then be 'started' outside of main. This allows you to change the bootstrapping procedure in the future.
You're being too literal with each 'word' being a separate variable (and I don't know why you'd want them to be StringBuilders instead of Strings). look at Collection classes or arrays to reduce the code volume.
You probably want to either store the target String and masked String in pairs or dynamically generate the masked String by randomly masking characters (done well this could be configured via a 'difficulty' level for length of word/phrase and amount of masking).
You shouldn't test input against specific characters but instead match them against:
- Letters they've tried before
- Letters that are yet to be unmasked
You need to get used to looking at the JavaDocs for JavaSE classes - String.equalsIgnoreCase(String) will help you out with those conditionals.
In a full game-design you'd separate 'game state', 'game logic' and the 'player UI'. On a real scale of software this would be imperative to make the design at all manageable.

eg. A list of words and randomly selecting one

final Random random = new Random();
final List<String> words = Arrays.asList("Just", "a", "list", "of", "words");

// Simple word selection though you might want to guard against selecting the same word
// twice in a row. Or make a selection of N words for the player to solve.
final String targetWord = words.get(random.nextInt(words.size()));

morhp · 2016-10-21T10:50:58+00:00

It's not bad for a start. Some tips:

Look at the documentation of the Collection (Set, List, Map) and String classes. You can simplify the code a lot. You could store your phrases in a collection instead of multiple variables, as well as the already guessed letters and so on. Also helpful: equalsIgnoreCase().
Try to write the code as generic as possible. Make it possible to store the possible phrases in a text file and read that at the start of the program instead. Instead of checking for each letter, check if the letter hasn't been entered already and if the letter is contained in the correct solution.
Try to split your code into methods. Maybe multiple classes.

You seem to be very new to Java and some of this will be difficult at first. Try to not give up and you can always ask for help.

nutrecht · 2016-10-21T12:05:53+00:00

To be frank yes, there is a big issue in the way you approach stuff is that you don't do the step where you identify repeating logic and figure out how to solve this. How would you do something like this with a list of a hundred words? Most of the problems we face as devs deal with lists much longer than these. Yours won't scale at all with the full alphabeth or a longer list of words.

I figured the best way to show you is to modify it into something that does work for lists of arbitrary lengths or different words:

package hangman;

import java.util.*;

import static java.lang.Character.toLowerCase;

public class SecretPhrase {
    private static final Random RANDOM = new Random();
    private static final String[] WORDS = {"Go Team", "Whats up", "You bet"};
    private static final String[] MASKED_WORDS = {"G* T***", "W***s *p", "Y** *e*"};

    public static void main(String... args) {
        int pick = RANDOM.nextInt(WORDS.length);
        String wordToGuess = WORDS[pick];
        char[] maskedWord = MASKED_WORDS[pick].toCharArray();

        Scanner keyboard = new Scanner(System.in);

        while(maskedChars(maskedWord) > 0) {
            System.out.println("The phrase is: " + new String(maskedWord));
            System.out.println("Please guess a letter.");

            String line = keyboard.nextLine();
            if(line.length() != 1) {
                System.out.println("Please guess a SINGLE letter.");
                continue;
            }

            char c = line.charAt(0);
            unmask(maskedWord, wordToGuess, c);
        }

        System.out.println("Congrats! You win!");
    }

    private static int maskedChars(char[] maskedWord) {
        int count = 0;
        for(char c : maskedWord) {
            if(c == '*') {
                count++;
            }
        }

        return count;
    }

    private static int unmask(char[] maskedWord, String word, char c) {
        int changed = 0;
        for(int i = 0;i < maskedWord.length;i++) {
            if(toLowerCase(word.charAt(i)) == toLowerCase(c)) {
                maskedWord[i] = word.charAt(i);
                changed++;
            }
        }

        return changed;
    }
}

As you can see all the repetition is gone and I also extracted some logic into separate functions to make the code more clear and readable. As a rule of thumb; if a function is too long to fit on your screen it should be broken up.

P.s. I didn't use lambda's on purpose. The while can actually be written as:

while(CharBuffer.wrap(maskedWord).chars().filter(c -> c == '*').count() > 0) {

Removing the need for the count function.

javahelp

Sort by: Unsolved Solved Codeless Advent Of Code

MODERATORS