3
\$\begingroup\$

For purpose of practice and experience, I was working on a "Port Availability Scanner" written in Java. So far it's working but it's really slow. Especially on relatively large quantities of ports (1000+).

It's quite faster scanning localhost then my external IP (If it actually scans externally correctly).

As an additional note for if it really matters: I am using Java 9.

Code:

package test;

import java.io.IOException;
import java.net.Socket;
import java.util.Arrays;

public class Test {

    private static final Test instance = new Test("157.97.56.42", 80, 443, 21, 22, 25, 8080);
    private static String ip;
    private int[] ports = new int[1_000_000];

    private Test(String ip, int start, int end) {
        Test.ip = ip;

        ports: {
            int c = 0;

            for (int i = start; i < end; i++) {
                ports[c++] = i;
            }

            ports = Arrays.copyOfRange(ports, 0, c + 1);
        }
    }

    private Test(String ip, int... ports) {
        Test.ip = ip;
        this.ports = ports;
    }

    public static void main(String[] args) {
        long start = System.currentTimeMillis();
        Thread thread = new Thread(instance::check);
        thread.start();
        long end = System.currentTimeMillis();

        System.out.println("Took: " + (end - start) + "ms.");
    }

    public void check() {
        if(ports.length > 1) {
            for (int port : ports) {
                System.out.println("Available " + port + ": " + Test.isAvailable(port));
            }
        } else if(ports.length == 1) {
            System.out.println("Available " + ports[0] + ": " + Test.isAvailable(ports[0]));
        } else {
            throw new IllegalStateException();
        }
    }

    private static boolean isAvailable(int port) {
        Socket socket = null;

        try {
            socket = new Socket(ip, port);
            return false;
        } catch (IOException e) {
            return true;
        } finally {
            if(socket != null) {
                try {
                    socket.close();
                } catch (IOException ignored) { }
            }
        }
    }
}

Please can you review my code, especially performance-wise?

Edit: Would it be possible by using NIO or any other form of MultiThreading the sake of speed?

\$\endgroup\$
  • \$\begingroup\$ I forgot that was one of the rules here, thank you for reverting :) I'll wait another day as you suggest to look at eventually other reviews. \$\endgroup\$ – Azoraqua Oct 2 '17 at 22:35
3
\$\begingroup\$
  1. Why do you create a million element array? (int[] ports = new int[1_000_000]) It'd better to make it as large as the ports in constructor (end-start or ports.length)

  2. Why all of this code?

    if(ports.length > 1) {
        for (int port : ports) {
            System.out.println("Available " + port + ": " + Test.isAvailable(port));
        }
    } else if(ports.length == 1) {
        System.out.println("Available " + ports[0] + ": " + Test.isAvailable(ports[0]));
    } else {
        throw new IllegalStateException();
    }
    

    You only really need

    for (int port : ports) {
        System.out.println("Available " + port + ": " + Test.isAvailable(port));
    }
    

    The result will be equivalent and the code is now more readable

  3. Why did you make ip and instance static? Do you want to make the class a singleton?

  4. The ports label is unnecessary in the first constructor

  5. Use InetAddress instead of String for IPs' as from what I know Sockets convert the strings into InetAdresses anyway. It should somewhat improve performance (change ip variable from String to InetAddress so you don't recreate the address on and on again)

  6. Shouldn't this:

    try {
        socket = new Socket(ip, port);
        return false;
    } catch (IOException e) {
        return true;
    } finally {
        if(socket != null) {
            try {
                socket.close();
            } catch (IOException ignored) { }
        }
    }
    

    work the other way round? Why method isAvailable returns true when the socket throws a exception? Looks like a bug to me I misunderstood what you meant by available. Your code is correct

\$\endgroup\$
  • \$\begingroup\$ I've changed my code to reflect your suggestions, but why I used return false when the Socket connected was because I thought, if it can connect something is using that port (A webserver for example) which would mean it's not available. \$\endgroup\$ – Azoraqua Oct 2 '17 at 20:26
  • \$\begingroup\$ @Azoraqua I get it know. I misunderstood what you meant by available, sorry for that \$\endgroup\$ – Mibac Oct 2 '17 at 20:29
  • \$\begingroup\$ Don't worry, thank you though. see the edit for additional question related to it, maybe you can answer it. \$\endgroup\$ – Azoraqua Oct 2 '17 at 20:31
  • \$\begingroup\$ @Azoraqua I edited my answer a bit as you kind of misunderstood point 5 of my answer. Regarding the slower performance on external IP's, well that's no surprise. It's always slower to connect to some external IP than to connect to localhost straight away. There's not much you can do about it \$\endgroup\$ – Mibac Oct 2 '17 at 20:38
  • \$\begingroup\$ I understand it, but could you explain to me, why connecting to my OVH machine is much faster then connecting to my own external IP (Almost 3 times faster)? And secondly, can you read my 2th edit about the changes related to code efficiency and such. \$\endgroup\$ – Azoraqua Oct 2 '17 at 20:47

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.