2
\$\begingroup\$

I'm very new to python and scripting in general. I've come up with the following function to find the first and last IP when an IP range string is inputted. I want to only use base Python (and not download any libraries essentially).

Example:

genRange("1.2.3.*"):
Start IP = 1.2.3.0
End IP = 1.2.3.255

genRange("1.*"):
Start IP = 1.0.0.0
End IP = 1.255.255.255

genRange("1.2.3.4"):
Start IP = 1.2.3.4
End IP = 
NotRange

This seems very hardcoded to me, because it really is. Is there a a better way to do this that is obvious/jumps out at you?

I still need to handle invalid input.

def genRange(ipRangeString):
    startIP = ""
    endIP = ""
    if '.' in ipRangeString:
        ipV = 4
    if ':' in ipRangeString:
        ipV= 6
    if ipV == 4:
        parts = ipRangeString.split(".")
        i = 0
        for item in parts:
            if item != "*":
                if 0 <= int(item) <= 255:
                    if (i<=3):
                        startIP = startIP + item + '.'
                        if (i==3): startIP = startIP[:-1]
                    i+= 1
            else:
                tempSIP = startIP
                endIP = startIP
                while (i<=3):
                    endIP = endIP + "255"
                    startIP = startIP  + "0"
                    if (i<3):
                        endIP = endIP + '.'
                        startIP = startIP + '.'
                    i+= 1
    print "Start IP = ", startIP
    print "  End IP = ", endIP
    if not (endIP):
        print "NotRange"
\$\endgroup\$
7
\$\begingroup\$

The first thing that jumps out at me is that this is a single function, with no classes and only one possible use. But without context, I couldn't comment on whether or not that's good. Since the snippet is so short and limited, your only motivation for changing it is that you might want to use it in a larger context. So I'm going to assume a larger context which may or may not apply.

The second thing, then, is that there are no comments. I can look at this code, see the ipV is 4 or 6, see "Start IP" and "End IP", and figure out that this is computing an IP address range for IPv4. But the function name doesn't really tell me that. Just for maintainability sake, you might want to do something about that.

The third thing that jumps out at me is that this function does nothing useful. It has two (conditionally 3) print statements as side effects, which means that you can't use these results. You might want to separate the computation of the range from the printing of the results. This is where I have to assume a larger context. If you just want to print those 2 or 3 lines, why bother modifying working code to begin with other than to maybe add some comments for the future. What if you want to accept ranges (10.0.1.5-50)? Heck, what if you want ranges with holes (10.0.5-10.2-100)?

Fourth, your variable i is the index of item in the loop, which is used in a lot of logic in your function. See https://stackoverflow.com/questions/522563/accessing-the-index-in-python-for-loops for some discussion on that. Specifically, don't manage your own loop index variable. I see why you are doing so; to loop the necessary times between 1.* and 1.1.*. You can do this a few different ways. Statically, you need 4 parts to your IPv4 addresses. You could iterate over the range 4 and generate these 4 parts. You could iterate over your input parts and append the missing parts as needed once complete. I don't actually have a strong opinion of the right way, only that your way looks funny to me.

Fifth, tempSIP. important? used?

Punchline without assuming a larger context: Add a comment about usage. Add a comment about the program logic. Rename the function to have IP in the name. Delete tempSIP.

\$\endgroup\$
4
  • \$\begingroup\$ Thank you very much for the input. You were correct in assuming thist is going to be used in much larger context. The print statements I have were just for debugging, I will be passing startIP, endIP, and the NotRange flag as well and using them elsewhere in code. Also completely forgot about the built in indexing of enumerate() and also forgot to remove tempSIP after a code change :) \$\endgroup\$ Nov 30 '11 at 14:39
  • \$\begingroup\$ Also, the second part of your fourth point was the reason I wanted input, because it seems funny to me as well. Sure it works, but it just doesn't look like the best code. \$\endgroup\$ Nov 30 '11 at 14:42
  • \$\begingroup\$ Well, then one more comment about this. Does your context require assignable addresses? Remember that *.255 is multicast and won't be assigned to a machine. Of course, if you are assigning addresses (Hey, it's a use case; I wrote a network simulator for security research.), you should consider making an IPRange iterator. Computing the range returns an IPRange object with start, end, and the python iterator interface. Abstracting the iteration is doubly important if you do assignment in a range with holes. \$\endgroup\$
    – ccoakley
    Nov 30 '11 at 14:49
  • \$\begingroup\$ *.255 could be assignable and not a multicast address ofc, but there's no worries there. The big picture here is I'm simply prepping the input from human being the range to be a start/end IP address that plug into an SQL query to match startIP <= IP <= endIP. I am however getting closer to understanding how to think in python based on your replies, and I'll experiment with it. Thanks greatly ccoakley :) \$\endgroup\$ Nov 30 '11 at 16:30

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.