6
\$\begingroup\$

Below is the current working code for an ongoing project.

I am wondering if there is anything I can do to improve the code snippet below. My main concerns lie to two area. First being that I append the answer of an input to a list called inputList and then call it in a block of conditionals. I am wondering if this in particular is a good approach. Are there any better approaches?

Secondly, similar concerns lay with how I defined the answers and dictionaries of their variants. My main purpose there is that, regardless of how you enter "yes" or "no", as long as it has those letters and in the proper order, i.e "nO" or "yES", it would be accepted. I also did it as I did so that I would not have to concern myself on checking whether or not the first letter of any of those two words are capitalised or not.

Again, my question being, is this a "solid" approach? What alternative approaches would be better for this snippet of code? Perhaps a better question would be, how can I improve this snippet?

I am also interested in seeing if I can shorten it as much as possible. Similar to how permLet() is written.

from itertools import product, zip_longest
import pylab as plot
import numpy as number
import pprint

inputFile = input('File Name: ') 


def yLimits():
    yMin = int(input('Set lower limit: '))
    yMax = int(input('Set upper limit: '))
    labelLocation = number.arange(len(count))
    plot.bar(labelLocation, list(count.values()), align='center', width=0.5)
    plot.xticks(labelLocation, list(count.keys()))
    plot.xlabel('Characters')
    plot.ylabel('Frequency')
    plot.ylim(yMin, yMax)
    plot.show()

def ylimChoice():

    #returns all permutations of letter capitalisation in a certain word.
    def permLet(s):
        return(''.join(t) for t in product(*zip(s.lower(), s.upper())))

    inputList = []
    yesNo = input('Would you like to set custom ylim() arguments? ')
    inputList.append(yesNo)
    yes = list(permLet("yes"))
    no = list(permLet("no"))

    if any(yesNo in str({y: y for y in yes}.values()) for yesNo in inputList[0]):
        yLimits()
    elif any(yesNo in str({n: n for n in no}.values()) for yesNo in inputList[0]):
        labelLocation = number.arange(len(count))
        plot.bar(labelLocation, list(count.values()), align='center', width=0.5)
        plot.xticks(labelLocation, list(count.keys()))
        plot.xlabel('Characters')
        plot.ylabel('Frequency')
        plot.autoscale(enable=True, axis='both', tight=False)
        plot.show()

count = { }
with open(inputFile, 'r') as info:
    readFile = info.read()
    for character in readFile.upper():
        count.setdefault(character, 0)
        count[character] = count[character]+1

value = pprint.pformat(count)
print(value)

ylimChoice()
\$\endgroup\$
1
  • \$\begingroup\$ please could you post the full code . \$\endgroup\$ – Tolani Sep 24 '16 at 22:06
3
\$\begingroup\$

Reusability

One of the main weakness of your code is that it is not reusable. Say I have a piece of code that compute some bounds that fits my needs better than the autoscale, I cannot feed this computation to your function: you force me to enter them by hand at the input prompt.

Instead, these kind of parameters to a function should be declared, well…, as parameters. Whether you get them using input or dark woodoo magic doesn't matter to me, because I just want to feed my own to the function. Same kind of argument applies to the data.

Lastly, looking at the similar pieces of code in yLimits or ylimChoice, you can reduce duplication of code:

def show_figure(data, y_bounds=None):
    labelLocation = number.arange(len(data))
    plot.bar(labelLocation, list(data.values()), align='center', width=0.5)
    plot.xticks(labelLocation, list(data.keys()))
    plot.xlabel('Characters')
    plot.ylabel('Frequency')
    if y_bounds is None:
        plot.autoscale(enable=True, axis='both', tight=False)
    else:
        y_min, y_max = y_bounds  # tuple unpacking
        plot.ylim(y_min, y_max)
    plot.show()

This way, you can ask for input before and then call the function:

if any(yesNo in str({y: y for y in yes}.values()) for yesNo in inputList[0]):
    yMin = int(input('Set lower limit: '))
    yMax = int(input('Set upper limit: '))
    show_figure(count, (yMin, yMax))
elif any(yesNo in str({n: n for n in no}.values()) for yesNo in inputList[0]):
    show_figure(count)

CLI

Separating user input from computation also let you more options about how to get said input. One of them is using a Command Line Interface to let the user configure the script at startup without worrying for it being stuck at some point waiting for input. getopt and argparse are two standard python modules that let you do that.

In your case, you would like to define an interface allowing the user to optionnaly enter two integers that will define your bounds, if any.

Using argparse you could write:

import argparse


def parse_command_line():
    parser = argparse.ArgumentParser(decription='My script description')
    parser.add_argument(
        '-y', '--y-bounds', dest='bounds', nargs=2, type=int,
        metavar='Y', help='Define y_min and y_max, the plot '
        'ordinates bounds, if you don\'t want to rely on autoscale')
    args = parser.parse_args()
    return args.bounds

By default, if the user don't use the command-line option, args.bounds will be set to None (same as the default value of show_figure, how convenient); but if the user launches the script using python the_script.py -y 3 15, args.bounds will be [3, 15]. argparse will make sure for you that there are two values following the `-y' switch and that those values are integers.

You can obsiously do the same with whatever parameters you want, such as the input file name. And get support from your shell tab-completion feature when typing said name.

\$\endgroup\$
3
\$\begingroup\$

It does not improve readability to rename modules. There are a few common aliases used, usually to shorten the name: import numpy as np and import matplotlib.pyplot as plt. Apart from that you should stick to the module name.

I was searching your whole code for the initialization of plot and number, because they sound more like normal variables and not like modules. Especially plot sounds like an object.


You should also choose more descriptive variable names (and adhere to PEP8, Python's official style-guide). capitalization_permutations tells you immediately what the function does, permLet does not.


To iterate over the lines of a file, it is sufficient to do:

with open(inputFile, 'r') as info: for character in info: character = character.upper() # do something

But since you actually want the characters, you could just use collections.Counter here:

with open(inputFile, 'r') as info:
    count = collections.Counter(info.read().upper())

Instead of value = pprint.format(count); print(value) you can directly do pprint.pprint(count).


Currently your main code is all over the place. inputFile is defined in the beginning, followed by the function definitions, followed by more code.

Instead follow this build-up:

  1. Module imports
  2. CONSTANTS
  3. Class definitions
  4. function definitions
  5. rest of the code

You could put all your code into a main function and call it like this:

def main():
    inputFile = input('File Name: ') # Delete whitespace at end of line here
    with open(inputFile, 'r') as info:
        count = collections.Counter(info.read().upper())
    pprint.pprint(count)
    ylimChoice(count)

if __name__ == "__main__":
    main()

This allows you to do e.g. import plot_parameters to use plot_parameters.ylimChoice() in another script without the main part of the code being run.


At the same time you can remove the reliance on the global variables by just passing the relevant object as parameters (e.g. count to ylimChoice and yCounts).


Lastly this monstrosity:

inputList = []
yesNo = input('Would you like to set custom ylim() arguments? ')
inputList.append(yesNo)
yes = list(permLet("yes"))
no = list(permLet("no"))

if any(yesNo in str({y: y for y in yes}.values()) for yesNo in inputList[0]):
    ...

What you want to know is if the user-choice is some permutation of yes with random capitalization. This is easiest achieved with if user_choice.lower() == 'yes'. At the same time you append the user_value to a list, and then iterate over the first element of that list to use in an any (with one element)(!). That's just too much overhead. Just use this:

user_input = input('Would you like to set custom ylim() arguments? ')
if user_input.lower() in ('y', 'yes'):
    yLimits(count)
else:
    ...
    # do other stuff

Note that I also allowed the user to just type y. Also, the other code will run on all other input. Your code does nothing (not even a notice that the user input was not recognized) if a wrong word is entered.

\$\endgroup\$
0

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.