4
\$\begingroup\$

We just finished working on a Database project in Java Netbeans. We added the feature Import which will import the database record of one user to a text file.

String IMPORT_QUERY = 
    "SELECT pe.entry_id ID, pe.entry_account ACCOUNT, pe.entry_username USERNAME, pe.entry_password PASSWORD, pe.entry_category CATEGORY FROM password_entries pe JOIN accounts a ON a.user_id = pe.user_id WHERE username = ? AND password = ?";
    try {
        PrintWriter writer = new PrintWriter("D:\\Users\\mandu\\Documents\\NetBeansProjects\\PasswordVault\\src\\PasswordList.txt", "UTF-8");
        String location = "C:\\\\Users\\\\mandu\\\\Documents\\\\NetBeansProjects\\\\PasswordVault\\\\src";
        pstmt = conn.prepareStatement(IMPORT_QUERY);
        pstmt.setString(1, LogInFrame.username.getText());
        pstmt.setString(2, LogInFrame.password.getText());
        res = pstmt.executeQuery();
        writer.println("ACCOUNT" + "\t\t|\t" + "USERNAME" + "\t\t\t|\t" + "PASSWORD" + "\t\t\t|\t" + "CATEGORY" + "\t\t|\t\n");
        writer.println("--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------");
            while(res.next()){
                entryAccount = res.getString("ACCOUNT");
                entryUsername = res.getString("USERNAME");
                entryPassword= res.getString("PASSWORD");
                entryCategory = res.getString("CATEGORY");
                writer.println(entryAccount + "\t\t\t|\t" + entryUsername + "\t\t\t|\t" + entryPassword + "\t\t\t|\t" + entryCategory + "\t\t\t|\t\n");
            }
                writer.println();
                writer.println();
                writer.println("++++++++++++++++++++++WARNING!++++++++++++++++++++++");
                writer.println();
                writer.println("FILE ADDRESS: " + location);
                writer.println();
                writer.println("Please COPY the FILE ADDRESS above and DELETE the 'PasswordList.txt' file AFTER you COPY you RECORDS.");
                writer.println();
                writer.println("TO AVOID SOMEONE FROM COPYING YOUR RECORDS.");
                writer.close();
            JOptionPane.showMessageDialog(null,"\nData Imported!","Prompt:", JOptionPane.INFORMATION_MESSAGE);
            OpenFile();
    } catch (FileNotFoundException ex) {
        JOptionPane.showMessageDialog(null,"\nFile not found. Please check the file location.","Error x203", JOptionPane.ERROR_MESSAGE);
        ex.printStackTrace();
    } catch (UnsupportedEncodingException ex) {
        JOptionPane.showMessageDialog(null,"\nUnsupported Encoding. Please check the Target file.","Error x204:", JOptionPane.ERROR_MESSAGE);   
        ex.printStackTrace();
    } catch(SQLException | HeadlessException ex){
        JOptionPane.showMessageDialog(null,"\nError occured while communicating with the server.","Error x205", JOptionPane.ERROR_MESSAGE);
        ex.printStackTrace();
    } catch (IOException ex) {
        JOptionPane.showMessageDialog(null,"\nError occured in getting input.\nPlease try again.\n","Error x206:", JOptionPane.ERROR_MESSAGE);         
        ex.printStackTrace();
    }

}

Any ideas/opinions about the code, or is there any way to get the columns aside from getString()? Did I violate some rules with regards to error checking?

\$\endgroup\$
1
  • 3
    \$\begingroup\$ Would it not be clearer to call this feature Export? (The user record is being written out to a text file.) \$\endgroup\$
    – hardmath
    Commented Mar 10, 2015 at 14:23

2 Answers 2

7
\$\begingroup\$

Any ideas/opinions about the code I written?

A couple of points on readability:

  • your code does too much. It reads from the database, it prints to a file, it prints to a GUI, and it formats strings. I would create separate methods for each of these activities (or at least for the formatting and the printing to GUI).
  • 240 characters are definitely too long for one line. I would try for 80 chars, 120 max.
  • your directory location string is duplicated. Define it in a static field and then use new PrintWriter(location + "\\PasswordList.txt", "UTF-8");
  • why are there so many backslashes in your path? I'm not that familiar with Java + Windows, but this seems unnecessary.
  • just using a bunch of tabs for alignment is quite messy (the code is messy, and the result will be messy as well). Use either CSV (the code will be cleaner, the output really messy, but at least it will be on purpose), or use some code for proper alignment.
  • you have way too many println. Just use \n.
  • your indentation is off, which makes it harder to read.
  • try to reduce the length of your try blocks. It's really hard to see what exception might be thrown by what operation if you just catch them all at the very end. You could also use try-with-resources.
  • Please COPY the FILE ADDRESS above and DELETE the 'PasswordList.txt' file: people are not going to do that. If this is important for security (and it kind of seems like it would be), you should find a different way to handle this.
  • a lot of your variables seem to be defined in a large scope, even though it isn't necessary. Eg entryX (which aren't needed at all, just use the value directly), pstmt, res, etc.
\$\endgroup\$
1
  • 1
    \$\begingroup\$ I used to code at around 120 to 160 characters per line (widescreen FTW), reading CR.SE is slowly making me shift to no more than 90 per line... \$\endgroup\$
    – h.j.k.
    Commented Mar 10, 2015 at 15:26
5
\$\begingroup\$

This is on top of @tim's excellent review. (Please follow everything he said.)

Paths in Java

Here's a tip about typing paths: just use / as the path separator, Java will figure out the right thing to use for the underlying operating system, for example:

File basedir = new File("C:/Users/mandu/Documents/NetBeansProjects/PasswordVault/src");

Then you can rewrite your writer and location in terms of that:

PrintWriter writer = new PrintWriter(new File(basedir, "PasswordList.txt"), "UTF-8");
String location = basedir.getPath();

By the way you used two very similar paths, one starting with C:/ and another starting with D:/. Looks like a bug. This tends to happen when duplicating stuff in code rather than reusing common logic.

Order of operations

I suggest to think through the order in which you perform operations. In particular, it's pointless and potentially dangerous to open an output file for writing before you have anything to write. You should create the writer after you could create the database connection and execute queries and ready to write results.

Your questions

Using getString to get column values is fine like that, when you work with a ResultSet object. There are other ways, for example using frameworks that can map rows to Java objects, such as Hibernate and Spring and probably many others.

As for error handling, in a serious application there is no place for printing stack traces on the console. The recommended practice is to write to a log file instead. But the bigger problem is having a large try block with many catch statements. As @tim already suggested, you should split that up to smaller functions, and then you will have smaller try-catch blocks naturally.

\$\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.