Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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

        for (int i = 0; i < 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 <= 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<int> 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;           
    }
share|improve this question
1  
Your code treats, e.g., "123-45-678" as identical to "123-678". Is that intentional? Is there a reason your code doesn't throw exceptions for crummy input, like "1,-,2" or "123-45"? It seems like it would be nice to let users know that their input is bad, rather than doing arbitrary "magic" to interpret the bogus input (IMO). –  Travis Snoozy Apr 19 at 20:27
    
the message of error to the user of course i will get done, for now i'm only trying to make the method work. Your code treats, e.g., "123-45-678" as identical to "123-678". Is that intentional? -> i didn't know that, i will try to improve it. The point that it doesn't get the 1st range because is not right(123-45), is descending and has to be ascending, what i have to do is maybe treat that 45 as a single port –  blade Apr 19 at 20:33

2 Answers 2

up vote 5 down vote accepted

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<int> listOfPorts = new List<int>(); 

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<int>. Is this a conscious design choice? You might want to read up on the reasoning why I prefer IEnumerable<int>.

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 "<= length - 1" is usually interpreted in the condition section of a for loop (which usually gets refactored to "< length").

Duplicates

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

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

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

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

share|improve this answer
    
@Travis: appreciate the edit, that was a rather silly mistake from my part. –  Jeroen Vannevel Apr 19 at 21:57
    
Np. I almost deleted the section, but I figured if someone who'd given otherwise spot-on advice could make that mistake in reading the code, it might still be worth mentioning the potential hiccup in clarity. –  Travis Snoozy Apr 19 at 22:16

First thoughts...

  • Use an interface for your return type (e.g., IEnumerable<int> instead of List<int>)
  • Use a SortedSet<int> internally to get rid of duplicates, and do the sorting too. That should get rid of most of the bottom part of your function.
  • Use foreach instead of for for your collection traversal

So, with that said, here's some slightly modified code. Note that I'm using UInt16 to prevent too-big items from being added in the first place (which means we don't have to remove them later). This is more efficient, but it is less-defensive. Alternatively, we could make the whole set be a UInt16 type.

using System;
using System.Collections.Generic;

public class PortParser
{
    private static IEnumerable<int> extractPorts(string ports)                
    {
        string[] portList = ports.Split(',');
        SortedSet<int> retval = new SortedSet<int>();

        foreach(string element in portList)
        {
            if (element.Contains("-"))
            {
                // Range case...
                try
                {
                    // Low value in [0], high value in [length - 1],
                    // And ignore anything else in the middle.
                    int[] range = Array.ConvertAll(element.Split('-'), int.Parse);

                    // Add each number, from the low-end of the range to the
                    // high end. If the range is inverted, we add nothing.
                    for (int j = range[0]; j <= range[range.Length - 1] && j <= UInt16.MaxValue; j++) 
                    {                               
                        retval.Add(j);
                    }
                }
                catch { }
            }
            else
            {
                // Single port case...
                try
                {
                    retval.Add(UInt16.Parse(element));
                }
                catch { }
            }
        }
        return retval;
    }

    static void Main(String[] args)
    {
        if(args.Length < 1)
        {
            System.Console.WriteLine("Please give me some numbers to play with!");
            return;
        }

        IEnumerable<int> results = extractPorts(args[0]);

        foreach(int result in results)
        {
            System.Console.WriteLine("Scanning port " + result);
        }
    }
}
share|improve this answer
    
in this case is maybe a good idea not to try to fix the list of ports and advice the user to do it by himself, then proceed with the rest after the user got it fixed? –  blade Apr 19 at 20:52
1  
@fernando -- generally, yes, if you run into an ambiguous/malformed situation (such as 2-1-3), it's best to yell at the user, tell them what's broken, and make them fix up the input until everything is right. If your text field supports regex matching input, try something like /(([:digit:]+(,|$))|([:digit:]+-[:digit:]+(,|$)))+/. Then all you should have to worry about is too-big numbers and inverted ranges (and there's no reason you can't handle 5-1 just like 1-5). The purist approach would still have the function throw if the input violates the regex, however. :) –  Travis Snoozy Apr 19 at 22:25

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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