Server and Client programs for guessing game

Question

I recently had to complete this for a school assignment and I really enjoyed it, but was hoping someone with experience in OO design or maybe just Java in general could have a look and point out anything that's glaringly bad, or give me some pointers; I would appreciate it a lot. I hope it's not too much code.

This is BullsAndCowsServer.java

//-------------------------
//  Multithreaded server program
//-------------------------

import java.net.*;
import java.io.*;

public class BullsAndCowsServer
{
    public static void main(String[] args) throws IOException
    {

        final int PORT = (args.length > 0) ? Integer.parseInt(args[0]) : 1337;
        ServerSocket s = new ServerSocket(PORT);

        System.out.println("Started: " + s);

        try
        {
            while (true)
            {
                Socket socket = s.accept();
                System.out.println("New client connected: " + socket.getRemoteSocketAddress());
                try
                {
                    new ThreadWorker(socket);

                } catch (IOException e)
                {
                    socket.close();
                } finally
                {
                    //socket.close();
                }
            }

        } finally
        {
            s.close();
        }
    }

}

This is ThreadWorker.java

//-------------------------
//  Thread class for server use
//-------------------------

import java.io.*;
import java.net.Socket;
import java.util.ArrayList;
import java.util.Random;

//this handles each game connection
class ThreadWorker extends Thread
{
    public Socket socket;
    public BufferedReader in;
    public PrintWriter out;

    public ThreadWorker(Socket s) throws IOException
    {
        socket = s;
        in = new BufferedReader(new InputStreamReader(s.getInputStream()));
        out = new PrintWriter(new BufferedWriter(new OutputStreamWriter(s.getOutputStream())), true);
        start();
    }

    public void run()
    {
        try
        {
            out.println("--------------------------------\nWelcome to the Bulls and Cows guessing game!\nType any of " +
                    "the following options:\n" +
                    "HELP\nNEWGAME\nRESUME <passcode>\n--------------------------------");

            String str = in.readLine();
            while (!str.equals("END"))
            {

                switch (str.split(" ", 10)[0])
                {
                    case "HELP":
                        help();
                        break;
                    case "NEWGAME":
                        newgame();
                        break;
                    case "RESUME":
                        resume(str);
                        break;
                    default:
                        out.println("Sorry, that's not a valid option; please try again!");
                        break;
                }

                out.println("--------------------------------\nType any of the following options:\n" +
                        "HELP\nNEWGAME\nRESUME <passcode>\n--------------------------------");
                str = in.readLine();
            }
        } catch (IOException | NullPointerException e){}
        finally
        {
            try
            {
                System.out.println(socket.getRemoteSocketAddress() + " disconnected.");
                socket.close();
            } catch (IOException e)
            {
            }
        }
    }

    private void newgame() throws IOException
    {
        bcGame game = new bcGame();

        out.println("\n---------------------\nGreetings challenger.\nWhat is your name?");
        String str = in.readLine();
        str = str.replace(' ', '_');
        game.name = str;
        out.println("\n\n\n\n\n\n\nGreetings " + str + ". Play by typing 4 digit numbers where each digit is unique.\nYou can" +
                " type PAUSE at any time to receive a passcode allowing you to return later.\nGoodluck!\n---------------------------");
        playGame(game);

    }

    private void playGame(bcGame game) throws IOException
    {
        String str;
        while (game.guesses < 6)
        {
            boolean guessed = false;
            int bulls = 0, cows = 0;
            out.println("\nYou have " + (6 - game.guesses) + " guesses remaining.\nPlease guess a number!");
            str = in.readLine();

            //pausing code
            if (str.equals("PAUSE"))
            {
                pause(game);
                return;
            }

            //check input is valid
            if (!validate(str))
            {
                out.println("\nInvalid input; please enter a 4 digit number with no duplicate digits");
                continue;
            }
            game.guesses++;

            //calculate score
            for (int i = 0; i < 4; i++)
            {
                if (str.charAt(i) == game.num.charAt(i))
                    bulls++;
                else if (game.num.contains(str.charAt(i) + ""))
                    cows++;
            }

            //check win condition
            if (bulls == 4)
            {
                out.println("\n~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~");
                out.println("Congratulations, you are correct!\nYou won after " + game.guesses + " guess" + ((game.guesses > 1) ? "es" : "") + "!");
                out.println("~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n");
                out.println("Press enter to return to the menu..");
                in.readLine();
                return;
            } else
            {
                out.println("Bulls: " + bulls + "\nCows: " + cows);
            }
        }
        out.println("\nSorry, you are out of guesses! The number was " + game.num + " \nBetter luck next time\n");
    }

    private synchronized void pause(bcGame game) throws IOException
    {
        out.println("\nPausing game...");
        String dir = System.getProperty("user.dir");
        File file = new File(dir + "/savedgames.txt");
        file.createNewFile();
        FileWriter writer = new FileWriter(file, true);
        int key = new Random().nextInt(99999);
        writer.append(key + " " + game.name + " " + game.num + " " + game.guesses + " ");
        writer.close();
        out.println("\nYour game has been saved.\nYou can resume at any time with the code " + key);
        in.readLine();
        return;
    }

    private void resume(String str) throws IOException
    {

        String gamekey = str.split(" ")[1];
        out.println("\nReloading game <" + gamekey + ">");

        String dir = System.getProperty("user.dir");
        File file = new File(dir + "/savedgames.txt");
        if (!file.exists())
        {
            out.println("Saved game not found, press enter to return to menu");
            in.readLine();
            return;
        }

        FileReader reader = new FileReader(file);
        char[] a = new char[5000];
        reader.read(a);
        String s = new String(a);
        String[] saved = s.split(" ");
        for (int i = 0; i < saved.length; i += 4)
        {
            if (saved[i].equals(gamekey))
            {
                try
                {
                    bcGame resumed = new bcGame(saved[i + 1], saved[i + 2], Integer.parseInt(saved[i + 3]));
                    out.println("Game found! Welcome back " + resumed.name);
                    playGame(resumed);
                    return;
                } catch(NumberFormatException e)
                {
                    System.out.println("Saved game is corrupt!");
                    break;
                }
            }
        }
        out.println("Saved game not found, press enter to return to menu");
        in.readLine();
        return;

    }

    private boolean validate(String str)
    {
        try
        {
            if (str.length() != 4) return false;

            int num = Integer.parseInt(str);
            boolean[] digs = new boolean[10];
            for(int i = 0; i < 4; i++)
            {
                if (digs[num % 10]) return false;
                digs[num % 10] = true;
                num /= 10;
            }
            return true;
        }
        catch(NumberFormatException e) { return false;}
    }

    private void help() throws IOException
    {

        out.println("\nIn this game you must guess the secret 4 digit number.\nEach digit is unique; ie. no digit " +
                "appears more than once." +
                "\nAfter each guess you will be told a number of bulls and cows.\nYou get a bull if" +
                " you guessed a digit in the correct place.\nYou get a cow if you guessed the digit correctly, but it " +
                "was in the wrong place.\n" +
                "Guess the number correctly in 6 attempts to win.\n Best of luck, challenger.");
        out.println("\nPress enter to return to the menu");
        in.readLine();
    }


}

class bcGame
{
    String num, name;
    int guesses;

    public bcGame()
    {
        guesses = 0;
        num = generateNum();
        name = "";
    }

    public bcGame(String namer, String numr, int guessesr)
    {
        guesses = guessesr;
        num = numr;
        name = namer;
    }

    //generates a 4 digit number in string form where all the digits are unique
    String generateNum()
    {
        ArrayList arr = new ArrayList();// = {"0", "1", "2", "3", "4", "5", "6", "7", "8", "9"};
        for (int i = 0; i < 10; i++) arr.add(i);

        String ret = "";

        Random rand = new Random();
        for (int i = 0; i < 4; i++)
        {
            int num = rand.nextInt(arr.size());
            ret += arr.remove(num);
            arr.trimToSize();
        }
        return ret;
    }
}

And this is BullsAndCowsClient.java

//-------------------------
//  Multithreaded client program
//-------------------------

import java.net.*;
import java.util.Scanner;
import java.io.*;

public class BullsAndCowsClient
{
    public static void main(String args[]) throws IOException
    {
        //read in command line arguments
        final String HOST = (args.length > 0) ? args[0] : "localhost";
        final int PORT = (args.length > 1) ? Integer.parseInt(args[1]) : 1337;

        try
        {
            Socket sock = new Socket(HOST, PORT);
            Reader reader = new Reader(sock);
            Writer writer = new Writer(sock);
            while(reader.isAlive() && writer.isAlive() )
            {
                Thread.sleep(100);
            }
            //System.out.println("Quitting!");
            if(reader.isAlive())
                reader.kill();
            else if(writer.isAlive())
                writer.kill();

        } catch (SocketException e)
        {
            System.out.println("Server not available! Is it running?");
            System.out.println("Correct syntax is: java BullsAndCowsClient <hostname> <port>");
        }
        catch (InterruptedException e)
        {
            e.printStackTrace();
        }
    }
}

class Writer extends Thread
{
    //keep running while 'running' == true
    private volatile boolean running = true;
    private PrintWriter output;
    private Scanner scanner = new Scanner(System.in);

    public Writer(Socket sock) throws IOException
    {
        output = new PrintWriter(new BufferedWriter(new OutputStreamWriter(sock.getOutputStream())),true);
        start();
    }

    public void kill()
    {
        running = false;
    }

    public void run()
    {
        String str;
        while(running)
        {
            str = scanner.nextLine();
            if(str.equals("QUIT")) running = false;
            output.println(str);
        }

        output.close();
    }
}

class Reader extends Thread
{
    //keep running while 'running' == true
    private volatile boolean running = true;
    private BufferedReader input;

    public Reader(Socket sock) throws IOException
    {
        input = new BufferedReader(new InputStreamReader(sock.getInputStream()));
        start();
    }

    public void kill()
    {
        running = false;
    }

    public void run()
    {
        String str;
        while(running)
        {
            try
            {

                str = input.readLine();
                System.out.println(str);
            }
            catch (IOException ex)
            {
                running = false;
            }

        }
        try
        {
            input.close();
        } catch (IOException e)
        {
            e.printStackTrace();
        }
    }
}

Hopefully it all formatted correctly and I'm not using any truly abhorrent programming styles. Look forward to any and all feedback, thanks!

Answer 1

I also did not get to the Client. Here are some observations:

Don't use tombstone comments at the top of a class. Use proper JavaDoc which details the expected use of the class.

Star imports are discouraged. It's preferable to specify what classes you're importing in most cases.

Curly braces should be used even when they're optional. It's arguably pretty to avoid them, but they prevent an annoying class of error and their consistent use is considered a best practice.

Classes not designed for extension should be made final. Variables which should not change after initial assignment should also be made final. In addition to documenting design intent, they make it easier to read the code because you know they won't change.

In idiomatic Java, curly braces belong on the same line, not on a newline.

Resources that you open must be closed. The preferred means of doing so is the try-with-resources block. Not doing so can lead to resource leaks that eventually bring down your server / client applications or otherwise negatively affect the machine the code is running on.

Avoid abbreviations for variable names. Variable names should clearly indicate what they are.

Silently consuming exceptions is typically a very poor idea. It results in code that fails without giving you any clue as to where or why. Strongly consider always logging an exception.

Top-level classes should be declared in their own files. Classes that belong to another class should be declared inside the parent class. It's a bad idea to define multiple classes in the same file, because if there's a naming conflict between two such classes, the runtime behavior of your system is going to depend on which one the compiler sees first.

Types and variables should be as private as possible. Only expose functionality you expect and support external callers accessing.

Try to use Java generics properly. They really help out with type safety. You can turn on compiler warnings to give you a nudge.

ThreadWorker

This class is not a Thread. You haven't added generically useful functionality relevant to multiple clients who want to run a Thread. You have a specific block of code you want to run on it's own thread. The correct way to do that is to implement Runnable, and attach your code to a generic Thread instance.

This class probably shouldn't need to worry about what Socket called it. All it needs are the input and output streams, which can be passed in from the server instance.

It greatly enhances readability when complex strings are formatted neatly.

Consuming IOException from the socket stream and not failing upwards is dubious. I expect it will lead to ghost threads sitting around waiting on a socket that cannot possibly interact with the server.

playgame

Variables should be declared on their own lines, even if they share a type.

If you return from an if clause, you don't need an else clause.

In a non-toy application, it would be preferable to have the game track the guess state. ThreadWorker would pass in a guess and get back the number of bulls and cows. Given the nature of this application, that might be overkill here.

pause

It's a very bad idea to synchronize on yourself. You should always synchronize on a private member that no other class or instance knows about. The reason is that if somebody else synchronizes on your ThreadWorker instance, your pause method will be totally at their mercy as to if and when it might be able to run. In this case, you probably want a ReentrantReadWriteLock so you can read lock from resume as well.

Using a totally random key will only work because this is a toy application. In any real application you'd very quickly get conflicts and cranky users.

resume

resume() should probably take a key, and not a magic number it has to know to split and how.

The first error message is misleading. It's not the saved game that's missing, it's the whole file.

A saved game will live forever. It might be preferable to delete them from the file when they get resumed, but that requires streaming and rewriting the whole file. Perhaps a feature enhancement for later.

Your magic number 5000 is a bad idea. Better would be to store the games one to a line, and read the file line-by-line. Just check if the line starts with the key. If it does, then worry about splitting.

The return at the end of the method does nothing.

The application flow is a bit wonky here. If you resume a game, you play the entire game from within the context of the resume method. Might it make more sense to return the game, and then play it from the run method?

validate

Predicate methods such as validate typically begin with is or has. isValidTarget might be a better name. digs is a terrible abbreviation for digits. And using a Set would be easier to read than your div/mod math.

In help, you don't need the extra println.

BcGame

Java method names should use camelCase. Java class names should start with a capital letter.

There's no reason to append r at the end of some of your variable names in the constructor.

generateNum should be static, as it doesn't depend on the state of the class.

If you're going to use a Random, it's a good idea to pass it into the class that's using it. That helps you with testing later, because you can pass in a random with a known seed so you get consistent test results.

generateNum can be written more cleanly with Collections.shuffle(). It's somewhat less efficient, but easier to read. Additionally, trimToSize isn't buying you anything.

Objects should be created in a known good state. If it's necessary for a BcGame to have a name, that should be in the constructor. If it can't change, it should be final.

num isn't really descriptive. target might be better. Likewise name and playerName, and guesses and guessesUsed. For that matter, guessesRemaining might be a better abstraction, because then the game controls the guesses, not the ThreadWorker.

Answer 2

Nothing looks too bad, but there's some things you can improve when it comes to readability and other Java-specific stuff.

BullsAndCowsServer

//-------------------------
//  Multithreaded server program
//-------------------------

import java.net.*;
import java.io.*;

public class BullsAndCowsServer
{
    public static void main(String[] args) throws IOException
    {
        final int PORT = args.length > 0 ? Integer.parseInt(args[0]) : 1337;

        try (ServerSocket s = new ServerSocket())
        {
            System.out.println("Started: " + s);
            while (true)
            {
                try (Socket socket = s.accept())
                {
                    System.out.println("New client connected: " + socket.getRemoteSocketAddress());
                    new ThreadWorker(socket);
                }
                catch (IOException ignored)
                {
                }
            }
        }
        catch (SocketException e)
        {
            System.out.println("Server not available! Is it running?");
            System.out.println("Correct syntax is: java BullsAndCowsClient <hostname> <port>");
        }
    }

}

Changelog:

• If you're going to go with Allman style braces (which is totally fine), things like catch, finally, and else should all be on their own line.
• Typically when going with Allman, avoid having an extra new-line after the start of a code block. The point of Allman is to make things more readable, but adding the extra new-line just takes up space.
• Removed unnecessary parentheses around the ternary statement. You can see them both ways, but if it's a single conditional statement there's not really much need to have them.
• Use try-with-resources whenever possible. It will close your resources for you so you don't need to worry about it. This will greatly reduces the amount of code you use and makes it so much more readable.

ThreadWorker

//-------------------------
//  Thread class for server use
//-------------------------

import java.io.*;
import java.net.Socket;
import java.util.ArrayList;
import java.util.Random;

//this handles each game connection
public class ThreadWorker extends Thread
{
    private Socket socket;
    private BufferedReader in;
    private PrintWriter out;

    public ThreadWorker(Socket s) throws IOException
    {
        socket = s;
        in = new BufferedReader(new InputStreamReader(s.getInputStream()));
        out = new PrintWriter(new BufferedWriter(new OutputStreamWriter(s.getOutputStream())), true);
        start();
    }

    @Override
    public void run()
    {
        try
        {
            out.println("--------------------------------\n" +
                    "Welcome to the Bulls and Cows guessing game!\n" +
                    "Type any of the following options:\n" +
                    "HELP\n" +
                    "NEWGAME\n" +
                    "RESUME <passcode>\n" +
                    "--------------------------------");

            String str;
            while (!(str = in.readLine().toUpperCase()).equals("END"))
            {
                switch (str.split(" ", 10)[0])
                {
                    case "HELP":
                        help();
                        break;
                    case "NEWGAME":
                        newgame();
                        break;
                    case "RESUME":
                        resume(str);
                        break;
                    default:
                        out.println("Sorry, that's not a valid option; please try again!");
                        break;
                }

                out.println(
                        "--------------------------------\n" +
                        "Type any of the following options:\n" +
                        "HELP\n" +
                        "NEWGAME\n" +
                        "RESUME <passcode>\n" +
                        "--------------------------------");
            }
        }
        catch (IOException | NullPointerException ignored) { }
        finally
        {
            try
            {
                System.out.println(socket.getRemoteSocketAddress() + " disconnected.");
                socket.close();
            }
            catch (IOException ignored)
            {
            }
        }
    }

    private void newgame() throws IOException
    {
        BCGame game = new BCGame();

        out.println("\n---------------------\n" +
                "Greetings challenger.\n" +
                "What is your name?");
        game.name = in.readLine().replace(" ", "_");
        out.println("\n\n\n\n\n\n\n" +
                "Greetings " + game.name + ". Play by typing 4 digit numbers where each digit is unique.\n" +
                "You can type PAUSE at any time to receive a passcode allowing you to return later.\n" +
                "Goodluck!\n" +
                "---------------------------");
        playGame(game);
    }

    private void playGame(BCGame game) throws IOException
    {
        String str;
        while (game.guesses < 6)
        {
            int bulls = 0;
            out.println("\nYou have " + (6 - game.guesses) + " guesses remaining.\nPlease guess a number!");
            str = in.readLine();

            //pausing code
            if (str.equals("PAUSE"))
            {
                pause(game);
                return;
            }

            //check input is valid
            if (!validate(str))
            {
                out.println("\nInvalid input; please enter a 4 digit number with no duplicate digits");
                continue;
            }
            game.guesses++;

            //calculate score
            int cows = 0;
            for (int i = 0; i < 4; i++)
            {
                if (str.charAt(i) == game.num.charAt(i))
                    bulls++;
                else if (game.num.contains(str.charAt(i) + ""))
                    cows++;
            }

            //check win condition
            if (bulls == 4)
            {
                out.println("\n~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~");
                out.println("Congratulations, you are correct!\n" +
                        "You won after " + game.guesses + " guess" + ((game.guesses > 1) ? "es" : "") + "!");
                out.println("~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n");
                out.println("Press enter to return to the menu..");
                return;
            }
            else
            {
                out.println("Bulls: " + bulls + "\nCows: " + cows);
            }
        }
        out.println("\nSorry, you are out of guesses! The number was " + game.num + " \n" +
                "Better luck next time\n");
    }

    private synchronized void pause(BCGame game) throws IOException
    {
        out.println("\nPausing game...");
        String dir = System.getProperty("user.dir");
        File file = new File(dir + "/savedgames.txt");
        file.createNewFile();
        int key;
        try (FileWriter writer = new FileWriter(file, true)) {
            key = new Random().nextInt(99999);
            writer.append(String.valueOf(key))
                    .append(" ")
                    .append(game.name)
                    .append(" ")
                    .append(game.num)
                    .append(" ")
                    .append(String.valueOf(game.guesses))
                    .append(" ");
        }
        out.println("\nYour game has been saved.\nYou can resume at any time with the code " + key);
        in.readLine();
    }

    private void resume(String str) throws IOException
    {

        String gamekey = str.split(" ")[1];
        out.println("\nReloading game <" + gamekey + ">");

        String dir = System.getProperty("user.dir");
        File file = new File(dir + "/savedgames.txt");
        if (!file.exists())
        {
            out.println("Saved game not found, press enter to return to menu");
            in.readLine();
            return;
        }

        FileReader reader = new FileReader(file);
        char[] a = new char[5000];
        reader.read(a);
        String s = new String(a);
        String[] saved = s.split(" ");
        for (int i = 0; i < saved.length; i += 4)
        {
            if (saved[i].equals(gamekey))
            {
                try
                {
                    BCGame resumed = new BCGame(saved[i + 1], saved[i + 2], Integer.parseInt(saved[i + 3]));
                    out.println("Game found! Welcome back " + resumed.name);
                    playGame(resumed);
                    return;
                } catch(NumberFormatException e)
                {
                    System.out.println("Saved game is corrupt!");
                    break;
                }
            }
        }
        out.println("Saved game not found, press enter to return to menu");
        in.readLine();
    }

    private boolean validate(String str)
    {
        try
        {
            if (str.length() != 4) return false;

            int num = Integer.parseInt(str);
            boolean[] digs = new boolean[10];
            for(int i = 0; i < 4; i++)
            {
                if (digs[num % 10]) return false;
                digs[num % 10] = true;
                num /= 10;
            }
            return true;
        }
        catch(NumberFormatException e) { return false;}
    }

    private void help() throws IOException
    {

        out.println("\nIn this game you must guess the secret 4 digit number.\nEach digit is unique; ie. no digit " +
                "appears more than once." +
                "\nAfter each guess you will be told a number of bulls and cows.\nYou get a bull if" +
                " you guessed a digit in the correct place.\nYou get a cow if you guessed the digit correctly, but it " +
                "was in the wrong place.\n" +
                "Guess the number correctly in 6 attempts to win.\n Best of luck, challenger.");
        out.println("\nPress enter to return to the menu");
        in.readLine();
    }

    class BCGame
    {
        String num, name;
        int guesses;

        public BCGame()
        {
            guesses = 0;
            num = generateNum();
            name = "";
        }

        public BCGame(String namer, String numr, int guessesr)
        {
            guesses = guessesr;
            num = numr;
            name = namer;
        }

        //generates a 4 digit number in string form where all the digits are unique
        String generateNum()
        {
            ArrayList arr = new ArrayList();// = {"0", "1", "2", "3", "4", "5", "6", "7", "8", "9"};
            for (int i = 0; i < 10; i++) arr.add(i);

            String ret = "";

            Random rand = new Random();
            for (int i = 0; i < 4; i++)
            {
                int num = rand.nextInt(arr.size());
                ret += arr.remove(num);
                arr.trimToSize();
            }
            return ret;
        }
    }
}

Changelog:

• Have classes be public if they are the same as the file name.
• Typically in Java you use private or protected variables and then have getters/setters unless they are constants, which socket, in, and out are not.
• If a method overrides a parent method (which run() does), make sure you have @Override
• If you have an empty catch block, name the exception in it ignored
• If you add a line break after each \n, it makes the actual output much easier to understand.
• Do you want to handle lowercase options? If so, call str.toUpperCase().
• You can read in the line from within the while condition;
• try-with-resources is your friend!
• Every time you do + on a String, it will create a new StringBuilder behind the scenes. Avoid it whenever you can (especially when you have convenient access to an appender)
• No need to have return as the last statement in a method
• You can chain String calls all you want
• Always name classes starting with a capital letter
• If you have multiple classes in one file, have them be an inner class. Don't have them outside of one another.

BullsAndCowsClient

I don't have time to go through this one, but the feedback from the previous two should help you out getting that one cleaned up. Enjoy the OOP life!