Would appreciate feedback on college-level project

Question

In this program, I have been asked to read a an existing textfile (1324passlist) containing a list of passwords, then create a dictionary file with both the password and MD5 hash of the password on the same line. Have also allowed user to input a password hash, search the created dictionary file and print the corresponding password, if it exists. I'm not a programmer and it's not pretty, but it runs according to the assignment guidelines. I would like advice on where I could tidy up and write the code more efficiently.

public class main
{
    public static void searchTextFile(String hash) throws IOException
    {
        File fileName = new File("dictionary.txt");  
        Scanner scan = new Scanner(fileName);

        String line = scan.nextLine();
        StringTokenizer st = new StringTokenizer(line, " ");

        while(st.hasMoreTokens())
        {
            String word1 = st.nextToken();
            String word2 = st.nextToken();

            if(word2.equals(hash))
            {
                System.out.println(word1);
                break;
            }

            if(!st.hasMoreTokens()) 
            {
                System.out.println("Hash not found.");
            }
        }
    }

    public static String getMd5(String pInput) 
    { 
        try 
        {
            MessageDigest lDigest = MessageDigest.getInstance("MD5"); 
            lDigest.update(pInput.getBytes()); 
            BigInteger lHashInt = new BigInteger(1, lDigest.digest()); 
            return String.format("%1$032X", lHashInt); 
        } 
        catch(NoSuchAlgorithmException lException) 
        { 
            throw new RuntimeException(lException); 
        } 
    }

    public static void findPassword(String fileName, String hash)
    {
        try
        {
            Scanner scanner = new Scanner(new File(fileName));
            List<String> pwAndHash = new ArrayList<String>();

            while(scanner.hasNext())
            {
                pwAndHash.add(scanner.next());
            }

            try
            {
                String hash1 = hash.toUpperCase();
                int hashIndex = pwAndHash.indexOf(hash1);
                int passwordIndex = hashIndex - 1;

                System.out.println(pwAndHash.get(passwordIndex));
            }
            catch(ArrayIndexOutOfBoundsException a)
            {
                System.out.println("The hash was not found.");
            }
        }
        catch(FileNotFoundException e)
        {
        }
    }

    public static String hashPassword(String password)
    {
        String encrypted = "";

        try 
        {
            MessageDigest digest = MessageDigest.getInstance("MD5"); 
            byte[] passwordBytes = password.getBytes(); 

            digest.reset();
            digest.update(passwordBytes);
            byte[] message = digest.digest();

            StringBuilder hexString = new StringBuilder();

            for ( int i=0; i < message.length; i++) 
            {
                hexString.append(Integer.toHexString(0xFF & message[ i ]));
            }

            encrypted = hexString.toString();
        }
        catch(Exception e)
        {       
        }

        return encrypted;
    }

    public static void main(String[]args)
    {
        File file = new File("1324passlist.txt");
        Scanner userInput = new Scanner(System.in);

        try 
        {   
            Scanner input = new Scanner(file);

            while (input.hasNextLine()) 
            {
                try
                {
                    PrintWriter out = new PrintWriter(new BufferedWriter(new FileWriter("dictionary.txt", true)));

                    String password = input.nextLine();
                    out.print(password);
                    out.flush();
                    String hashedPassword = getMd5(password);

                    out.print(" " + hashedPassword + " ");
                    out.close();
                }
                catch(IOException d)
                {
                }
            }
        }
        catch (FileNotFoundException e)
        {
        }

        System.out.println("Enter a hash to search for its corresponding password.");
        String hashToSearch = userInput.next();
        findPassword("dictionary.txt", hashToSearch);

        try
        {
            searchTextFile(hashToSearch);
        }
        catch(IOException e)
        {
        }
    }
}

Answer 1

Some comments that don't relate to efficiency:

Exceptions and exception handling. In a couple of places you are SQUASHING exceptions; e.g.

    catch(FileNotFoundException e)
    {
    }

So, if we can't open the password file, we effectively ignore the error, and ... behave exactly as if the password was correct.

    catch( Exception e ) { }

This is even worse. You are now squashing all exceptions, including any NPEs, class cast exceptions, etc caused by bugs in your code or (hypothetically) in library code you are calling.

Lesson #1: - Do not squash exceptions. Any exception that is out of the ordinary should result in at least an error message to report the problem. And if your code is not anticipating the exception, then you should print / log a stack trace.

This is bad:

    try {
        String hash1 = hash.toUpperCase();
        int hashIndex = pwAndHash.indexOf(hash1);
        int passwordIndex = hashIndex - 1;
        System.out.println(pwAndHash.get(passwordIndex));
    } catch(ArrayIndexOutOfBoundsException a) {
        System.out.println("The hash was not found.");
    }

Reasons:

• You are expecting that the ArrayIndexOutOfBoundsException will be thrown in this: pwAndHash.get(passwordIndex). But that statement won't throw that exception. If the index is wrong is wrong it throws IndexOutOfBoundsException.
• Supposing it did throw that exception, you still have the problem that other statements in the block could throw that exception ... for a completely different reason. For example, if you were using your own List implementation class, it could be due to an unexpected bug in that class.

• Throwing and catching an exception in Java is relatively expensive. In this case, there is a simpler, cleaner, more efficient solution:

  String hash1 = hash.toUpperCase();
  int hashIndex = pwAndHash.indexOf(hash1);
  if (hashIndex == -1) {
      System.out.println("The hash was not found.");
  } else {
      int passwordIndex = hashIndex - 1;
      System.out.println(pwAndHash.get(passwordIndex));
  }
  

Lesson #2: Be really careful when catching unchecked exceptions to make sure that you don't hide bugs ... or introduce new ones by catching the wrong exception.

Lesson #3: Don't use exceptions for normal control flow ... unless it really simplifies things. (Some people would say never use exceptions for control flow, but there are limited cases where it is (IMO) justified to use them.)

Stylistic:

1. Indent your code properly, and make sure that you are consistent in your use of TAB characters. (Ideally reconfigure your IDE so that it doesn't use them at all!).

2. Be consistent about line breaks, and embedded whitespace.

3. I'd recommend NOT putting opening braces for code blocks on a new line. It wastes vertical screen space ... and goes against Best Practice as set out in the Sun Java Style Guide (see below).

_{When I am marking student code, they will get severely penalized for poor style. And in a professional situation, poor style results in rejection of code on quality grounds. When you write code, you write it for other people to read. If it is not readable, it is not fit for purpose. Period.}

---

To those who think that placement of braces is a personal preference, the following is quoted from the Sun Java Style Guide:

7.2 Compound Statements

Compound statements are statements that contain lists of statements enclosed in braces "{ statements }". See the following sections for examples.

• The enclosed statements should be indented one more level than the compound statement.
• The opening brace should be at the end of the line that begins the compound statement; the closing brace should begin a line and be indented to the beginning of the compound statement.
• Braces are used around all statements, even single statements, when they are part of a control structure, such as an if-else or for statement. This makes it easier to add statements without accidentally introducing bugs due to forgetting to add braces.

And the Guide has many examples to illustrate what they mean by that.

Answer 2

Many small tips, always talking about the code snippet above the tip. More on form and function, the things others might miss. I know next to nothing about crypto :)

    public class main {

public static void searchTextFile(String hash) throws IOException

Poor name. What exactly are we searching this text file for?

{
    File fileName = new File("dictionary.txt");

The first thing I would do is check if hash is null and throw an Exception. Is there any reason in continuing?

    Scanner scan = new Scanner(fileName);
    String line = scan.nextLine();
    StringTokenizer st = new StringTokenizer(line," ");

    while(st.hasMoreTokens())
    {
        String word1 = st.nextToken();
        String word2 = st.nextToken();

This breaks if there are an odd number of tokens. Probably OK because you write the dictionary file, but it's bad code; which would you rather see: "NoSuchElementException on line xxx" or "Dictionary file is corrupt, doesn't consist of password/hash pairs"?

        if(word2.equals(hash))
        {
            System.out.println(word1);
            break;
        }

Separate action from presentation. This would better written as a method which looks up the password, and returns it, and then another very simple method displayPassword which looks it up and prints it to System.out. This way your internal method is reusable should you want to get the password again, and not just want it printed.

Also, don't use break. There are times when it makes sense, but I would argue not here; the code can easily be structured to not need it; while(st.hasMoreTokens && result == null)

        if(!st.hasMoreTokens())
        {
            System.out.println("Hash not found.");
        }

Don't recheck in the loop whether or not st.hasMoreTokens()... it's harder to read. Instead, have a String result = null; which you're looking for, and after the loop completes if it is still null, then decide you didn't find anything. This way you're checking st's elements in one place, and looping over them.

    }
}

public static String getMd5(String pInput) 
{ 
    try 
    {
    MessageDigest lDigest = MessageDigest.getInstance("MD5"); 
    lDigest.update(pInput.getBytes()); 
    BigInteger lHashInt = new BigInteger(1, lDigest.digest()); 
    return String.format("%1$032X", lHashInt); 

Not commenting as I'm not a security guy, but it looks good to me

    } 
    catch(NoSuchAlgorithmException lException) 
    { 
    throw new RuntimeException(lException); 
    } 

I think this is fine. You remember to include the cause (masking exceptions is bad), and if the JVM this is running in can't figure out how to use the provided algorithm, then the whole application should halt (nothing will work anyway).

}

    public static void findPassword(String fileName, String hash)
{
    try
    {Scanner scanner = new Scanner(new File(fileName));

        List<String> pwAndHash = new ArrayList<String>();
        while(scanner.hasNext())
        {
        pwAndHash.add(scanner.next());
        }

Fine, but here is an important CS tradeoff. Your method is reading in, doing something, and printing out. This means that once you've handled one pwAndHash, you can forget about it. With that knowledge, which is better:

• Reading in every thing from the file, then iterating through and printing everything out
• Reading in one thing at a time, doing your thing and printing, and then forgetting it

The answer; it depends, and here you're dealing with small files, so it doesn't matter. But consider a JVM running your code with 256 MB of RAM. What happens when the file you're reading is bigger than that? Your code (everything in, everything out) breaks, but the streaming version works. Not important on the scale you're working, but important to think about.

                    try
        {
        String hash1 = hash.toUpperCase();
        int hashIndex = pwAndHash.indexOf(hash1);
                    int passwordIndex = hashIndex - 1;

                    System.out.println(pwAndHash.get(passwordIndex));
            }
            catch(ArrayIndexOutOfBoundsException a)
            {
        System.out.println("The hash was not found.");
            }

Exceptions are meant for exceptional circumstances. Using one as part of a loop like this is not very performant. Better might be to find where the delimiter is between pw and hash, assume the hash is the rest of that line, and compare to the hash you're searching for.

    }
    catch(FileNotFoundException e)
    {
    }

Do something with the exception!

}
public static String hashPassword(String password)
{
    String encrypted = "";

        try 
    {
    MessageDigest digest = MessageDigest.getInstance( "MD5" ); 
            byte[] passwordBytes = password.getBytes( ); 

        digest.reset( );

do we need to reset? Didn't we just initialize it?

        digest.update( passwordBytes );

If we never use passwordBytes again, why not just digest.update( password.getBytes() );?

        byte[] message = digest.digest( );

        StringBuilder hexString = new StringBuilder();

        for ( int i=0; i < message.length; i++) 
        {
            hexString.append( Integer.toHexString
                                    (0xFF & message[ i ] ) );
        }
        encrypted = hexString.toString();

Not commenting on this bit.

    }
    catch( Exception e ) { }
    return encrypted;
}


    public static void main(String[]args)
{
    File file = new File("1324passlist.txt");
    Scanner userInput = new Scanner(System.in);
    try 
    {   
        Scanner input = new Scanner(file);

       while (input.hasNextLine()) 
        {
    try
    {
PrintWriter out = new PrintWriter(new BufferedWriter(new FileWriter
            ("dictionary.txt", true)));

    String password = input.nextLine();
    out.print(password);
    out.flush();
    String hashedPassword = getMd5(password);

                out.print(" " + hashedPassword + " ");
                out.close();
            }
            catch(IOException d)
            {
            }
        }
    }
    catch (FileNotFoundException e)
    {
    }

    System.out.println("Enter a hash to search for its corresponding password.");
String hashToSearch = userInput.next();
findPassword("dictionary.txt", hashToSearch);
    try
    {
    searchTextFile(hashToSearch);
    }
    catch(IOException e)
    {
    }
}

Not commenting on main method as they are usually just a "runner" for what is essentially the guts on the class.

Answer 3

Handling runtime exceptions is the wrong way of coding. Also embedding all your code in a try-catch is wrong. What I would do is to handle all the exceptions in the main() method and allow individual methods to throw the checked exceptions. Its obvious that the main() wont be able to continue if any of the sub-methods throws a checked exception so it makes sense. Here is your edited class :

import java.io.BufferedWriter;
import java.io.File;
import java.io.FileNotFoundException;
import java.io.FileWriter;
import java.io.IOException;
import java.io.PrintWriter;
import java.math.BigInteger;
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
import java.util.ArrayList;
import java.util.List;
import java.util.Scanner;
import java.util.StringTokenizer;

public class main {

    public static void searchTextFile(String hash) throws IOException {
        File fileName = new File("dictionary.txt");
        Scanner scan = new Scanner(fileName);

        String line = scan.nextLine();
        StringTokenizer st = new StringTokenizer(line, " ");

        while (st.hasMoreTokens()) {
            String word1 = st.nextToken();
            String word2 = st.nextToken();

            if (word2.equals(hash)) {
                System.out.println(word1);
                break;
            }
            if (!st.hasMoreTokens()) {
                System.out.println("Hash not found.");
            }
        }
    }

    public static String getMd5(String pInput) throws NoSuchAlgorithmException {

        MessageDigest lDigest = MessageDigest.getInstance("MD5");
        lDigest.update(pInput.getBytes());
        BigInteger lHashInt = new BigInteger(1, lDigest.digest());
        return String.format("%1$032X", lHashInt);

    }

    public static void findPassword(String fileName, String hash)
            throws FileNotFoundException {

        Scanner scanner = new Scanner(new File(fileName));
        List<String> pwAndHash = new ArrayList<String>();
        while (scanner.hasNext())
            pwAndHash.add(scanner.next());

        String hash1 = hash.toUpperCase();
        int hashIndex = pwAndHash.indexOf(hash1);
        int passwordIndex = hashIndex - 1;

        System.out.println(pwAndHash.get(passwordIndex));

    }

    public static String hashPassword(String password)
            throws NoSuchAlgorithmException {
        String encrypted = "";

        MessageDigest digest = MessageDigest.getInstance("MD5");
        byte[] passwordBytes = password.getBytes();

        digest.reset();
        digest.update(passwordBytes);
        byte[] message = digest.digest();

        StringBuilder hexString = new StringBuilder();

        for (int i = 0; i < message.length; i++) {
            hexString.append(Integer.toHexString(0xFF & message[i]));
        }
        encrypted = hexString.toString();

        return encrypted;
    }

    public static void main(String[] args) {
        File file = new File("1324passlist.txt");
        Scanner userInput = new Scanner(System.in);
        try {
            Scanner input = new Scanner(file);

            while (input.hasNextLine()) {
                try {
                    PrintWriter out = new PrintWriter(new BufferedWriter(
                            new FileWriter("dictionary.txt", true)));

                    String password = input.nextLine();
                    out.print(password);
                    out.flush();
                    String hashedPassword = getMd5(password);

                    out.print(" " + hashedPassword + " ");
                    out.close();
                    System.out
                            .println("Enter a hash to search for its corresponding password.");
                    String hashToSearch = userInput.next();
                    findPassword("dictionary.txt", hashToSearch);

                    searchTextFile(hashToSearch);
                }  catch (NoSuchAlgorithmException e) {
                    e.printStackTrace();
                }
            }
        } catch (FileNotFoundException e) {
            e.printStackTrace();
        }catch (IOException e) {
            e.printStackTrace();
        }

    }
}