patternpythonMinor
Implementation of *nix ipcalc
Viewed 0 times
implementationnixipcalc
Problem
I posted a JS subnetting calc before, but I wanted to try a CLI version, so I made this.
The parts I'm most worried about are:
This was primarily a challenge so I could get better at writing Python... obviously it'll never compete with Perl or C.
Also, I do know I need to comment more/better.
```
#!/usr/bin/env python
# -- coding: utf-8 --
"""
some comments
"""
import argparse
import math
import socket
import struct
prog = 'ipcalc'
usage = '{0} [-n NUMHOST | -s SUBMASK] [IPv4 address/mask]'.format(prog)
description = ('Subnetting Calculator\n=====================\nTakes both an '
'IPv4 address and either a submask in CIDR notation,\n'
'quad-dotted notation, or an arbitary number of hosts.'
'\n\nAddress range, host values, submasks, etc.')
epilogue = (
'example:\n\tipcalc 192.168.0.1/24\n\tipcalc 192.168.0.1 24\n\t'
'ipcalc -n 192.168.0.1 250')
parser = argparse.ArgumentParser(usage=usage,
description=description,
epilog=epilogue,
formatter_class=argparse.RawTextHelpFormatter)
parser.add_argument(
'ip',
metavar='IPv4 address/mask',
type=str,
help='CIDR notation of IPv4 address\n192.168.0.1/24')
parser.add_argument(
'prefix',
metavar='CIDR prefix',
type=str,
help='CIDR prefix OR submask\n24, 16, etc. OR 255.255.0.0, etc.',
const=None,
nargs='?')
parser.add_argument(
'-n',
'--numhost',
type=int,
help='integer number of hosts')
parser.add_argument(
'-c',
'--colors',
action='store_true',
help='Colors off')
parser.add_argument('--version', action='version', version='%(prog)s 1.0')
args = parser.parse_args()
HEADER = '\033[95m'
OKBLUE = '\033[94m'
OKGREEN = '\033[92m'
WARNING = '\033[93m'
FAIL = '\033[91m'
ENDC = '\033[0m'
if False is not args.colors:
HEADER = O
The parts I'm most worried about are:
- structure (could I refactor the layout better?)
- Did I goof any math?
This was primarily a challenge so I could get better at writing Python... obviously it'll never compete with Perl or C.
Also, I do know I need to comment more/better.
```
#!/usr/bin/env python
# -- coding: utf-8 --
"""
some comments
"""
import argparse
import math
import socket
import struct
prog = 'ipcalc'
usage = '{0} [-n NUMHOST | -s SUBMASK] [IPv4 address/mask]'.format(prog)
description = ('Subnetting Calculator\n=====================\nTakes both an '
'IPv4 address and either a submask in CIDR notation,\n'
'quad-dotted notation, or an arbitary number of hosts.'
'\n\nAddress range, host values, submasks, etc.')
epilogue = (
'example:\n\tipcalc 192.168.0.1/24\n\tipcalc 192.168.0.1 24\n\t'
'ipcalc -n 192.168.0.1 250')
parser = argparse.ArgumentParser(usage=usage,
description=description,
epilog=epilogue,
formatter_class=argparse.RawTextHelpFormatter)
parser.add_argument(
'ip',
metavar='IPv4 address/mask',
type=str,
help='CIDR notation of IPv4 address\n192.168.0.1/24')
parser.add_argument(
'prefix',
metavar='CIDR prefix',
type=str,
help='CIDR prefix OR submask\n24, 16, etc. OR 255.255.0.0, etc.',
const=None,
nargs='?')
parser.add_argument(
'-n',
'--numhost',
type=int,
help='integer number of hosts')
parser.add_argument(
'-c',
'--colors',
action='store_true',
help='Colors off')
parser.add_argument('--version', action='version', version='%(prog)s 1.0')
args = parser.parse_args()
HEADER = '\033[95m'
OKBLUE = '\033[94m'
OKGREEN = '\033[92m'
WARNING = '\033[93m'
FAIL = '\033[91m'
ENDC = '\033[0m'
if False is not args.colors:
HEADER = O
Solution
Nice tool! And I think it's pretty well done. There's still some room for improvements though ;-)
Structure
Much of your code is within functions, which is good. But then, roughly half of the code is in the global namespace. It would be better have pretty much everything inside functions. For example:
All this and similar stuff would be good inside a function that's in charge of parsing arguments, for example:
Unit testing
Since many of the functions can be tricky to get right, it would be good to unit test them, for example
Note that moving all functionality inside functions as I mentioned in the earlier point is a requirement before you can add unit tests. You cannot import the script for testing purposes if it immediately launches the command line interface. Once all the code is properly inside functions, this won't be a problem anymore, and unit testing becomes possible.
Pythonic expressions
These are strange and non-Pythonic examples:
The Pythonic way would be:
Also this:
More common and simpler to write like this:
There are other similar examples in the code, I suggest to update everywhere similarly.
Misc
I find it not intuitive that a flag called
Instead of this:
I would find more readable this way:
Structure
Much of your code is within functions, which is good. But then, roughly half of the code is in the global namespace. It would be better have pretty much everything inside functions. For example:
prog = 'ipcalc'
usage = '{0} [-n NUMHOST | -s SUBMASK] [IPv4 address/mask]'.format(prog)
description = ('Subnetting Calculator\n=====================\nTakes both an '
'IPv4 address and either a submask in CIDR notation,\n'
'quad-dotted notation, or an arbitary number of hosts.'
'\n\nAddress range, host values, submasks, etc.')All this and similar stuff would be good inside a function that's in charge of parsing arguments, for example:
def parse_args():
# ...
def main():
# ...
if __name__ == '__main__':
args = parse_args()
main(args)Unit testing
Since many of the functions can be tricky to get right, it would be good to unit test them, for example
set_bits_from_submask, submask_from_set_bits, and many of the others too.Note that moving all functionality inside functions as I mentioned in the earlier point is a requirement before you can add unit tests. You cannot import the script for testing purposes if it immediately launches the command line interface. Once all the code is properly inside functions, this won't be a problem anymore, and unit testing becomes possible.
Pythonic expressions
These are strange and non-Pythonic examples:
if False is not args.colors:
HEADER = OKGREEN = OKBLUE = WARNING = FAIL = ENDC = '\033[0m'
if None is not hosts:
# ...The Pythonic way would be:
if args.colors:
HEADER = OKGREEN = OKBLUE = WARNING = FAIL = ENDC = '\033[0m'
if hosts:
# ...Also this:
uint32_t = uint32_t - ((uint32_t >> 1) & 0x55555555)
uint32_t = uint32_t + (uint32_t >> 8)More common and simpler to write like this:
uint32_t -= ((uint32_t >> 1) & 0x55555555)
uint32_t += (uint32_t >> 8)There are other similar examples in the code, I suggest to update everywhere similarly.
Misc
I find it not intuitive that a flag called
-c or --colors actually turns colors off. (Hard to suggest something better though... --no-colors, --simple, --plain ?)Instead of this:
epilogue = (
'example:\n\tipcalc 192.168.0.1/24\n\tipcalc 192.168.0.1 24\n\t'
'ipcalc -n 192.168.0.1 250')I would find more readable this way:
epilogue = (
'example:\n'
'\tipcalc 192.168.0.1/24\n'
'\tipcalc 192.168.0.1 24\n'
'\tipcalc -n 192.168.0.1 250')Code Snippets
prog = 'ipcalc'
usage = '{0} [-n NUMHOST | -s SUBMASK] [IPv4 address/mask]'.format(prog)
description = ('Subnetting Calculator\n=====================\nTakes both an '
'IPv4 address and either a submask in CIDR notation,\n'
'quad-dotted notation, or an arbitary number of hosts.'
'\n\nAddress range, host values, submasks, etc.')def parse_args():
# ...
def main():
# ...
if __name__ == '__main__':
args = parse_args()
main(args)if False is not args.colors:
HEADER = OKGREEN = OKBLUE = WARNING = FAIL = ENDC = '\033[0m'
if None is not hosts:
# ...if args.colors:
HEADER = OKGREEN = OKBLUE = WARNING = FAIL = ENDC = '\033[0m'
if hosts:
# ...uint32_t = uint32_t - ((uint32_t >> 1) & 0x55555555)
uint32_t = uint32_t + (uint32_t >> 8)Context
StackExchange Code Review Q#62335, answer score: 2
Revisions (0)
No revisions yet.