I'm looking to improve the efficiency of my code. Although my current method works, I feel it can be improved.

if ouroraddrlen == (4,):
        ouropip = struct.unpack(">bbbb", payload[6:10]) # Need to change this to accept ipv6 as well 
        print "Our I.P : ", ouropip
        notheirip = struct.unpack(">b", payload[10])
        print "No. of their I.P's : ", notheirip    
        theiroripv = struct.unpack(">b", payload[11])   
        print "Their I.P version:: ", theiroripv
        theiroraddrlen = struct.unpack(">b", payload[12])
        print "Length of their Ip : ", theiroraddrlen
        theirfirstip = struct.unpack(">bbbb", payload[13:17])
        print "First Ip : ", theirfirstip
        theirsecondip = struct.unpack(">bbbb", payload[18:22])
        print "Second Ip : ", theirsecondip

The output is:

Time :  (1401734263,)
Our onion address : 
Ip version :  (4,)
Ip length :  (4,)
Our I.P :  ( )
No. of their I.P's :  (2,)
Their I.P version::  (4,)
Length of their Ip :  (4,)
First Ip :  ( )
Second Ip :  ( )

I have removed the real IPs, but they are just IPV4 addresses.

However, what I am wondering, is if it is possible to include an if statement after this section of code:

notheirip = struct.unpack(">b", payload[10])
        print "No. of their I.P's : ", notheirip    

where if the notheirip is greater than zero and depending on the length of:

        theiroraddrlen = struct.unpack(">b", payload[12])
        print "Length of their Ip : ", theiroraddrlen

which would be either 4 or 16, then it would set the payload values of the next section.

For example, if notheirip = (2,) and theiroraddrlen = (4,), then I would want it to print out

theirip = struct.unpack(">b << the number of b required so either 4 or 16 and then the range, this will always start at 13 and go up to either 4 or 16 in the future and loop until all the IPs are displayed.


Your question was a little confusing. But I will try and summarize it before I go into my solution:

  • You have a struct that you are unpacking and during that process you get the number of IPs 'they' have and the length of those IPs (either 4 or 16)
  • You want to get all of their IPs dynamically based on that length and number of IPs.

Based on my hopefully correct summary above, you can do this simply with a for loop:

num_their_ips = struct.unpack(">b", payload[10])
len_their_ips = struct.unpack(">b", payload[12])

byte_string = '>{}'.format('b'*len_their_ips[0])
their_ips = []
for count in range(num_their_ips[0]):
    start = 13 + (count*len_their_ips[0])
    end = start + len_their_ips[0]

    ip = struct.unpack(byte_string, payload[start:end])
    print 'IP #{}: {}'.format(count+1, ip)


Now a quick note on your naming conventions. Pythonic convention is to use_underscores in variable names. This is especially needed in your code because your variable names are quite hard to distinguish, as was evident in how much 'trouble' I had converting your names below:

our_op_ip, no_their_ip, their_or_ip_v, their_or_addr_len, their_first_ip, their_second_ip

I would also suggest making your names more descriptive. Right now they feel wordy and are a little confusing. Take no_their_ips as an example: does that mean they don't have any IPs, or is it a shortening of 'number'?

Now, I may be being a tad too purposefully dense. However, this is the way people could look at your code; they may not know what some of the acronyms and abbreviations mean. Writing code with descriptive variable names helps improve the readability of your program.

  • \$\begingroup\$ Thank you very much, i will update my code to include the underscore and re look at the naming convention, it is great having someone who can look at my code and help me improve it :) \$\endgroup\$ – user2962401 Jun 3 '14 at 17:26
>>> from collections import namedtuple
>>> Record = namedtuple("Record","IP NoIP IPV LenIP FirstIP SecondIP")
>>> Record._asdict(Record._make(struct.unpack(">LbbbLL",payload[6:])))
{'FirstIP': 1145324612, 'NoIP': 17, 'SecondIP': 1431655765, 'IP': 3140690449L,
IPV': 34, 'LenIP': 51}

is nice and short (note that i used a madeup payload so the values might not make sense , and my short names may note make sense ... I tried to name them based on your names but much shorter ... )

if you wanted to also unpack the IP addresses

new_record["IP"] = struct.unpack("bbbb",struct.pack("L",new_record["IP"]))

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