Refactoring a program for multithreading

[poopuh]

I've been asked to edit the code in this post http://www.daniweb.com/software-development/java/threads/455560/java-recursion-brute-force-sim so that it "Runs the function as a thread which wouldn’t then hang the program whilst it’s waiting for the result". I've looked it up, but I'm struggling to apply it to this program, any tips?

[JamesCherrill]

You have two parts to this task.
The second part is to create and start a new Thread to execute your code, but that requires a run() method (see the Thread API doc for examples etc).
So, the first part is to extract the relevant code from its current place and restructure (refactor) it so it can be called from the Thread's run() method. In its simplest form this could just be to cut that code and paste it into a run method. You'll have to messs about with your existing variables a bit to make them accessible in the run method. With a bit more work you can make it a lot tidier by refactoring the existing code into one or more methods with the appropriate values being passed in as parameters, and have a simple run() that just calls those methods. Best of all, you may even want to have a new class (extending Thread or Runnnable) to perform the function, so you create a new instance passing the values to the constructor, then just execute that in a new Thread. (This will all make more sense when you have read the API doc for Thread).

[poopuh]

I've been playing around for a couple of hours and don't seem to be having any luck. The class I linked above is instantiated by the GUI and then the methods are called there. I altered it so that the GUI calls a BruteThread class which looks like this.

package brute.force.password;

/**
 *
 * @author Jack
 */
public class BruteThread extends Thread {

    public String Password;
    public BruteForcePassword a;
    public String Output;
    public String Check ;


    public BruteThread(String Pass)
    {
        Password = Pass;
        new Thread(new BruteThread(Password)).start();
    }
    @Override
    public void run()
    {
        BruteForcePassword a = new BruteForcePassword(Password);
        Output = a.check();
        Check = a.getCounter();
    }
    public String returnResult()
    {
        return Output;
    }
    public String returnCheck()
    {
        return Check;
    }

}

[JamesCherrill]

You have a constructor that calls new BruteForcePassword, which invokes the constructor... <infinite loop> ...
And the run method ignores the instance(s) you already created amd creates yet another of its own...
One simple arrangement is to call it something like

BruteForcePassword bfw = new BruteForcePassword(password);
new Thread(bfw).start();

so in the constructor you just need to take a copy of password, and in run() you can just start calling your methods - ie in BruteForcePassword there are no calls to new BruteForcePassword

[poopuh]

ok, so this is what I have now:

The GUI start button:

private void jButton1ActionPerformed(java.awt.event.ActionEvent evt) {                                         

     boolean checker = false;
        jTextField2.setText("");
      jTextField4.setText("");
        BruteForcePassword Check = new BruteForcePassword(jTextField1.getText());

        while(checker == false)
        {
            String checkChange="";

            if(Check.getCounter() != checkChange )
            {
                checker = false;
               checkChange = Check.getCounter();
            }
            else
            {
                checker= true;
            }

        jTextField4.setText(Check.getCounter() + " out of 72379772059249583476264088436736");
        }
        jTextField2.setText(Check.check());  
    }                                        

The BruteForcePassword Class

import java.util.Arrays;

public class BruteForcePassword  {

    public String Password;
    public BruteThread a;
    public String getResult;
    public String getCounter;

    public BruteForcePassword(String Pass) {
       Password = Pass;
       BruteThread a = new BruteThread(Pass);
       new Thread(a).start();

    }
   public String getBoolean(){
       while(getResult==null)
       {
           getResult= a.returnResult();
       }
       return getResult;
   }
    public String getCounter()
    {
       return a.currentCount(); 
    }

    public String check() {
        if (a.returnCheck() == true) {
            return "Password Hacked!";
        } else {
            return "Could not hack password!";
        }

    }



 }

and the BruteThread Class

public class BruteThread implements Runnable{

    public String Password;
    public String Output;
    public String Checker ;
     public int counter;
   public boolean check;
    public int passLength;
    public char[] checkPass;
    public char[] inPass;
    public char[] ascii;
    public String Builder;

    public BruteThread(String Pass)
    {
        //Builds the ascii array
        char x;       
        for(x=' '; x<='~';  x++ ) 
        {  
            Builder += x;
        }
        ascii = Builder.toCharArray();

        Password = Pass;
        inPass = Password.toCharArray();
        passLength = inPass.length;
        checkPass = new char[passLength];  
        }


    public String returnResult()
    {
        return Output;
    }

    public String returnChecker()
    {

        return Checker;
    }
    public String currentCount()
    {
        return String.valueOf(counter);
    }
    public Boolean returnCheck()
    {
        return check;
    }

    @Override
       public void run()
    {       
         {

        int i;
        int j;
        int k;
        int l;
        int m;
        int n;
        int o;
        int p;
        int q;
        int r;
        int s;
        int t;
        int u;
        int v;
        int w;
        int x;

        /*
         * 1 Char
         */

        for (i = 0; i <= 98; i++) {

            checkPass[0] = ascii[i];
            counter++;
            if (Arrays.equals(inPass, checkPass)) {
                check = true;
                return;

            }


        } etc, as seen in the first link. 

The compiler is currently throwing

Exception in thread "AWT-EventQueue-0" java.lang.NullPointerException
    at brute.force.password.BruteForcePassword.getCounter(BruteForcePassword.java:36)

and I am completely stumped.

[stultuske]

that's not all the compiler is currently throwing. in order for us to figure out where that call is going wrong, you may want to show the next lines of the stacktrace as well...

as you probably have noticed, the line numbering you have here is not the same as the one being seen by the compiler, so it's a bit risky for us to look at just that.

a small tip about one of the parts of your code:

if (a.returnCheck() == true) {
            return "Password Hacked!";
        } else {
            return "Could not hack password!";
        }

this code can be written more efficiënt (and safe)

if ( a.returnCheck())
  return "Password Hacked!";
return "Could not hack password!";

is all you need there.

[JamesCherrill]

You have an instance variable "a", but in the constructor you create a second, local, variable "a" and initialise that. As soon as the constructor finishes that "a" disappears, and the getCounter method uses the instance varialbe "a" that is still not initialised.

[poopuh]

I've gotten rid of the brute force password class and am just calling the thread straight from the GUI. But it still seems to be hanging the program

 private void jButton1ActionPerformed(java.awt.event.ActionEvent evt) {                                         

        jTextField2.setText("");
      jTextField4.setText("");
         a = new BruteThread(jTextField1.getText());
       a.start();

       while(true)
       {
           jTextField4.setText(String.valueOf(a.currentCount()));
           Count = a.currentCount();
           if(a.returnCheck())
           {
               break;
           }
       }
       jTextField4.setText(String.valueOf(a.currentCount()));

            jTextField2.setText( "Password Hacked!");





    }    


public class BruteThread extends Thread{

    public String Password;

    public String Checker ;
     public int counter;
   public boolean check;
    public int passLength;
    public char[] checkPass;
    public char[] inPass;
    public char[] ascii;
    public String Builder;

    public BruteThread(String Pass)
    {
        //Builds the ascii array
        char x;       
        for(x=' '; x<='~';  x++ ) 
        {  
            Builder += x;
        }
        ascii = Builder.toCharArray();

        Password = Pass;
        inPass = Password.toCharArray();
        passLength = inPass.length;
        checkPass = new char[passLength];  
        }



    public int currentCount()
    {
        return counter;
    }
    public Boolean returnCheck()
    {
        return check;
    }

    @Override
       public void run()
    {       
         {

        int i;
        int j;
        int k;
        int l;
        int m;
        int n;
        int o;
        int p;
        int q;
        int r;
        int s;
        int t;
        int u;
        int v;
        int w;
        int x;

        /*
         * 1 Char
         */

        for (i = 0; i <= 98; i++) {

            checkPass[0] = ascii[i];
            counter++;
            if (Arrays.equals(inPass, checkPass)) {
                check = true;
                return;

            }

[JamesCherrill]

Your while true loop (line 8) will do its very best to take 100% of the CPU time, at best slowing things down massively. Swing will also wait for your ActionPerformed to return before doing anything else, so you are completely freezing the whole GUI.
You can do a quich'n'dirty hack by giving the BruteThread a reference to the text field and display the result from there.
To do it "properly" use a SwingWorker which was designed to run CPU-intensive tasks in the background while keeping the GUI up to date with progess, interim results and final results.
http://docs.oracle.com/javase/7/docs/api/javax/swing/SwingWorker.html

[poopuh]

I'm not sure what you mean! Is there any way I can set up some sort of property change listener in a new main class and instantiate the GUI and BruteThread from there? and then use the call a set text method from the main and transfer values across?

[JamesCherrill]

Absolutely yes. You can do that just like you describe, or cut out the main class and have the GUI pass a ref to itself ("this") to the BruteThread so the BruteThread can call it back when the job is finished.
This link is a good intro to doing a change listener in Java...
http://www.javaprogrammingforums.com/java-programming-tutorials/7321-how-use-observer-pattern.html

In your case it's simpler because you don't need a generic solution. You can hard-code passing the listener as part of constructing the Thread, and hard-code the GUI's class name so you won't need to define a listener interface either.