2
\$\begingroup\$

Kindly accept my apologies as Java is something my mind cannot digest no matter what I do. At the moment I have an assignment and have written a piece of code and have used Constructor (I think so) but not sure if I have used it correctly and if Yes then will need some guidance in order to refactor the code.

Anyhow I am writing code for a Booking System where user is given option to input value and create a new Show along with date. I have achieved this in the below code but note sure that I have used the constructor properly or not.

import java.io.*;
import java.util.*;

public class LocalPlay extends Play {

    private static String playName;
    private static String playDate;

    private LocalPlay(String playName, String playDate) {
        this.playName = playName;
        this.playDate = playDate;
    }

    public String getShowName() {
        return playName;
    }

    public String getShowDate() {
        return playDate;
    }

    public void setShowName(String value) {
        this.playName = value;
    }

    public void setShowDate(String date) {
        this.playDate = date;
    }

    public static void lPlayDetails() {
        try {
            int selection = 0;
            Scanner option = new Scanner(System.in);
            System.out.println();
            System.out.println("Press 1 to Add a New Play/ Show: ");
            System.out.println("Press 2 to View existing Play: ");
            System.out.println("Press 3 to delete existing Play: ");
            System.out.println("Press 4 for Previous Menu: ");
            System.out.println();
            System.out.print("Kindly make the selection: ");
            selection = option.nextInt();
            if (selection == 1) {
                addPlay();
            } else if (selection == 2) {
                viewPlay();
            } else if (selection == 3) {
                deletePlay();
            } else if (selection == 4) {
                managePlay();
            } else
                while (selection != 1 || selection != 2 || selection != 3) {
                    System.out.println();
                    System.out.println(selection + " is not a valid option. ");
                    managePlay();

                }
        } catch (InputMismatchException ime) {
            System.out.println("Invalid option selected.");
            System.out.println("Try again");
            lPlayDetails();
        }
    }

    private static void addPlay() {
        String name;
        String date;
        Scanner option = new Scanner(System.in);
        System.out.print("Enter name of Play: ");
        name = option.nextLine();
        System.out.print("Enter date of Play: ");
        date = option.nextLine();
        LocalPlay lp = new LocalPlay(name, date);
        System.out.println("The name of play is " + playDate + " date played is " + playDate);
        lPlayDetails();
        BufferedWriter writer = null;
        try {
            writer = new BufferedWriter(new FileWriter("play.txt"));
            writer.write(name + " " + date);

        } catch (IOException e) {
        } finally {
            try {
                if (writer != null)
                    writer.close();
            } catch (IOException e) {
            }
        }
        return;
    }

    private static void deletePlay() {
        System.out.println("Method not implemented");
        lPlayDetails();

    }

    private static void viewPlay() {
        System.out.println("Method not implemented");
        lPlayDetails();

    }

}
\$\endgroup\$
0

2 Answers 2

3
\$\begingroup\$

Structure

Generally, a method should do one thing. For example addPlay should add a play (although it's an odd method for a play class, as a play generally cannot own another play). But your method doesn't just add something, it actually retrieves user input, prints stuff, writes stuff to a file, and then calls a seemingly unrelated method, which asks for more user input and processes it in some way.

It's also very unclear what your LocalPlay class is actually supposed to do. Looking at the fields, constructor, and getter/setter, it looks as if it holds the data for one play, but then it is also possible to add a play, which doesn't make sense.

You should definitely try to separate the holding of the data from the parsing of user input.

You should also try to better define the tasks and differences of lPlayDetails and addPlay, and you should try to remove the confusing call structure (the functions call each other, which makes it difficult to follow the control flow of your program).

Misc

  • there is no reason to make the fields (or the methods) static.
  • your naming is a bit confusing. There doesn't seem to be a difference between a play and a show, you seem to be using both interchangeably. In that case, use just one of the terms for clarity.
  • if a method is not implemented, it shouldn't do anything. Instead of doing something totally different, throw something like an UnsupportedOperationException.
  • lPlayDetails: It's very unclear what this method does. The name is vague and it is missing a JavaDoc comment. It is also very unclear what the exact difference to addPlay is.
  • it is very unclear to me why you create a new LocalPlay in addPlay. There doesn't seem to be a reason for it, and it seems to be unused.
\$\endgroup\$
1
  • \$\begingroup\$ Hi Tim,Thanks for the suggestions, actually local play Class is to create a play locally as in my program there is another option for Foreign Play as well, so that is why I created addPlay() method in LocalPlay class and also thinking to create same method in Foreign Play class as well. Furthermore I am calling lPlayDetails() method as I do not want my program to stop executing and this is the solution I got inorder to keep the program going. Will appreciate if you can suggest a better solution to me for this as well. \$\endgroup\$
    – Baig
    Commented Apr 15, 2016 at 8:14
0
\$\begingroup\$

In addition to @tim's answer...

static fields and methods

private static String playName;
private static String playDate;

The static modifier means that all LocalPlay objects will be sharing the same fields, which is likely not what you want. Furthermore, the getter and setter methods should not be static too: they should 'belong' to each instance.

try-with-resources

Your Scanner and BufferedWriter usage should be done through try-with-resources for safe and efficient handling of the underlying I/O resources:

public static void lPlayDetails() {
    try (Scanner scanner = new Scanner(System.in)) {
        // use scanner here, or pass it to other methods that require user inputs
    }
}

private static void addPlay(Scanner scanner) {
    System.out.print("Enter name of Play: ");
    String name = scanner.nextLine();
    // ...
    try (FileWriter fileWriter = new FileWriter("play.txt");
            BufferedWriter writer = new BufferedWriter(fileWriter)) {
        writer.write(name + " " + date);
    }
}

Input validation

System.out.print("Enter date of Play: ");
date = option.nextLine();

What happens if the user entered an empty String or a nonsensical input like "abc"?

Method calling

deletePlay() and viewPlay() should not be calling lPlayDetails(), having been called by that very method. They should just return - not literally with the return; statement, but implicitly at the end of the method.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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