2
\$\begingroup\$

This program asks the user for a players name, # of goals, saves, games played, etc. This information is ten organized on a .txt file along with the time the information was logged. users can also compare the players based on # of goals, Assists and saves made.

If anything can be improved, cleaned or replaced let me know.

Main:

import java.io.*;
import java.nio.file.Path;
import java.nio.file.Paths;

public class Main {

    public static void main(String[] args){

        Path path = Paths.get("/Users/Coding/Desktop/myFile.txt").toAbsolutePath();
        try(
            BufferedWriter out = new BufferedWriter(new FileWriter(String.valueOf(path)))){
            Player player = new Player(out, path);

            player.menu();
        } catch (IOException e) {
            e.printStackTrace();
        }
    }
}

Player:

import java.io.BufferedWriter;
import java.io.IOException;
import java.nio.file.Path;
import java.time.LocalDateTime;
import java.time.format.DateTimeFormatter;
import java.util.*;

public class Player {
    Path path;
    BufferedWriter out;
    Scanner scan;
    HashMap<String, int[]> map;


    public Player(BufferedWriter out, Path path) {
        this.out = out;
        this.scan = new Scanner(System.in);
        this.map = new HashMap<>();
        this.path = path;
    }

    public void menu() throws IOException {
        String task;
        out.write("Name      GP  G  A  P  S  S%\n");

        do {
            System.out.print("What would you like to do today? 1: Add Data, 2: Get The Top Player, 3: Exit the Program: ");
            task = scan.nextLine();
            switch (task) {
                case "1" -> addData();
                case "2" -> getTopPlayers();
                case "3" -> {
                    System.out.println("Goodbye!");
                    System.exit(0);
                }
            }
        } while (!task.equals("3"));
    }

    void addData() {
        boolean cont;

        do try {
            System.out.print("Player First Name: ");
            String firstName = scan.nextLine() + " ";

            System.out.print("Player Last Name: ");
            String lastName = scan.nextLine();

            String fullName = firstName.concat(lastName);

            System.out.print("Enter Number of Games Played: ");
            int gamesPlayed = Integer.parseInt(scan.nextLine());

            System.out.print("Enter Number of Goals Made: ");
            int goals = Integer.parseInt(scan.nextLine());

            System.out.print("Enter Number of Assists Made: ");
            int assists = Integer.parseInt(scan.nextLine());

            System.out.print("Enter Number of Points Scored: ");
            int points = Integer.parseInt(scan.nextLine());

            System.out.print("Enter Number of Saves Made: ");
            int saves = Integer.parseInt(scan.nextLine());

            System.out.print("Enter Number of Shots Made: ");
            int shotsOnGoal = Integer.parseInt(scan.nextLine());

            int[] data = {gamesPlayed, goals, assists, points, saves, shotsOnGoal};

            DateTimeFormatter log = DateTimeFormatter.ofPattern("yyyy/MM/dd HH:mm:ss");
            LocalDateTime time = LocalDateTime.now();
            String logTime = log.format(time);

            map.put(fullName, data);
            out.write("==================================\n");
            out.write(fullName + " : " + Arrays.toString(data) + " (" + logTime + ")");
            out.flush();
            out.newLine();

            cont = false;
        } catch (NumberFormatException | IOException e) {
            System.out.println("Enter Valid Input");
            cont = true;
        } while (cont);

    }

    void getTopPlayers() {
        int index = 0;
        System.out.println("What Stat Do You Want To Find? Goals, Assists, Points, Saves: ");
        String choice = scan.nextLine().trim().toLowerCase();
        switch (choice) {
            case "goals" -> index = 1;
            case "assists" -> index = 2;
            case "points" -> index = 3;
            case "saves" -> index = 4;
            default -> System.out.println("Not A Valid");
        }
        int finalIndex = index;
        var max = map.values().stream().mapToInt(e -> e[finalIndex]).max().getAsInt();
            for (Map.Entry<String, int[]> entry : map.entrySet()) {
                if (entry.getValue()[index] == max) {
                    System.out.println(entry.getKey());// Print the key with max value
                    break;
                }
            }
        }
}
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Path

The use of Path is a bit pointless.

  • There is no need to use an absolute path to access the file.
  • Converting the Path to a String seems wrong. Conversion to a File with the method .toFile() would make more sense.
  • IF you convert to a String, then the method .toString() may be more conventional than using String.valueOf().
  • You pass the path to the Player class but don't use it in there.

So either directly use a String:

String path = "/Users/Coding/Desktop/myFile.txt";

try (Writer out = new BufferedWriter(new FileWriter(path))) {

or convert to a File:

Path path = Paths.get("/Users/Coding/Desktop/myFile.txt"); // .toAbsolutePath is not needed

try (Writer out = new BufferedWriter(new FileWriter(path.toFile()))) {

Names

Choose variable and class names that represent their use and make reading the code easier. For example, the class Player doesn't represent "a player", so that's a bad name. Instead it's the main class of the application, so something like PlayerStatsApp would be better.

Also the name out for the BufferedWriter is easily confused with System.out. fileOutput or writer would be a better names. (See also the remarks to the BufferedWriter further down).

Also scan should be called scanner, and map is also too generic. Something like players or playerStats would be better.

Program to interfaces

The Player class doesn't need to "know" that you are specifically passing in a BufferedWriter, instead declare the constructor parameter and field simply as a Writer.

And the map field should be similarly just declared a Map instead of a HashMap.

Bug/Hidden feature

In getTopPlayers when the user enters an invalid number you output an error message, but still execute the logic with the index 0 which actually returns the player(s) with the most games.

Instead use the switch expression actually as an expression, and catch the error case with a value such as -1. Example:

void getTopPlayers() {
    System.out.println("What Stat Do You Want To Find? Games, Goals, Assists, Points, Saves: ");
    String choice = scan.nextLine().trim().toLowerCase();

    int index = switch (choice) {
        case "games" -> 0; // New case to properly access "games played"
        case "goals" -> 1;
        case "assists" -> 2;
        case "points" -> 3;
        case "saves" -> 4;
        default -> -1;
    }

    if (index == -1) {
        System.out.println("Not A Valid");
        return;
    }

    // ...
}

(This also makes index "final-like", so you no longer need to make the finalindex copy.)

Data structure

Storing the data in a Map is a pointless, since you are not using it as a map. Also while storing the stats in an array makes accessing the separate fields simple with the index, it doesn't make good readable code.

Instead create a class to store all the data of a player (name and stats) and store them in a list. For example:

class PlayerStats { // Or maybe just "Player"

    private final String firstName;
    private final String lastName;

    private final int gamesPlayed;
    private final int goalsMade;
    // etc.

    PlayerStats(String firstName, String lastName, int gamesPlayed, int goalsMade /* etc. */) {
        this.firstName = firstName;
        this.lastName = lastName;
        this.gamesPlayed = gamesPlayed;
        this.goalsMade = goalsMade;
    }

    public String getFirstName() {
        return firstName
    }

    public String getLastName() {
        return lastName
    }

    public int getGamesPlayed() {
        return gamesPlayed;
    }

    public int getGoalsMade() {
        return goalsMade;
    }
    // etc.

    public String getFullName() {
         return firstName + " " + lastName;
    }
}

However this makes accessing them a bit more advanced. Instead of an index you have to use method references. Following on from the example above for getTopPlayers() the switch becomes:

Function<PlayerStats, Integer> method = switch (choice) {
        case "games" -> PlayerStats::getGamesPlayed;
        case "goals" -> PlayerStats::getGoalsMade;
        // etc.
        default -> null;
    }

if (method == null) {
    System.out.println("Not A Valid");
    return;
}

And then, when you use a list instead of a map (List<PlayerStats> playersStats) the logic becomes (using a stream for the second part, too):

var max = playersStats.stream().mapToInt(method).max().getAsInt();

playersStats.stream()
    .filter(player -> method.apply(player) == max)
    .forEach(player -> System.out.println(player.getFullName()));

(BTW, it is possible to do this logic with a single loop/stream.)

Separation of concerns

One thing I'd should mention is separation of concerns. One method (or class) should not contain (for example) input and output functions on the one hand and functional logic on the other hand. But this answer is long enough now, so I won't go into that further right now.

Other

Catching IOException and just printing stack is pointless. That is what happens when the exception isn't caught anyway. Only catch exceptions if you specifically can do something about it.

Don't exit a program somewhere in the middle with System.exit(0);. Instead just let it run to the end of the main method. In this case, since you catch that case in the while expression, just remove the line.

Catching the NumberFormatException for all inputs at once makes input a bit irritating, if you have to start from the start any time you mistype.

\$\endgroup\$
1
  • \$\begingroup\$ Couple problems updating the code. 1: The Stats arent printed onto the text file when the List is printed. 2: ClassCastException on the line var max = playerData.stream().mapToInt((ToIntFunction<? super PlayerData>) method).max().getAsInt(); I was getting an error on 'method' and so the IDE suggested to cast ToIntFunction \$\endgroup\$ Jun 8, 2021 at 23:53

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.