Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote -1 down vote favorite

I have a for loop that loops infinity when i don't breakout of it and only loops once when i do break out of it. I'm sure im missing something simple.

The if and else should be identical aside from the "String addr = " line. Right now both only loop once. Removing the "break;" sends them into an infinite loop.

public Discover(String enterIP) throws IOException {

    IP = enterIP;
    if (IP.length() < 4) {
        for (int i = 0; i < oct.length; i++) {

            String addr = IP + i + "." + i + "." + i;

            Runtime aRT = Runtime.getRuntime();
            Process aProc = aRT.exec("/usr/local/bin/nmap -sn -n " + addr);

            InputStream is = aProc.getInputStream();
            Scanner sc = new Scanner(is);
            while (sc.hasNextLine()) {
                System.out.println(sc.nextLine());
                // if (sc.findInLine("Host is up") != null) {
                // System.out.println(addr + "is up");
                // }

            }
            sc.close();
            break;

        }
    } else
        for (int i = 0; i < oct.length; i++) {
            String addr = IP + 1 + "." + i;

            Runtime aRT = Runtime.getRuntime();
            Process aProc = aRT.exec("/usr/local/bin/nmap -sn -n " + addr);

            InputStream is = aProc.getInputStream();
            Scanner sc = new Scanner(is);
            while (sc.hasNextLine()) {
                System.out.println(sc.nextLine());
                // while(sc.findWithinHorizon("Host is up", 9999999) !=
                // null){ // need to loop, stops after first find
                // System.out.println(addr + " is up");
            }   sc.close();
                                    break;

            }


        }

}
share|improve this question
1  
The two loops can be a single loop; the loops are virtually identical. – Dave Jarvis Jun 3 at 17:05
@fge Discover is a constructor, does that rule still apply? – Cody H Jun 3 at 19:21
@CodyH Argh, forget it, I misread and thought it was a method! Removing my comment... – fge Jun 3 at 19:26
Have you considered using a static factory method instead? – fge Jun 3 at 19:37

closed as off topic by Jeff Vanzella, Joe F, p.s.w.g, Yuushi, svick Jun 4 at 12:58

Questions on Code Review Stack Exchange are expected to relate to code review request within the scope defined by the community. Consider editing the question or leaving comments for improvement if you believe the question can be reworded to fit within the scope. Read more about reopening questions here.If this question can be reworded to fit the rules in the help center, please edit the question.

up vote 1 down vote accepted

The principle of "Don't Repeat Yourself" (DRY) is rather deep and there is one subtle nuance that most people miss when implementing classes. Consider:

public class PaleBlueDot {
  private int[] oct;

  public Repeat() {
    this.oct = new int[4];
  }

  public void discover( String ip ) {
    for( int i = 0; i < this.oct.length; i++ ) {
    }
  }

  public void somethingElse() {
    this.oct[ 1 ] = 24;
    this.oct[ 2 ] = 64;
  }
}

In this simple example above, the DRY principle has been violated. The violation is caused by accessing the internal variable oct in multiple locations. This opens up the code to race-conditions and a wide variety of other errors, not to mention maintenance, testing, and debugging issues.

This is easily resolved. Whenever a class must manipulate one of its internal variables, always use an accessor; limit the scope of how and when the variable can be changed. Consider again that pale blue dot:

public class PaleBlueDot {
  private int[] myOct;

  public Repeat() {
    setOct( new int[4] );
  }

  public void discover( String ip ) {
    int[] oct = getOct();

    for( int i = 0; i < oct.length; i++ ) {
    }
  }

  public void somethingElse() {
    int[] oct = new int[4];
    oct[ 1 ] = 24;
    oct[ 2 ] = 64;
    setOct( oct );
  }

  private int[] getOct() { return this.myOct; }
  public void setOct( int[] oct ) { this.myOct = oct; }
}

This should also magically fix your bug because the locally scoped oct variable's length will not change, even if the class-scoped oct changes. If this doesn't fix the bug, it will make debugging the problem easier, as you only have two touch-points for when the variable is used or modified.

You'll note that even in the second example, the DRY principle has been violated:

    setOct( new int[4] );
    int[] oct = new int[4];

This is also easily resolved:

    setOct( createOct() );
    int[] oct = createOct();

    protected int[] createOct() { return new int[4]; }

This now allows subclasses to control how the variable is instantiated, greatly increasing the class's extensibility (and re-usability).

share|improve this answer
up vote 2 down vote

Instead of copying all your code across both sides of the if statement, why don't you simplify down to this?

for (int i = 0; i < oct.length; i++) {

        String addr; 
        if(IP.length() < 4) {
            addr = IP + i + "." + i + "." + i;
        }
        else {
            addr =  IP + 1 + "." + i;
        }

        Runtime aRT = Runtime.getRuntime();
        Process aProc = aRT.exec("/usr/local/bin/nmap -sn -n " + addr);

        InputStream is = aProc.getInputStream();
        Scanner sc = new Scanner(is);
        while (sc.hasNextLine()) {
            System.out.println(sc.nextLine());
        }
        sc.close();
        break;
    }

That makes it much more readable. As far as your infinite loop goes, what are the values of oct as its looping? If they're being changed that would cause an infinite loop.

share|improve this answer
i was going to condense it later, promise!! oct is an array int oct[] = new int[256]; – Cody H Jun 3 at 17:13
I'm not seeing anything that would case infinite behavior then unless the nmap process is producing an unending stream, in which case the scanner I suppose could always have a next line. – ben336 Jun 3 at 17:18
this worked just a few days ago but between then and now I've managed to mess it up. so i don't believe it to be nmap. – Cody H Jun 3 at 17:49
up vote 0 down vote

Think i got it. I ended up just doing away with the array, after looking it over it wasnt really needed. In the for loop I replaced oct.length with 256 and everything works fine. Working code(with extra junk for troubleshooting), thanks @ben336 for condensing it.

import java.io.IOException;
import java.io.InputStream;
import java.util.Scanner;

public class Discover extends Start {

String IP = "";
//  private int oct[] = new int[256];

public Discover() {
    // default constructor
}


public Discover(String enterIP) throws IOException {

    IP = enterIP;


//      System.out.println(oct.length);
    for (int i = 0; i < 256; i++) {

        String addr;

        if (IP.length() < 4) {
            addr = IP + i + "." + i + "." + i;

        } 
        else {
            addr = IP + i + "." + i;
        }

        Runtime aRT = Runtime.getRuntime();
        Process aProc = aRT.exec("/usr/local/bin/nmap -sn -n " + addr);

        InputStream is = aProc.getInputStream();
        Scanner sc = new Scanner(is);
        while (sc.hasNextLine()) {
            System.out.println(sc.nextLine());
//               if (sc.findInLine("Host is up") != null) {
//               System.out.println(addr + "is up");
//               }

        }
        sc.close();
        System.out.println(i);
//          break;
    }
}

}
share|improve this answer
This code still has potential (race-condition) bugs. See my answer. – Dave Jarvis Jun 4 at 4:44

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