2
\$\begingroup\$

Please give me some pointers on how I can improve this project with Python 2.7.

import json
import urllib
urls = ["https://data.calgary.ca/resource/kqmd-3dsq.json","https://data.calgary.ca/resource/4wni-k3sg.json"]
print " \nThis is Json Data Parser Program. \nThis program will print the Unique Voting Station Name from 2013 or 2017 City Elections from City of Calgary OpenData Portal"
CRED = '\033[91m'
CEND = '\033[0m'

try:
    year= int (raw_input("\nElection Year to Search [2013 or 2017]:"))

    if ( year not in range(2013,2018,4) ):
        year=int(raw_input("Invalid Year ! \nPlease type again Year to Search [2013 or 2017]:"))\

except (EOFError, KeyboardInterrupt, ValueError):
    year= int (raw_input("\nThe exception has occured !!\nPlease type again Year to  Search [2013 or 2017]:"))
else:
    print "You choose: {} year for Elections Results".format(year)

try:
    ward=int(raw_input("\nWard to Search:"))

    if ( ward not in range(0,14)):
        ward=int(raw_input("Ward does not exist in City of Calgary !\nPlease type again Ward to Search:"))

except (EOFError,ValueError,KeyboardInterrupt):
    ward=int(raw_input("\nThe exception has occured !!\nPlease type again Ward to Search:"))
else:
    print "\nYou choose ward: {} for Elections Results".format(ward)



def jsonUrl(year):
    global url
    if (year == 2017):
        url = urls[0]
        return url
    elif (year == 2013):
        url = urls[1]
        return url
    else:
        raise Exception('The City of Calgary only has data for 2013 and 2017')

jsonUrl(year)

response = urllib.urlopen(url)
data= json.loads(response.read())

def wardstnname(ward):
    wardwithscore = "{}".format(ward)
    wardstnresult = []
    for i in data:
        try:
            if (i["ward"] == wardwithscore and  i["voting_station_name"] not in wardstnresult):
                        wardstnresult.append(i["voting_station_name"])
        except KeyError:
            continue
    if len(wardstnresult) > 0:
        print "\nThere were total", len(wardstnresult),"unique voting stations in your ward"
        print "\nThe list of the Voting Station were as follows"
        for votingstnname,b in enumerate(wardstnresult,1):
            print '{} {}'.format(votingstnname,b)
    else:
        print CRED,"\nCity of Calgary has no Data on this Ward for Voting Station Name on Year", year,CEND

wardstnname(ward)

def wardOffice(ward):
    wardwithscore= "{}".format(ward)
    officeResult = []
    for i in data:
        if (i["ward"] == wardwithscore and i["office"] == "COUNCILLOR" and i["ballot_name"] not in officeResult):
            officeResult.append(i["ballot_name"])
    if len(officeResult) > 0:
        print "\nThere were total", len(officeResult),"councillors in your ward"
        print "\nThe list of the Councillors were as follows"
        for councillorname,b in enumerate(officeResult,1):
            print '{} {}'.format(councillorname,b)
    else:
        print CRED,"\nCity of Calgary has no Data on this Ward for Councillors on Year", year,CEND

wardOffice(ward)
\$\endgroup\$
  • 1
    \$\begingroup\$ Change the title and description to reflect what this program does. \$\endgroup\$ – yuri Jul 22 '18 at 7:21
  • 2
    \$\begingroup\$ @yuri Can you please try to be more friendly in your comments? \$\endgroup\$ – Mast Jul 22 '18 at 9:26
  • \$\begingroup\$ @yuri Hello , Apologize as this has been my first post . The title has been fixed now.Please let me know if this is ok \$\endgroup\$ – iking15 Jul 22 '18 at 14:47
  • \$\begingroup\$ @Mast Hello Apologize as this is my first post, The title has been fixed for now. \$\endgroup\$ – iking15 Jul 22 '18 at 14:48
  • \$\begingroup\$ @iking15 Do go through How to Ask :) \$\endgroup\$ – hjpotter92 Jul 22 '18 at 16:12
2
\$\begingroup\$

First, you should stop using Python 2, if possible. It will stop being supported in a bit over a year. Switch to Python 3, it is better in many ways.


Now, let's start with year not in range(2013,2018,4). This is a very inefficient way to do this, because in Python 2, range just generates a list and in lookup in lists is \$\mathcal{O}(n)\$. Instead use a collection for which in is \$\mathcal{O}(1)\$, like set or `dict. This is a good habit to form, even though here this list has only two elements.

As a side note, this was such a common beginner's mistake that they made sure that the Python 3 implementation of range (which is a generator on steroids), is actually \$\mathcal{O}(1)\$.


Now, to come to the underlying problems of the code. It is currently a mix of global variables, global code, function definitions, which sometimes take arguments, but also use global variables, and all of which modify global state (even though some of hem also return something). And these functions are then called right away.

Instead, try to define pure functions, that is functions that take all they need as arguments and return their result, without modifying any global state. You can then use these functions in a main method that does all the work.

import json
import requests
CRED = '\033[91m'
CEND = '\033[0m'

URLS = {2013: "https://data.calgary.ca/resource/kqmd-3dsq.json",
        2017: "https://data.calgary.ca/resource/4wni-k3sg.json"}

WARDS = set(range(0, 14))


def ask_year():
    while True:
        try:
            year = int(raw_input("\nElection Year to Search {}:".format(sorted(URLS.keys()))))
            if year not in URLS:
                print "No data for that year!"
                continue
        except ValueError:
            print "Please enter a valid year!"
            continue
        return year


def ask_ward():
    while True:
        try:
            ward = int(raw_input("\nWard to Search:"))
            if ward not in WARDS:
                print "Ward does not exist in City of Calgary !"
                continue
        except ValueError:
            print "Invalid ward number!"
            continue
        return ward

def get_voting_stations(ward, data):
    for station in data:
        try:
            if station["wards"] == ward:
                yield station["voting_station_name"]
        except KeyError:
            continue


def print_ward_voting_stations(ward, data):
    vorting_stations = set(get_voting_stations(ward, data))
    if vorting_stations:
        print "\nThere were in total", len(vorting_stations), "unique voting stations in your ward"
        print "\nThe list of the Voting Station were as follows"
        for i, name in enumerate(vorting_stations, 1):
            print '{} {}'.format(i, name)
    else:
        print CRED, "\nCity of Calgary has no Data on voting stations for ward {} in year {}".format(ward, year), CEND


def get_councillors(ward, data):
    for ballot in data:
        if ballot["ward"] == ward and ballot["office"] == "COUNCILLOR":
            yield ballot["ballot_name"]

def print_ward_councillors(ward, data):
    councillors = set(get_councillors(ward, data))
    if councillors:
        print "\nThere were in total", len(councillors), "councillors in your ward"
        print "\nThe list of the Councillors were as follows"
        for i, name in enumerate(councillors, 1):
            print '{} {}'.format(i, name)
    else:
        print CRED, "\nCity of Calgary has no Data on this Ward for Councillors in year", year, CEND


if __name__ == "__main__":
    print " \nThis is Json Data Parser Program. \nThis program will print the Unique Voting Station Name from 2013 or 2017 City Elections from City of Calgary OpenData Portal"

    year = ask_year()
    print "You chose: {} year for Elections Results".format(year)
    ward = str(ask_ward())
    print "\nYou chose ward: {} for Elections Results".format(ward)

    data = requests.get(URLS[year]).json()
    print_ward_voting_stations(ward, data)
    print_ward_councillors(ward, data)

The major differences are: I used a dict to map from the year to the url of the json file. This automatically raises a KeyError if an undefined year is given. You also already ensure that what the user entered is a valid year, here I re-used the keys of that dictionary.

I used a set for the valid wards. Both the urls and the wards are constant, and therefore written in ALL_CAPS, as recommended by Python's official style-guide, PEP8.

I separated the filtering of the data for what you need and making it unique. For this I defined each one function for outputting a stream of the voting stations/the office the ballot was for. This is then consumed by set, which makes it unique (but removes the order, which I think is not important here). This once again removes the necessity to do e.g. i["voting_station_name"] not in wardstnresult, which is again \$\mathcal{O}(n)\$.

I also removed catching KeyboardInterrupt and EOFError. Catching these errors should be well motivated. The first one appears when the user presses Ctrl+C, which means they want to abort whatever you are doing. Ignoring that and telling them to please enter a valid year is almost passive aggressive. The same goes for the latter, which happens if the user presses Ctrl+D. This is used to denote that the input is finished (especially important for multi line input). I don't see any reason whay you would want to catch that here.

Instead I replaced these with an infinite while True loop, which continues asking the user until they enter a valid year/ward or press Ctrl+C, which kills the whole program.

\$\endgroup\$
  • \$\begingroup\$ This is really good answer. For the new Python programer like me some of the concepts are still new and seen first time. Like _main_ function. I will try to digest this first and let you know if I have any questions or concerns.I really appreciate your input \$\endgroup\$ – iking15 Jul 22 '18 at 20:59
  • \$\begingroup\$ so How does the _main_ function works over here. I thought python goes code by code line and then execute. Thus I had to use some Global variable like url in the code previously. Since it was failing with the Name Error: url not defined But in your code _main_ function block comes very last and executed at First step. Any concepts help would be really appreciated. \$\endgroup\$ – iking15 Jul 22 '18 at 21:13
  • 1
    \$\begingroup\$ @iking15 Python does indeed execute sequentially. So in my code, it first defines all functions (this runs only the headers of the functions). It then gets to the block surrounded by the if, in which it goes from top to bottom. Note that all the variables each function needs are either global constants (like URLS), or local parameters (like ward and data), which are passed along to the function. \$\endgroup\$ – Graipher Jul 24 '18 at 18:43

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.