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&lt;int&gt; 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&lt;int&gt; listOfPorts = new List&lt;int&gt;();                // creates a new list of integers to save the valid given ports

        for (int i = 0; i &lt; portList.Length; i++ )              // iterates through port list including single ports and ranges
        {
            if (portList[i].Contains('-'))                      // if finds a range like 80-120...
            {
                try
                {
                    int[] range = Array.ConvertAll(portList[i].Split('-'), int.Parse);  // saves the edges of a range into an array ex: 80-120 as 80 and 120

                    for (int j = range[0]; j &lt;= range[range.Length - 1]; j++)           // iterates through the range saved above ex: from 80 to 120 the get 
                    {                               
                        listOfPorts.Add(j);                                             // every single port ex: in this case it gets 40 ports, from 80 to 120
                    }                                                                   // if finds an error like a negative port or a wrong range like 120-100
                }                                                                       // instead of 100-120 will discard it
                catch { }
            }
            else
                listOfPorts.Add(Convert.ToInt32(portList[i]));  // adds a single port to the list
        }

        List&lt;int&gt; intPortsList = listOfPorts.Distinct().ToList();   // eliminate duplicated ports

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

        foreach (int item in intPortsList)
        {
            if (item &lt;= 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&lt;int&gt; listOfPorts = new List&lt;int&gt;(); 


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 &lt;= 65536)                                      
 listOfPorts.Add(item);


and

if (item &lt;= 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&lt;int&gt;. Is this a conscious design choice? You might want to read up on the reasoning why I prefer IEnumerable&lt;int&gt;.

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 &lt;= max


to be easier to read than

j &lt;= range[range.length - 1]


This is a minor gripe, but it can be confusing given how "&lt;= length - 1" is usually interpreted in the condition section of a for loop (which usually gets refactored to "&lt; length").

Duplicates

When you don't want duplicates, a List&lt;T&gt; is not your preferred datastructure. Instead, use a Set&lt;T&gt; like HashSet&lt;T&gt;. This will automatically take care of it for you.

If you combine it with your sorting at the end you can use a SortedSet&lt;T&gt; instead:


  A SortedSet&lt;T&gt; 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&lt;T&gt; you will already do the sorting and duplicate removal so that's not needed either.

Best part: SortedSet&lt;T&gt; implements IEnumberable&lt;T&gt; so you can simply return your set as-is.