5
\$\begingroup\$

I wrote a simple program to create a random order for an NBA draft, prompt the user for input of draft picks, print the order of the draft and the results, and write the results to a text file.

This is the first thing I've written from scratch so I was curious to see if there was anything that I needed to improve, add or get rid of in order to make the program run better.

package com.company;
import java.io.*;
import java.util.*;
import javax.swing.*;

public class Main {

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

        NbaDraft nbaDraft1 = new NbaDraft(); // Create object of class w/ methods to help create draft

        ArrayList<String> nbaTeams = new ArrayList<>(); // create ArrayList to hold all 30 NBA teams
        nbaTeams.addAll(nbaDraft1.addNbaTeams()); // call method to add teams in alphabetical order

        ArrayList<String> draftOrder = new ArrayList<>(); // create ArrayList to hold teams in the order they'll pick
        draftOrder.addAll(nbaDraft1.createDraftOrder(nbaTeams)); // create order from team list, add to order list
        nbaDraft1.printDraftOrder(draftOrder); // print draft order

        ArrayList<String> draftPlayers = new ArrayList<>(); // create ArrayList to hold players the teams pick
        draftPlayers.addAll(nbaDraft1.draftPlayers(draftOrder)); // prompt for players, add to list

        ArrayList<String> draft1 = new ArrayList<>();
        draft1.addAll(nbaDraft1.createDraft(draftOrder, draftPlayers));
        // create ArrayList to hold draft info (teams in proper order associated w/ players they drafted)
//        nbaDraft1.printDraft(draft1); // print draft (Un-comment code in order to print draft instead of
        // reading from text file to be created below)

        CreateDraftFile draft = new CreateDraftFile();
        // create object of class w/ methods to write and read file that'll hold your particular draft
        draft.writeToFile(draft1); // call method to write your draft to a text file
System.out.println();
        draft.readDraftFile(); // call method read and print the info on file
    }
}

public class NbaDraft {

    private ArrayList<String> list;
    private ArrayList<String> order;
    private ArrayList<String> picks;
    private ArrayList<String> draft;
    private String rounds;
    private int numRounds;

    public NbaDraft() {
        list = new ArrayList<>();
        order = new ArrayList<>();
        picks = new ArrayList<>();
        draft = new ArrayList<>();
        numRounds = 0;
    }

    public ArrayList<String> addNbaTeams(){

        String teams = ("Atlanta Hawks,Boston Celtics,Brooklyn Nets,Charlotte Hornets,Chicago Bulls," +
                "Cleveland Cavaliers,Dallas Mavericks,Denver Nuggets,Detroit Pistons,Golden State Warriors" +
                ",Houston Rockets,Indiana Pacers,Los Angeles Clippers,Los Angeles Lakers,Memphis Grizzlies" +
                ",Miami Heat,Milwaukee Bucks,Minnesota Timberwolves,New Orleans Pelicans,New York Knicks," +
                "Oklahoma City Thunder,Orlando Magic,Philadelphia 76ers,Phoenix Suns,Portland Trailblazers" +
                ",Sacramento Kings,San Antonio Spurs,Toronto Raptors,Utah Jazz,Washington Wizards");

        String[] data = teams.split(",");

        list.addAll(Arrays.asList(data));

        return list;
    }

    public ArrayList<String> createDraftOrder(ArrayList<String> teams){
        while (numRounds == 0) {
            try {
                rounds = (JOptionPane.showInputDialog("How many rounds will the draft have? (1-5)"));
                numRounds = Integer.parseInt(rounds);
            } catch (NumberFormatException e) {
                JOptionPane.showMessageDialog(null, "Wrong data type, please enter an integer");
            }
        }

        Collections.shuffle(teams);
        order.addAll(teams);

            for (int i = 0; i < (numRounds - 1); ++i) {
                Collections.reverse(teams);
                order.addAll(teams);
            }

        return order;
    }

    public void printDraftOrder(ArrayList<String> list){

        if(numRounds == 0){
            System.out.println("Draft cancelled!");
        }
        if(numRounds>=1) {
            System.out.println("\nRound 1: \n");
            for (int x = 0; x < 30; ++x) {
                System.out.println((x + 1) + ". " + list.get(x));
            }
        }
        if(numRounds >= 2) {
            System.out.println("\nRound 2: \n");
            for (int x = 30; x < 60; ++x) {
                System.out.println((x + 1) + ". " + list.get(x));
            }
        }

        if(numRounds >= 3) {
            System.out.println("\nRound 3: \n");
            for (int x = 60; x < 90; ++x) {
                System.out.println((x + 1) + ". " + list.get(x));
            }
        }

        if(numRounds >= 4) {
            System.out.println("\nRound 4: \n");
            for (int x = 90; x < 120; ++x) {
                System.out.println((x + 1) + ". " + list.get(x));
            }
        }

        if(numRounds == 5) {
            System.out.println("\nRound 5: \n");
            for (int x = 120; x < 150; ++x) {
                System.out.println((x + 1) + ". " + list.get(x));
            }
        }
    }

    public ArrayList<String> draftPlayers(ArrayList<String> players){

        for (int i=0; i<players.size(); ++i){
            String pick = JOptionPane.showInputDialog("Pick #" + (i+1) + ": \nEnter your draft selection for the " + players.get(i));
            if(pick != null){
                picks.add(pick);
            }
            else {
                break;
            }
        }
        return picks;
    }

    public ArrayList<String> createDraft(ArrayList<String> order, ArrayList<String> picks) {
        for (int x=0; x<picks.size(); ++x) {
            if((order.get(x)!= null) && (picks.get(x)!= null)) {
                    draft.add((x + 1) + ". " + order.get(x) + ": " + picks.get(x));
            }
        }
        return draft;
    }

    public void printDraft(ArrayList<String> draft){

            if (numRounds == 0) {
                System.out.println("Draft cancelled!");
            }

            if (numRounds >= 1) {
                System.out.println("\nRound 1: \n");
                for (int x = 0; x < 30; ++x) {
                    System.out.println(draft.get(x));
                }
            }

            if (numRounds >= 2) {
                System.out.println("\nRound 2: \n");
                for (int x = 30; x < 60; ++x) {
                    System.out.println(draft.get(x));
                }
            }

            if (numRounds >= 3) {
                System.out.println("\nRound 3: \n");
                for (int x = 60; x < 90; ++x) {
                    System.out.println(draft.get(x));
                }
            }

            if (numRounds >= 4) {
                System.out.println("\nRound 4: \n");
                for (int x = 90; x < 120; ++x) {
                    System.out.println(draft.get(x));
                }
            }

            if (numRounds == 5) {
                System.out.println("\nRound 5: \n");
                for (int x = 120; x < 150; ++x) {
                    System.out.println(draft.get(x));
                }
            }
        }
}

public class CreateDraftFile {
    private Writer draftFile;
    private InputStream inDraft;
    private BufferedReader draft;
    private File file;

    public CreateDraftFile() {
        try {


String fileName = JOptionPane.showInputDialog("Input file name: ");
        String filePath = JOptionPane.showInputDialog("Input file path (aside from file name, end with \\\\): ");
        file = new File(filePath + fileName + ".txt");
                draftFile = new BufferedWriter(new FileWriter(file));
                inDraft = new FileInputStream(file);
                draft = new BufferedReader(new InputStreamReader(inDraft));
            }
            catch (IOException e){
                e.getMessage();
            }
        }

    public void writeToFile(ArrayList<String> draftInfo) throws IOException {
        try {
            String separator = System.getProperty("line.separator");
            for (int i=0; i<draftInfo.size(); ++i) {
                draftFile.write(draftInfo.get(i) + separator);
            }
        }
        catch (IOException e){
            e.getMessage();
        }
        draftFile.close();
    }

    public void readDraftFile() throws IOException {
            String s;
            while((s = draft.readLine())!= null){
            System.out.println(s);
        }
            draft.close();
    }

}

}
\$\endgroup\$
5
\$\begingroup\$

Thanks for sharing your code,

Full disclaimer: I know nothing about NBA, so I'll just be talking about the code itself :)

Avoid returning references to mutable objects, here you return the underlying ArrayList objects, this means that the caller can edit these lists and it will (most likely unintentionally) mutate your NbaDraft object.

Consider this example

ArrayList<String> privateList = draft.createDraftOrder(someOtherList);
privateList.add("Hello"); // draft private variable has now changed!

To prevent this you can return a Defensive Copy

return new ArrayList<String>(order);

Throughout your code you deal with plenty of ArrayLists, however you should Program to an Interface. When you declare a variable as an ArrayList, all of the calling code will be locked into using this Implementation.

I would declare your variables like this;

private List<String> list;
private List<String> order;
private List<String> picks;
private List<String> draft;

The code in your constructor can stay the same. You can change all of the return types and arguments to List as well, instead of ArrayList, this will allow the calling code to provide any implementation of the List interface instead of just an ArrayList. Now in this example, an ArrayList is perfectly fine and probably the best choice, but in general it's best practice to program to an Interface if able.

In your addNbaTeams method, at the moment the team names are all hard coded. This means if you wanted to add any new teams, you'd have to go in and change the source code of your NbaDraft class. I would prefer to see a construction like this

NbaDraft nbaDraft1 = new NbaDraft(myListOfTeamNames);

This would make your NbaDraft object more robust, could provide it with any number of team names to deal with. You could also easily then keep a file full of all of these names so as not to clutter the code itself.

You have a printDraft method, it would be more flexible if this method simply returned a String instead, and then you can just print it, or use it for any other purpose. Eg. in a JPane/JPanel or a JavaFX window. at the moment you're constricting yourself to ONLY printing to the console.

String draftDisplay = nbaDraft.printDraft();
System.out.println(draftDisplay);
Logger.info(draftDisplay);

You have a CreateDraftFile class, I don't think I liked the name of this class. In general you want to name a class what the object will do, not what you want to do with the object.

Here you want to create a file, so CreateDraftFile was chosen as the name, but your object will be doing the creating, I would maybe consider DraftFileCreator or something along those lines. And createDraftFile could be a method (what you want the object to do for you)

You seem to be doing a lot of work in your main method that doesn't need to be there, I think the body of your main method could be as short as this.

List<String> teamNames = ... // get this from anywhere
NbaDraft draft= new NbaDraft(teamNames);
DraftFileCreator dfc = new DraftFileCreator(draft); // notice how I pass in the NbaDraft object as an argument to the constructor
dfc.writeToFile(draft1);
System.out.println();
dfc.readDraftFile();

In your current main method, you do lots of List manipulation, all of this can be done elsewhere, you shouldn't need to know how it works.

As a side note, there are a lot of redundant comments in your code. Comments should say WHY you're doing something, not WHAT you're doing. I know that when I see object.method() that you're calling a method, but I might not know why you're saying index - 1 instead of just index, for example.

I hope this was helpful for you, keep it up!

\$\endgroup\$
1
\$\begingroup\$

In addition to @chatton's answer...

Creating String arrays

These can be better created with Arrays.asList(T):

public static final List<String> NBA_TEAMS = Arrays.asList(
                        "Atlanta Hawks",
                        "Boston Celtics",
                        "Brooklyn Nets",
                        /* ... */ );

Oh wait, you did use that, then I guess this advice here is to let you know that method accepts varargs, which is just syntatic sugar for the String[] array you are constantly creating.

Working with numbers and code de-duplication

With a bit of math, you can de-duplicate your for-loops as such:

for (int i = 1; i <= numOfRounds; i++) {
    System.out.println("\nRound " + i + ":\n");
    for (int j = (i - 1) * 30; j < i * 30; j++) {
        System.out.println((j + 1) + ". " + list.get(j));
    }
}

I/O

You should use try-with-resources for safe and efficient use of the underlying I/O resource.

Another tip to consider when writing to files is that Files.write(Path, Iterable, OpenOption can do the job in a simpler way. For example:

Files.write(destinationPath, draftInfo, StandardOpenOption.TRUNCATE_EXISTING);

This will overwrite the file every time, and there are other options listed under StandardOpenOption.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.