7
\$\begingroup\$

I started learning the Java language about 10 days ago, reading Introduction to Programming Using Java by David J. Eck. I've made it halfway through chapter six in about 9 days. I realized I was going way too fast to learn efficiently, and so I'm going back to reread the focal points of the chapters and finish the exercises in more detail.

I've written out a program that allows the user to choose to read or write to a file, whether it be existent or new. They also may choose to write to a file specified by a name, or chosen from the file tree via window (I did not code that part; I believe it is in the TextIO class). The reading class allows the user to read from the last file, or one specified from the file tree via window (see above).

Question: What are some ways to reformat the code for a more proper semantic approach, or for better flow of program? I'm concerned with how often Eclipse complained about referring to a static in a non-static field, and so (nearly?) everything had to be declared public static. Also, what might be some good ways to extend a program like this - assuming my minimal experience with the language - and also provide a good learning curve/challenge?

Link to GitHub repository

public class Filer {

final static int read = 1;
final static int write = 2;

public static void main(String[] args) {

    pl("********************************************************");
    pl("Welcome, and thank you for using Text File");
    pl("Manager, Version 0.0 . ");
    pl("");
    pl("********************************************************");
    pl("This software was created by Phil Carpenter at");
    pl("Tempest Design Studios © 2015");
    pl("This program allows you to read and write to");
    pl("files using a few different methods.");
    pl("");
    pl("********************************************************");
    pl("You may either choose to either write to a file, or");
    pl("read from an existing file.");
    pl("You may also either name a file, or choose one from the file");
    pl("tree screen.");
    pl("");
    pl("********************************************************");
    pl("");
    p("Would you like to run the program?:   ");

    boolean running = TextIO.getlnBoolean();
    while(running){
        pl("Would you like to read the contents of an existing file [1], or");
        p("rather write to a file [2]?:     ");

        int choice = TextIO.getlnInt();


        while(choice != read && choice != write){
                   pl("********************************************************");
            p("Please choose a valid option [1 or 2]:    ");
            choice = TextIO.getlnInt();
        } // end while(choice) //

        if(choice == read) {
            FileRead.Reader();
        } else if (choice == write) {
            FileWrite.Writer();
        } else; // end if(choice) //

        p("Run program again?:         ");
        running = TextIO.getlnBoolean();
    } // end while(running) //


    pl("********************************************************");
    pl("////////////////////////////////////////////////////////");
    pl("");
    pl("");
    pl("Thank you for choosing Tempest's Software! We hope it suited");
    pl("your needs completely.");
    pl("");
    pl("");
    pl("\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\");
    pl("********************************************************");

        System.exit(0);

    }



    public static void pl(String x) {
        TextIO.putln(x);
    } // end pl() //

    public static void p(String y) {
        TextIO.put(y);
    } // end p() //

    public static void pf(String z) {
        TextIO.putf(z);
    } // end pf() //

}

FileRead

public class FileRead {

    public static void Reader() {
        TextIO.putln("********************************************************");
        TextIO.putln("********************************************************");
        TextIO.putln("You are now using VeryBasic File Reader v.0,");
        TextIO.putln("created by TDS/2015.");
        TextIO.putln("");
        TextIO.putln("");
        TextIO.putln("********************************************************");
        TextIO.putln("Would you like to read from the last file in this");
        TextIO.put("session [1], or choose a file from the tree [2]?     ");

        int choice = TextIO.getInt();
        if (choice == 1){
            ReadLast();
        } else {
            ReadUser();
        } // end if(choice) //
    } // end Reader() //

    public static void ReadUser() { 
        TextIO.putln("********************************************************");
        TextIO.putln("Would you like to begin reading a file of");
        TextIO.put("your choosing?:     ");
        boolean reading = TextIO.getBoolean();
        while (reading) {
            TextIO.putln("");
            TextIO.putln("");
            TextIO.readUserSelectedFile();
            String y = TextIO.getln();
            TextIO.writeStandardOutput();
            TextIO.putln("********************************************************");
            TextIO.putln("Here is the data from the file you've");
            TextIO.putln("requested:    ");
            TextIO.putln("");
            TextIO.putln("********************************************************");
            TextIO.putln("////////////////////////////////////////////////////////");
            TextIO.putln("");
            TextIO.putln("");
            TextIO.putln(y);
            TextIO.putln("");
            TextIO.putln("");
            TextIO.putln("\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\");
            TextIO.putln("********************************************************");
            TextIO.readStandardInput();
            TextIO.put("Read from another file?:     ");
            reading = TextIO.getBoolean();
        } // end while(reading) //

    } // end ReadUser() //

    public static void ReadLast() {
        TextIO.putln("********************************************************");
        TextIO.putln("You are now using VeryBasic File Reader v.0");
        TextIO.putln("created by TDS/2015");
        TextIO.putln("");
        TextIO.putln("");
        String file = new String();
        String contents = new String();
        TextIO.putln("********************************************************");
        TextIO.putln("Would you like to begin reading from the last");
        TextIO.put("file written to in this session?     ");
        boolean reading = TextIO.getBoolean();
        while(reading) {
            TextIO.putln("");
            TextIO.putln("");
            file = FileWrite.lastfile;
            TextIO.putln("********************************************************");
            TextIO.putln("Here are the contents of the last file");
            TextIO.putln("you worked on this session, if any.");
            TextIO.putln("");
            TextIO.readFile(file);
            contents = TextIO.getln();
            TextIO.writeStandardOutput();
            TextIO.putln("********************************************************");
            TextIO.putln("////////////////////////////////////////////////////////");
            TextIO.putln(contents);
            TextIO.putln("\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\");
            TextIO.putln("********************************************************");
            TextIO.readStandardInput();
            TextIO.putln("");
            TextIO.put("Would you like to print it again?    ");
            reading = TextIO.getBoolean();
        } // end while(reading) //

    } // end ReadLast() //


}

FileWrite

public class FileWrite {

    public static void Writer() {

        pl("********************************************************");
        pl("********************************************************");
        pl("You are now using VeryBasic File Writer v.0,");
        pl("created by TDS/2015.");
        pl("");
        pl("");
        pl("********************************************************");
        pl("Note that when writing to a file:");
        pl("");
        pl("If the file does not exist, it will be");
        pl("created and written to.");
        pl("");
        pl("Existing files will be replaced, or");
        pl("\"overwritten\"...");
        pl("");
        pl("All writing will be copied as written;");
        pl("whitespace and symbols included.");
        pl("********************************************************");
        pl("");
        pl("Would you like to specify a file name [1],");
        p("or choose a file from the tree [2]?     ");

        int choice = TextIO.getInt();
        if (choice == 1){
            WriteFile();
        } else {
            WriteUser();
        } // end if(choice) //

    } // end Writer() //

    public static String lastfile = new String();

    public static void WriteUser() {
        boolean writing;
        do {
            pl("********************************************************");
            pl("Begin writing content to fill file.");
            pl("");
            pl("Return a line with a single SPACE or"); 
            pl("\"\\n\" to represent line breaks in your");
            pl("writing procedure.");
            pl("");
            pl("Return an EMPTY line when finished writing,");
            pl("and you will be then prompted to select a file");
            pl("to save your writing to.");
            pl("********************************************************");

            String input = TextIO.getln();
            String value = new String();

            do {
                if (input.equals(" ") || input.equals("\\n")) {
                    value += "\\n" + "\\n";
                } else {
                    value += input + " ";
                } // end if/else //
                input = TextIO.getln();
            } while(!(input.equals(""))); // end do/while(input) //
            pl("********************************************************");
            pl("Please choose a file to write/rewrite.");
            pl("");
            pl("");
            TextIO.writeUserSelectedFile();
            lastfile = TextIO.getOutputFileName();
            pl(value);
            TextIO.writeStandardOutput();
            pl("");
            pl("********************************************************");
            pl("Would you like to write to another file?");
            writing = TextIO.getlnBoolean();        
        } while(writing);// end do/while(writing) //

    } // end WriteUser() //

    public static void WriteFile() {
        boolean writing;
        do{
            pl("********************************************************");
            pl("Begin writing content to fill file.");
            pl("");
            pl("Return a line with a single SPACE or"); 
            pl("\"\\n\" to represent line breaks in your");
            pl("writing procedure.");
            pl("");
            pl("Return an EMPTY line when finished writing,");
            pl("and you will be then prompted to select a file");
            pl("to save your writing to.");
            pl("********************************************************");

            String input = TextIO.getln();
            String value = new String();

            do {
                if (input.equals(" ") || input.equals("\\n")) {
                    value += "\\n" + "\\n";
                } else {
                    value += input + " ";
                } // end if/else //
                input = TextIO.getln();
            } while(!(input.equals(""))); // end do/while(input) //
            pl("********************************************************");
            String filename = new String();
            p("Please specify a file name:      ");
            filename = TextIO.getln();
            TextIO.writeFile(filename);
            lastfile = TextIO.getOutputFileName();
            pl(value);
            TextIO.writeStandardOutput();
            pl("");
            pl("********************************************************");
            p("Would you like to write to another file?   ");
            writing = TextIO.getlnBoolean();
        } while(writing); // end do/while(writing) //

    } // end WriteFile() //



    public static void pl(String x) {
        TextIO.putln(x);
    } // end pl() //

    public static void p(String y) {
        TextIO.put(y);
    } // end p() //

    public static void pf(String z) {
        TextIO.putf(z);
    } // end pf() //

} // end FileWrite //
\$\endgroup\$
  • 1
    \$\begingroup\$ Filenames for your Java source code should have a .java extension. \$\endgroup\$ – 200_success Nov 22 '15 at 7:48
  • \$\begingroup\$ Do you mean in the git repository? \$\endgroup\$ – Phil C. Nov 22 '15 at 18:45
  • 1
    \$\begingroup\$ Yes, the files in your Git repository. \$\endgroup\$ – 200_success Nov 22 '15 at 18:50
4
\$\begingroup\$

Don't repeat yourself

You do this kind of thing a lot:

pl("********************************************************");
pl("Welcome, and thank you for using Text File");
pl("Manager, Version 0.0 . ");
pl("");

It would be better to create a helper function for that:

private static final printSection(String ... lines) {
    pl("********************************************************");
    for (String line : lines) {
        pl(line);
    }
    pl("");
}

Using this helper, you could rewrite the example at the top simpler, safer, with less duplication as:

printSection(
    "Welcome, and thank you for using Text File",
    "Manager, Version 0.0 . "
);

Sometimes you print the many ****** in other contexts too. So to make that piece more reusable, you should put that in another helper, for example:

private static final printHorizontalRule() {
    pl("********************************************************");
}

Constants

These look like intended as constants:

final static int read = 1;
final static int write = 2;

The convention for naming constants is SHOUT_CASE. And the ordering of the final static modifiers should be static final as per the Java Language Specification. Lastly, these constants are internal details of your implementation, no need to be visible outside, so they should be private:

private static final int READ = 1;
private static final int WRITE = 2;

Code organization

Filer has a lot of logic in main. That's not ideal. The job of "main" is to do only essential setup required for the program to start doing something. The real logic of the program should be in other methods, with descriptive names, outlining their purpose / responsibility.

Naming

The convention for method names is camelCase, so, for example, these names should change in FileRead:

  • Reader -> reader
  • ReadUser -> readUser
  • ReadLast -> readLast

Even more importantly, class names are typically nouns and method names are typically verbs, expressing actions taken on objects. It's not intuitive what a FileRead object represents, and what is the meaning of FileRead.reader.

Formatting

The indenting here is rather chaotic:

    while(choice != read && choice != write){
               pl("********************************************************");
        p("Please choose a valid option [1 or 2]:    ");
        choice = TextIO.getlnInt();
    } // end while(choice) //

And you should place a space before ( and after ):

    while (choice != read && choice != write) {
        pl("********************************************************");
        p("Please choose a valid option [1 or 2]:    ");
        choice = TextIO.getlnInt();
    } // end while(choice) //

Avoid System.exit

There's no need for this statement at the end of the main method:

    System.exit(0);

The program will automatically exit after leaving the main method.

In general, System.exit is often misused by beginners, try to avoid it when possible. Let the program exit naturally.

\$\endgroup\$
  • \$\begingroup\$ Wonderful. You hit so many points. Thank you for the feedback. \$\endgroup\$ – Phil C. Nov 22 '15 at 16:38
  • \$\begingroup\$ I only have one request: If you could tell what the technique is called that you used in the param and loop condition in the first refactored code? (DRY: printSection(String...lines) and for (String line : lines)). I would like to read more about it. \$\endgroup\$ – Phil C. Nov 22 '15 at 18:09
  • 1
    \$\begingroup\$ printSection(String...lines) is called varargs, and for (String line : lines) an enhanced for loop \$\endgroup\$ – janos Nov 22 '15 at 18:28
3
\$\begingroup\$

In addition to @janos's answer...

Comments

The good news here is that you don't commit any 'novice' mistakes of over-commenting every possible line. There's just one specific thing to point out, and it's the use of // end of .... These 'marker comments' are usually not recommended, as they don't describe much, and can get outdated when you change your loop/condition structures or even rename parts of your code.

If you are worried that a condition block is too lengthy and therefore you need one of these to remind you 'where you left off', then try to shorten the block itself, as this is actually more a symptom of lengthly method bodies. In other words, it's recommended to make code easier to read not by adding superfluous comments, but by addressing the root of the problem.

Accepting choices

Currently, you are using int values to determine where to send the user to based on their selection. While this may suffice for introductory Java, Java does have an enumerated type, appropriately referred to as an enum, that lets you specify a predefined set of constants. When you require user input within a given range, an enum may be a good model for that, since it is easier to perform validation. Consider:

pl("Would you like to specify a file name [1],");
p("or choose a file from the tree [2]?     ");

int choice = TextIO.getInt();
if (choice == 1){
    WriteFile();
} else {
    WriteUser();
}
  • By right, you need to reject int values that are not 1 or 2.
  • You have to specify 1 and 2 twice: first in the display, and second in the if statement.

A simple alternative with enum can be as such:

enum UserChoice {
    SPECIFY_NAME("Specify a file name"),
    CHOOSE_FROM_TREE("Choose a file from the tree")

    public static void display() {
        pl("What would you like to do?");
        for (UserChoice uc : values()) {
            pl(uc + " [" + (uc.ordinal() + 1) + "]");
        }
    }
}

// usage
UserChoice.display();
int choice = TextIO.getInt();
while (choice < 1 || choice > UserChoice.values().length) {
    pl("Wrong input, please try again.");
    choice = TextIO.getInt();
}
UserChoice userChoice = UserChoice.values()[choice - 1];
if (userChoice == UserChoice.SPECIFY_NAME) {
    writeFile();
} else if (userChoice == UserChoice.CHOOSE_TREE) {
    writeUser();
} else {
    // this wouldn't happen due to the validation done above
    // just to illustrate how the multiple enum values can be used
}
  • The display() method loops through the set of valid values via values() and uses ordinal() to derive the index for each of them. This means you are free to re-order them, delete or add news ones without much hassle.
  • Validation is done by checking choice < 1 or choice > UserChoice.values().length. If the input matches either condition, let the user know it's a wrong input, and they should re-enter another value.
  • Retrieve the desired user choice by using an index lookup on the values() array.
  • Now, you can compare what is the user selection based on the enum value.

It does look more complex for your simple use case, but when you start dealing with a larger range of valid values, or require more complex validation, the enum-based way tends to be better.

Program flow for FileWrite

Either selection prompts the user to enter the text first, with the only difference being how to tell TextIO to select a file to print to. In that case, you can make use of methods to first get the text, then pass the output of that method to however you want to specify the file as the output. Roughly in code:

public static String getContents() {
    pl("********************************************************");
    pl("Begin writing content to fill file.");
    // ...
    StringBuilder builder = new StringBuilder();
    String input = TextIO.getln();
    do {
        if (input.equals(" ") || input.equals("\\n")) {
            // how does this 'return a line with a single SPACE'?
            builder.append("\\n\\n");
        } else {
            builder.append(input).append(' ');
        }
        input = TextIO.getln();
    } while(!(input.equals("")));
    return builder.toString();
}

// Usage
String contents = getContents();
if (userChoice == UserChoice.SPECIFY_FILE) {
    p("Please specify a file name:      ");
    // inline the output TextIO.getln()
    TextIO.writeFile(TextIO.getln());
} else if (userChoice == UserChoice.CHOOSE_FROM_TREE) {
    TextIO.writeUserSelectedFile();
}
lastfile = TextIO.getOutputFileName();
pl(contents);
  • getContents() simply interprets the input from TextIO.getln() and returns them as a String.
  • Use either TextIO.writeFile(String) or TextIO.writeUserSelectedFile() to set the internal state of TextIO to the desired output file.
  • Then print the contents to the file.

Also note, you never have to do String a = new String() in Java, as new String() is just "". Then, if you ever need to initialize an empty String, only to append contents to it, you can make use of the StringBuilder class.

Reliance on third-party classes

I understand that TextIO is just a provided class from the introductory course you mentioned. Short of doing a code review on that class, do understand that it is only one of the many possible implementations of how to interact with various input/output (I/O) streams in Java. Hence, it's useful to also understand what are some of the assumptions in its workings to make certain operations easier for beginners to start.

Once you step outside the confines of TextIO, you will start to see File or Path objects to represent files, things like BufferedReader or BufferedWriter to buffer reading and writing of I/O streams, and so on. A more common approach to doing file I/O in Java is therefore more complex than just controlling the inner state of a single class.

As you start to warm yourself up to more classes of the JDK, you should consider revisiting your reliance on TextIO to see how you can write more standard code (and we'll gladly welcome future implementations for code review).

\$\endgroup\$
  • 1
    \$\begingroup\$ Wow, as if the first answer wasn't enough. You guys really hit it home on this one. I love the feedback; amazing community full of knowledgeable people. \$\endgroup\$ – Phil C. Nov 22 '15 at 16:44

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.