Inefficient Code Assistance

This is the place for queries that don't fit in any of the other categories.

Re: Inefficient Code Assistance

Postby setrofim » Fri Mar 29, 2013 3:19 pm

KHarvey wrote:My point was that what they currently mean is place holder. So in 6 months from now they will still mean place holder. This code is something that I actually wrote in VBScript around 10 or 11 years ago

If they have been place holders for 10 years, is it still worth holding that place?


KHarvey wrote: But when I tried to design a class for this script I couldn't quite figure out how, or maybe why, I would use a class. Pretty much all I could figure out is that I would just be using the variables in a class rather than the variables that I am using now. I guess the point is to move this whole script into a class of it's own?

One part that might be good to split out into a class is reading raw input and processing it into a form that makes it easier to work with in the rest of the script. You might also want to use classes for storing data rather than complicated list/dict structures. Basically, whenever you have a set functions all operating on the same set of variables, those functions fuctions should probably be methods in a class and those variables its data members. If you find that you want to stick the entire script in a class (and the script is sufficiently large), then you probably needs to think about decoupling its parts a bit more. Generally, a program can be broken down into distict parts, each with a specific responsibility.


KHarvey wrote:I prefer vim over emacs.

Amen to that; constantly holding down CTRL gets old fast :). If you're using vim then you already have basic auto-completion (the default mapping, IIRC, is <C-n>/<C-p> in insert mode; try ":help i_CTRL-n"). You can get more advanced completetion with ctags. You will also want to install the python-mode bundle. It pre-configures vim for Python development (things like four column indent, expand tabs to spaces, etc) and has some useful tools like pylint (finds bugs in your code -- can save you hours of debugging).
setrofim
 
Posts: 288
Joined: Mon Mar 04, 2013 7:52 pm

Re: Inefficient Code Assistance

Postby KHarvey » Fri Mar 29, 2013 6:28 pm

setrofim wrote:
KHarvey wrote:My point was that what they currently mean is place holder. So in 6 months from now they will still mean place holder. This code is something that I actually wrote in VBScript around 10 or 11 years ago

If they have been place holders for 10 years, is it still worth holding that place?

I meant a place holder to know that there is another set of variables in the list. I have to have the variables in there as it is part of the process to find the info that I need. But knowing what the variables mean does not effect the code, but knowing that they are there and what they are is helpful.

Okay so here is my new code. Some of my variables and functions have the same name they did before as I will have to go through and update all of my references in all of my scripts :roll:
Code: Select all
sql_ip_query = sql_cursor.fetchall()

#Ping all devices everywhere to populate the MAC table on the switches
#Pull the network segments for DocScan.xml
network_segments = funcNetworkSeg()
for network_segment in network_segments:
   #Run nmap scan using the Network Segments from DocScan.xml
   nmap_command = "nmap %s -sP 1>/dev/null 2>&1" % network_segment
   nmap_ip_scan = subprocess.Popen(nmap_command,
         stdin=None, stdout=-1, stderr=None, shell=True
   )
   nmap_ip_scan.wait()

snmp_mac_bp_ii_port = {}

total_parts = len(sql_ip_query)
for sql_ip_query_results in sql_ip_query:
   current_part = 1
   progress_bar = funcProgressBar(current_part, part_total_counter, current_part_running, total_parts)
   if progress_bar is not None:
      sys.stdout.write(str(progress_bar))
   current_part_running = current_part_running + 1

   ip = str(sql_ip_query_results[1])
   sql_switch_name = str(sql_ip_query_results[0])
   snmp_mac_bp_ii_port[sql_switch_name] = []

   ping_status = funcPingDevice(ip)
   if ping_status == "Success " + ip:
      snmp_vlan_walk = snmp_walk("2c", "", ip, "1.3.6.1.4.1.9.9.46.1.3.1.1.4.1")
      snmp_vlans = [snmp_oid(snmp_vlan, -1, "= STRING:") for snmp_vlan in snmp_vlan_walk]
      snmp_oid_macs = []
      snmp_oid_bps = []
      snmp_oid_iis = []
      snmp_oid_ports = []
      for snmp_vlan in snmp_vlans:
         snmp_vlan_prefix = "@"
         snmp_vlan_number = ''.join([snmp_vlan_prefix, snmp_vlan[0]])
         snmp_oid_switch_macs = snmp_walk("2c", snmp_vlan_number, ip, ".1.3.6.1.2.1.17.4.3.1.1")
         snmp_oid_switch_bps = snmp_walk("2c", snmp_vlan_number, ip, ".1.3.6.1.2.1.17.4.3.1.2")
         snmp_oid_switch_iis = snmp_walk("2c", snmp_vlan_number, ip, ".1.3.6.1.2.1.17.1.4.1.2")
         snmp_oid_switch_ports = snmp_walk("2c", snmp_vlan_number, ip, ".1.3.6.1.2.1.31.1.1.1.1")

         snmp_oid_macs.extend(snmp_oid_switch_macs)
         snmp_oid_bps.extend(snmp_oid_switch_bps)
         snmp_oid_iis.extend(snmp_oid_switch_iis)
         snmp_oid_ports.extend(snmp_oid_switch_ports)
      snmp_mac_bp_ii_port[sql_switch_name].append(snmp_oid_macs)
      snmp_mac_bp_ii_port[sql_switch_name].append(snmp_oid_bps)
      snmp_mac_bp_ii_port[sql_switch_name].append(snmp_oid_iis)
      snmp_mac_bp_ii_port[sql_switch_name].append(snmp_oid_ports)

switches_ports_macs = []
for switch_name in snmp_mac_bp_ii_port.keys():
   oid_macs = snmp_mac_bp_ii_port[switch_name][0]
   oid_bps = snmp_mac_bp_ii_port[switch_name][1]
   oid_iis = snmp_mac_bp_ii_port[switch_name][2]
   oid_ports = snmp_mac_bp_ii_port[switch_name][3]
   oid_macs_parsed = dict(
         snmp_oid(oid_mac, -6, "= Hex-STRING:") for oid_mac in oid_macs
   )
   oid_bps_parsed = dict(
         snmp_oid(oid_bp, -6, "= INTEGER:") for oid_bp in oid_bps
   )
   oid_iis_parsed = dict(
         snmp_oid(oid_ii, -1, "= INTEGER:") for oid_ii in oid_iis
   )
   oid_ports_parsed = dict(
         snmp_oid(oid_port, -1, "= STRING:") for oid_port in oid_ports
   )

   for mac_key, mac_address in oid_macs_parsed.iteritems():
      if mac_key in oid_bps_parsed:
         if oid_bps_parsed[mac_key] in oid_iis_parsed:
            if oid_iis_parsed[oid_bps_parsed[mac_key]] in oid_ports_parsed:
               switches_ports_macs.append((
                     switch_name,
                     oid_ports_parsed[oid_iis_parsed[oid_bps_parsed[mac_key]]],
                     mac_address.replace(" ", "-")
               ))

#The set fuction removes all duplicates from a list,
#and the list function turns the set string back into a list
switches_ports_macs = list(set(switches_ports_macs))


This code appears to be working. I will probably go through and add in some comments to help someone who may view my code later.
I did move the functions to another file, but they are same that I had before.

Let me know if there is anything else that is glaringly wrong and I will attempt to fix that too.

I did run into one problem that I was never able to figure out, so I just coded around it.
KHarvey
 
Posts: 34
Joined: Tue Mar 19, 2013 5:13 pm
Location: US

Re: Inefficient Code Assistance

Postby setrofim » Sat Mar 30, 2013 9:24 am

Your new code looks reasonable enough. Like I said, look into using classes. E.g. you can have a SnmpOid class that encapsulates all those lists, so instead of this
Code: Select all
        snmp_vlan_walk = snmp_walk("2c", "", ip, "1.3.6.1.4.1.9.9.46.1.3.1.1.4.1")
        snmp_vlans = [snmp_oid(snmp_vlan, -1, "= STRING:") for snmp_vlan in snmp_vlan_walk]
        snmp_oid_macs = []
        snmp_oid_bps = []
        snmp_oid_iis = []
        snmp_oid_ports = []
        for snmp_vlan in snmp_vlans:
            snmp_vlan_prefix = "@"
            snmp_vlan_number = ''.join([snmp_vlan_prefix, snmp_vlan[0]])
            snmp_oid_switch_macs = snmp_walk("2c", snmp_vlan_number, ip, ".1.3.6.1.2.1.17.4.3.1.1")
            snmp_oid_switch_bps = snmp_walk("2c", snmp_vlan_number, ip, ".1.3.6.1.2.1.17.4.3.1.2")
            snmp_oid_switch_iis = snmp_walk("2c", snmp_vlan_number, ip, ".1.3.6.1.2.1.17.1.4.1.2")
            snmp_oid_switch_ports = snmp_walk("2c", snmp_vlan_number, ip, ".1.3.6.1.2.1.31.1.1.1.1")

            snmp_oid_macs.extend(snmp_oid_switch_macs)
            snmp_oid_bps.extend(snmp_oid_switch_bps)
            snmp_oid_iis.extend(snmp_oid_switch_iis)
            snmp_oid_ports.extend(snmp_oid_switch_ports)

it'd be more like
Code: Select all
        snmp_vlan_walk = snmp_walk("2c", "", ip, "1.3.6.1.4.1.9.9.46.1.3.1.1.4.1")
        snmp_vlans = [snmp_oid(snmp_vlan, -1, "= STRING:") for snmp_vlan in snmp_vlan_walk]
        snmp_oid = SnmpOid()
        for snmp_vlan in snmp_vlans:
            snmp_oid.add_vlan(snmp_vlan)

And then the poulating of individual lists is handled by the class inside the add_vlan() method. Esentially, you'd be replacing four lines with one that means the same thing, making the structure of your code clearer, and also you'll be reducing number of places you need to modify, e.g. if you want to add another list with something.

Also rather than doing this:
Code: Select all
for switch_name in snmp_mac_bp_ii_port.keys():
    oid_macs = snmp_mac_bp_ii_port[switch_name][0]
    oid_bps = snmp_mac_bp_ii_port[switch_name][1]
    oid_iis = snmp_mac_bp_ii_port[switch_name][2]
    oid_ports = snmp_mac_bp_ii_port[switch_name][3]

you can just do
Code: Select all
for switch in snmp_mac_bp_ii_port.values():
    oid_macs = switch[0]
    oid_bps = switch[1]
    oid_iis = switch[2]
    oid_ports = switch[3]


Apart from that, looks good.
setrofim
 
Posts: 288
Joined: Mon Mar 04, 2013 7:52 pm

Previous

Return to General Coding Help

Who is online

Users browsing this forum: patzelt and 3 guests