Method to separate a list of ports to scan

I'm building a port scanner as an exercise. It contains a method to separate the ports that the user wants to have checked, which can include single ports and port ranges, ex: 22, 23, 80-120, 443-500, 1024, etc. It discards wrong ports such as negative ports, wrong range such as 120-80 instead of 80-120 which is the right form, and duplicate ports.

I'm wondering if there's a better way to do this, or to improve the code, make it shorter, using a more effective technique. The text box to enter the list of ports only accepts numbers, commas and dashes.

private List extractPorts(string ports)                // the string argument contains a list of ports entered through a text box
    {

        string[] portList = ports.Split(',');                   // save everything between commas, including ranges like 80-120

        List listOfPorts = new List();                // creates a new list of integers to save the valid given ports

        for (int i = 0; i  intPortsList = listOfPorts.Distinct().ToList();   // eliminate duplicated ports

        listOfPorts.Clear();                                        // clear the previous list

        foreach (int item in intPortsList)
        {
            if (item <= 65536)                                      // discard ports out of range
              listOfPorts.Add(item);
        }

        listOfPorts.Sort();                                         // to sort the final list

        return listOfPorts;           
    }

Comments

You have too many of them. Stick to comments that clarify the intent behind code instead of documenting the syntax.

When I see

List listOfPorts = new List();

I know already that it's a list of integers, intended to represent the ports. Comments should explain why you do what you do, for example:

// List of ints instead of strings so we can perform arithmetic logic

Whitespace

This is more personal but your code feels very spread out. You don't have to leave a space between every line of code (although I'll admit that the braces are a major part of it).

Braces

If you omit braces, you will guaranteed end up with a logical error one day. What if you decide to add something else to this code snippet but aren't paying 100% attention?

else
    listOfPorts.Add(Convert.ToInt32(portList[i]));

This will be a pain to debug.

For situations like this it is more forgiving, but honestly is there such a difference between

if (item <= 65536)                                      
 listOfPorts.Add(item);

and

if (item <= 65536) {
     listOfPorts.Add(item);
}

Exceptions

You have an empty catch block. That's no bueno, there should be some sort of feedback to the user that tells him what went wrong (at the very least there should be logging in place).

Abstraction

Your return statement is List. Is this a conscious design choice? You might want to read up on the reasoning why I prefer IEnumerable.

Naming

Two variables are respectively portList and listOfPorts. First of all: remove the underlying datastructure from the name.

In a way this can also be seen as enforcing encapsulation: it allows you to change the implementation to, for example, a Dictionary without suddenly having a discrepancy between the implementation and the variable name.

More descriptive names could be input/inputPorts/unparsedPorts and ports/parsedPorts/resultPorts, etc.

Input boundaries

I don't see any boundary checks to verify the following things:

• No negative numbers

• No positive numbers outside the range of ports

• A range should only consist of 2 values

Loop clarity

I consider

int max = range[range.length - 1];
// ...
j <= max

to be easier to read than

j <= range[range.length - 1]

This is a minor gripe, but it can be confusing given how " is not your preferred datastructure. Instead, use a Set like HashSet. This will automatically take care of it for you.

If you combine it with your sorting at the end you can use a SortedSet instead:


A SortedSet object maintains a sorted order without affecting performance as elements are inserted and deleted. Duplicate elements are not allowed.

Complexity

You're doing a lot of things with a lot of lists of ports (better naming would have come in handy here!).

This is what you do:

• Create list (A) of input ports

• Parse A into another list (B)

• Create new list that holds unique values from B

• Clear B

• Add all ports that adhere to a boundary check to list B

• Sort B

That's a lot of stuff that we can refactor.

First of all: put that boundary check earlier in your code. Once a port is added to our result list of ports, it is a valid port. We don't want to copy everything to a new list just to add that boundary check (and you need more than that!).

In fact, now it's already a lot easier: by using the SortedSet you will already do the sorting and duplicate removal so that's not needed either.

Best part: SortedSet implements IEnumberable` so you can simply return your set as-is.

StackExchange Code Review Q#47673, answer score: 5