6
\$\begingroup\$

I'm new to Python and experimenting with some simple scripts. I would appreciate if you could give me some suggestions on how the following code can be improved or made simpler or anything in general.

#Coverts MAC Addresses into cisco format

def mac_conv(mac, sep='-'):
  #split mac address into 3 groups
  splitted_mac = mac.split(sep)
  gr1 = ''.join(splitted_mac[0:2])
  gr2 = ''.join(splitted_mac[2:4])
  gr3 = ''.join(splitted_mac[4:])

  #join groups into a single MAC
  mac = gr1, gr2, gr3
  final_mac = '.'.join(mac)

  #print final MAC address
  print(final_mac)

#Open file with MAC addresses and convert them
with open('mac.txt', 'r') as f:
  for mac in f.readlines():
    mac_conv(mac)
\$\endgroup\$
3
\$\begingroup\$

Your code's pretty good. The only thing I'd change is to reduce the amount of lines. To do this I would base my algorithm around the use of a list comprehension.

  1. Split MAC into segments.
  2. Slice segments into a three item list. (Like gr1 without the join)
  3. Perform a list comprehension to join the sliced segments together.
  4. Join the new three item list together.
  5. Return it.
segments = mac.split(sep)
groups = [segments[0:2], segments[2:4], segments[4:]]
a = [''.join(group) for group in groups]
mac = '.'.join(a)
return mac

I'd then join the last three lines into one. As return and '.'.join are easy to understand in a single one-liner.

Other than the above function, you don't need to use f.readlines() you can just use f. And you should always use four spaces.

This can result in:

def mac_conv(mac, sep='-'):
    segments = mac.split(sep)
    groups = [segments[0:2], segments[2:4], segments[4:]]
    return '.'.join(''.join(group) for group in groups)

with open('mac.txt', 'r') as f:
    for mac in f:
        print(mac_conv(mac))
\$\endgroup\$
  • \$\begingroup\$ your answer seems more understandable for me at the moment thanks. \$\endgroup\$ – Erjol Bane Aug 12 '16 at 14:44
  • \$\begingroup\$ Well, either four spaces or a tab. But no mixing both!. \$\endgroup\$ – Kroltan Aug 13 '16 at 1:10
5
\$\begingroup\$

First of all, you should be using return rather than print to get a value out of a function, otherwise you'll have problems when you try to expand the program.

def mac_conv(mac, sep='-'):

  #etc

  #return final MAC address
  return final_mac

#Open file with MAC addresses and convert them
with open('mac.txt', 'r') as f:
  for mac in f.readlines():
    print(mac_conv(mac))

I would also avoid reusing variables, especially if its a function argument (mac). You can actually eliminate that usage of the variable all together:

#join groups into a single MAC
final_mac = '.'.join([gr1, gr2, gr3])

If I was writing this, however, I would probably do it in just a couple of lines, as you know exactly how the input and output strings are going to be formatted:

def mac_conv(mac, sep='-'):
    mac_parts = mac.split(sep)
    return "{0[0]}{0[1]}.{0[2]}{0[3]}.{0[4]}{0[5]}".format(mac_parts)

If this was going into a product, I would probably add some error handling too, to allow for input that isn't a mac address - too short, too long, wrong separators, alternate groupings (a cisco-formatted address is still a valid mac address, but this function won't handle it.)

\$\endgroup\$
  • 1
    \$\begingroup\$ why not use tuple unpacking: return "{}{}.{}{}.{}{}".format(*mac.split(sep)) \$\endgroup\$ – Graipher Aug 12 '16 at 14:15
  • \$\begingroup\$ thanks for your answer. I don't understand how does this part ""{0[0]}{0[1]}.{0[2]}{0[3]}.{0[4]}{0[5]}".format(mac_parts)" work though, because of my python knowledge so far. \$\endgroup\$ – Erjol Bane Aug 12 '16 at 14:50
  • \$\begingroup\$ @ErjolBane take a look at pyformat.info, specifically this section. \$\endgroup\$ – RoadieRich Aug 12 '16 at 15:02
3
\$\begingroup\$

Overall

Code seems fine - it works and is easy to comprehend.

Reusability

You print the result instead of return-ing it. If you wanted to do something else with MAC in that format (like printing it as a part of some string), what would you do?

Naming

There may be other formats for MAC addresses. If someone had successfully guessed that mac_conv converts MAC address, they'd still have to guess the format. mac_to_cisco_format is long, mac_to_cisco may be alright.

separator instead of sep may help a little bit to someone who only sees function's signature (as in many IDEs).

Documentation

#Coverts MAC Addresses into cisco format should be a docstring instead.

#print final MAC address is just a visual garbage - next line is self-descriptive enough.

Algorithm

You assume some format of input data, while it's not spelled-out explicitly. While you could handle that with naming/documentation, you can change your algorithm to convert MACs from several formats to CISCO one - simply remove separators from strings (multiple str.replace-s will do) and then slice the result as you wish.

In fact, you could accept desired format as a parameter (instead of your sep).

\$\endgroup\$

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.