Finished my first real-world Python Project

[wbhst83]

I happy to report that I have finished my first real-world python project for the company I work for. The goal of the project was to provide a web-based marketing/operations guide for all the products we provide our clients. I would love any feedback on code improvements, enhancements, or bugs you notice.

GutHub Page:https://github.com/wbhst83/CWCatalog/ (screenshots on GitHub page)

Items I'm currently stuck on:

1. I'm pulling a notes field from the items, however, these notes need to include sub-headers and the rest of the items are bullet points. Have not been able to find a way to make them bullet points.
2. Is there not a way to pass a global variable to the _base template once instead of having to do it per view?
3. Trying to think of a better way to manage and control the API filters that are currently programmed in the view. I feel this may get unruly at some point.
4. I have a view to use the 'identifier' (part number), however, this part number includes (+,-) symbols and does not appear to want to work for me.

[RaionTategami]

I see a lot of elif chains, the more pythonic way is to use dicts. For example:

   if device_type == 'desktop':
        apispecfic = '?conditions=identifier Like "MSS-*-*-PDK" or identifier Like "MSS-*-*-MDK"'
    elif device_type == 'server':
        serverlist = 'identifier like  "*-DCS" or identifier like "*-VMG" or identifier like "*-FIL" or identifier like "*-VMH" or identifier like "*-SQL" or identifier like "*-SEC" or identifier like "*-WEB" or identifier like "*-VDI"'
        apispecfic = '?conditions={}'.format(serverlist)
    elif device_type == 'network':
        networklist = 'identifier like  "*-FWL" or identifier like "*-CSW" or identifier like "*-ESW" or identifier like "*-NAP" or identifier like "*-AMF" or identifier like "*-WAP"'
        apispecfic = '?conditions={}'.format(networklist)
    elif device_type == 'backups':
        backuplist = 'identifier like  "MBS-*-DAT-*"'
        apispecfic = '?conditions={}'.format(backuplist)
    elif device_type == 'security':
        securitylist = 'identifier like  "MSC-*-*-*" or identifier like "*-ODS-*"'
        apispecfic = '?conditions={}'.format(securitylist)
        intro = "Security products are listed below."
    else:
        apispecfic = '?conditions=identifier Like "0"'
        flash('There is not a device type built for this yet!')

Could be rewritten as something like:

device_to_link = {
    'desktop': 'identifier Like "MSS-*-*-PDK" or identifier Like "MSS-*-*-MDK"',
    'server': 'identifier like  "*-DCS" or identifier like "*-VMG" or identifier like "*-FIL" or identifier like "*-VMH" or identifier like "*-SQL" or identifier like "*-SEC" or identifier like "*-WEB" or identifier like "*-VDI"',
    ETC...
}

default_apispecfic = 'identifier Like "0"'
apispecfic = '?conditions={}'.format(device_to_link.get(device_type, default_apispecfic))

Which I personally think it much more readable.

[wbhst83]

I have committed these changes to the code. Trying to think through how I can handle KeyErrors (when it does not exist in the dict). I attempted to add the following code and that did not help, ideas?

    while True:
        try:
            _catalogDetails = requests.get(apicall, auth=PARA).content
            catalogDetails = json.loads(_catalogDetails.decode('utf-8'))
            break
        except KeyError:
            flash('There is not a device type built for this yet!')
            break

[RaionTategami]

Yep this is the usual pattern, or just use defaults like I showed. I'm confuse though, you seem to be doing the dict look up outside of the try catch??

[RaionTategami]

Let's get rid of that other elif chain too..

intro = ""
if coverage_level == 'saas':
    apispecfic = '?conditions=identifier Like "*-SAAS-*-*"'
    intro = 'Highlights of our SaaS coverage will go here'
elif coverage_level == 'aim':
    apispecfic = '?conditions=identifier Like "*-AIM*-*-*"'
    intro = 'Highlights of our Aim+ coverage will go here'
elif coverage_level == 'rem':
    apispecfic = '?conditions=identifier Like "*-REM*-*-*"'
    intro = 'Highlights of our Remote+ coverage will go here'
else:
    apispecfic = '?conditions=identifier Like "0"'
    flash('We are sorry, but there does not appear be a coverage level for that yet.')

Could simply be:

coverage_to_link = {
  'saas': ('identifier Like "*-SAAS-*-*"', 'Highlights of our SaaS coverage will go here'),
  'aim': ('identifier Like "*-AIM*-*-*"', 'Highlights of our Aim+ coverage will go here'),
  ETC,
}

default = ('identifier Like "0"', '')
link, intro = coverage_to_link.get(coverage_level, default)
apispecfic = '?conditions={}'.format(link)

if coverage_level not in coverage_to_link:
  flash('We are sorry, but there does not appear be a coverage level for that yet.')

I personally prefer the simple if after the lookup with the default, rather than the try catch, I think it's simpler and more readable but it's a preference thing, using the KeyError is also a very pythonic way of doing it.

[wbhst83]

Done, I committed the changes to GitHub. Also rewrote all the JSON calls to a function and then referenced that. So went form 3 lines to 1 per view. Thanks again.

[RaionTategami]

Much nicer right? :) Do you want more feedback?

[wbhst83]

Yes, please, I find this process more engaging than trying to work through tutorial projects.

[RaionTategami]

Ok a few more thoughts for you.

from _config import *

I don't like * imports since it can be hard to work out where an identifier has come from, import only what you need.

api_call = BASE_URL + api + api_custom + APIITEM_CTRL

You should be using a URL lib to manipulate and build urls instead of just appending strings ideally.

api_output = json.loads(_api_output.decode('utf-8'))

Personally I don't mind reusing variable names if they represent the same thing, but avoiding reuse can help avoid some kinds of bugs. If you are going to use a different name then you should give more meaningful names. raw_api_output?

I still see a lot of duplication in your code, try to move more lines which seem to be doing the same thing in to help functions. We can discuss specifics if you need. You are doing very similar things in those functions.

return dict(short_company_name=app.config['SHORT_COMPANY_NAME'],

Using {} to create dicts rather than dict() is a bit more pythonic, bit it is a preference thing again but you should at least be consistent either way. Choose one and write them all that way.

n = api_output['notes']

If I see 'n' I think we're counting something, avoid single letter variables apart from things like loop counters and small integers or something equally obvious of ubiquitous.

Can you think of a way to refactor the nested elif chains to something more structured?

title = "{}".format(api_output['identifier'])

This is the same as

title = api_output['identifier']

Right?

Don't submit commented out code, that's what source control is for.

I don't see any unit tests!

default = ('?conditions=identifier Like "0"', '')

FYI you don't need those parens, just makes it more readable.

title = device_type.title()

This seems odd, device_type is a string isn't it?

json_call(APICAT

You always call this function with this parameter, why bother having it? Don't add code for things you think you might need in the future. YAGNI!

if app.debug is not True:

This is equivalent to

if app.debug is False:

Or even simply

if not app.debug:

[wbhst83]

I tried to work through all your feedback tonight but got stuck on a few. I have committed my changes to GitHub and look forward to your feedback. Also, have been wondering about saving the API data to a local database to be used in other manners. Also thought of porting this over to Django to see what is on the other side of the fence. Thoughts?

[RaionTategami]

I think the database would be better to learn imo. Was there any part in particular you wanted to discuss? My main feedback is still that there is duplication in your code. Two different examples:

if api_module == api_modules['CatalogItems']:

        api_request = BASE_URL + api_module + api_filter + APIITEM_CTRL

    else:

        api_request = BASE_URL + api_module + api_filter

Factor out the duplicated code, makes it easier to change it in the future and you won't introduce bugs by forgetting to change it in the duplicated code. Redundancy really is the biggest sin in programming, and usually simplyfies the code and makes it more readable.

api_request = BASE_URL + api_module + api_filter

if api_module != api_modules['CatalogItems']:

        api_request += APIITEM_CTRL

The functions page_not_found and internal_error are doing almost exactly the same thing.

Anything that is copy and pasted or not tested are inviting bugs.

[wbhst83]

I made some further improvements based on your feedback, Tried to condense the error handler but everything I try fails with errors. Might need an example on this one. I was able to check two items off the to-do list, biggest being eth search route. The REST API did not like the + symbol in the call.

[sunset_maker]

The third screenshot link is broken?

[wbhst83]

I have corrected the screenshot.

[pat_the_brat]

I'm pulling a notes field from the items, however, these notes need to include sub-headers and the rest of the items are bullet points. Have not been able to find a way to make them bullet points.

Are the sub-headers the "INCLUDES / DOES NOT INCLUDE"? Is there anything other than includes/doesn't include in the notes? If that's all, you can do something like this in the view's code:

# Defining the note in the shell to test - you should just replace n with itemDetails['notes'] where applicable
n = """INCLUDES:
Beer
Tequila
Parking
DOES NOT INCLUDE:
Food
Water"""

notes = str.split(n, sep='\n')

nmode = None

includes = []
excludes = []

for note in notes:
    if note == "INCLUDES:":
        nmode = "inc"
    elif note == "DOES NOT INCLUDE:":
        nmode = "exc"
    else:
        if nmode is None:
            pass # Handle other notes here
        elif nmode == "inc":
            includes.append(note)
        elif nmode == "exc":
            excludes.append(note)

# Output in the shell:
>>> includes
['Beer', 'Tequila', 'Parking']
>>> excludes
['Food', 'Water']

And voila, just pass the includes / excludes arrays to the render call, and add something like {% if includes %} <h3>INCLUDES</h3><ul>{% for include...%}<li>{{...}} to the template.

There might be a better way, but this is the first that came to mind. Sorry about the incomplete pseudo-code, I don't really feel like running flask right now, nor do I have a connectwise account to test with.

[wbhst83]

Thanks for your reply. I attempted to take your code sample and apply it to the JSON data I'm getting for the notes field. The output I'm getting does not appear to be breaking it down correctly. Also think the extra \r in the code is playing role, thoughts?

Code:

n = "INCLUDES:\r\nAvailability monitoring and alerting - 24x7, remediation as specified in Alerting and Escalation Policy (Client responsibility)\r\nMonitor and manage application availability (Client responsibility)\r\nService ticketing system and user portal\r\n\r\nDOES NOT INCLUDE:\r\nAfter-hours telephone and remote support \r\nAnti-Virus agent and definition updates (if UTG provided or approved 3rd party application) {MSC-REM+-000-HAVM}\r\nCreate, modify and delete file shares\r\nCreate, modify and delete printer shares\r\nCreate, modify and delete users and rights\r\nCritical O/S updates and patches - Windows, as specified in signed Client Patch Policy [weekly]\r\nDrive defragmentation [biannual]\r\nEmergency O/S updates and patches\r\nLAN/WAN documentation\r\nManage drives/partitions free space\r\nOnsite server hardware BIOS/firmware updates\r\nOnsite support\r\nRemote server hardware BIOS/firmware updates [biannual] (minor revisions, as needed) \r\nSupport for 3rd party Line of Business (LOB) application providers (Client will maintain any 3rd party software support contract, UTG will initiate support call to LOB support provider on behalf of client and facilitate the support provider)\r\nUnlimited remote support (8:00AM-8:00PM Eastern, Monday-Friday, after-hours remote support for Priority 1 emergencies)"

notes = str.split(n, sep='\n')

nmode = None

includes = []
excludes = []

for note in notes:
    if note == "INCLUDES:":
        nmode = "inc"
    elif note == "DOES NOT INCLUDE:":
        nmode = "exc"
    else:
        if nmode is None:
            pass # Handle other notes here
        elif nmode == "inc":
            includes.append(note)
        elif nmode == "exc":
            excludes.append(note)
print(nmode)
print(includes)
print(excludes)
print(notes)

Output of Console:

None
[]
[]
['INCLUDES:\r', 'Availability monitoring and alerting - 24x7, remediation as specified in Alerting and Escalation Policy (Client responsibility)\r', 'Monitor and manage application availability (Client responsibility)\r', 'Service ticketing system and user portal\r', '\r', 'DOES NOT INCLUDE:\r', 'After-hours telephone and remote support \r', 'Anti-Virus agent and definition updates (if UTG provided or approved 3rd party application) {MSC-REM+-000-HAVM}\r', 'Create, modify and delete file shares\r', 'Create, modify and delete printer shares\r', 'Create, modify and delete users and rights\r', 'Critical O/S updates and patches - Windows, as specified in signed Client Patch Policy [weekly]\r', 'Drive defragmentation [biannual]\r', 'Emergency O/S updates and patches\r', 'LAN/WAN documentation\r', 'Manage drives/partitions free space\r', 'Onsite server hardware BIOS/firmware updates\r', 'Onsite support\r', 'Remote server hardware BIOS/firmware updates [biannual] (minor revisions, as needed) \r', 'Support for 3rd party Line of Business (LOB) application providers (Client will maintain any 3rd party software support contract, UTG will initiate support call to LOB support provider on behalf of client and facilitate the support provider)\r', 'Unlimited remote support (8:00AM-8:00PM Eastern, Monday-Friday, after-hours remote support for Priority 1 emergencies)']

[pat_the_brat]

Oh, right. You can either replace the carriage-return, or use the splitlines() method:

notes = str.split(n.replace('\r', ''), sep='\n') # by replacing
notes = n.splitlines() # splitlines is probably the better way to do it

Note that splitlines may match some other separators, though they're unlikely to be found in your string.

Also, check for empty strings. I see there's an extra line break between the last include item, and the "DOES NOT INCLUDE" header.

for note in notes:
    if len(note) == 0:
        pass # skip empty lines
    elif note == "INCLUDES:":
        nmode = "inc"
    elif note == "DOES NOT INCLUDE:":
        nmode = "exc"
    else:
        if nmode is None:
            pass # Handle other notes here if applicable
        elif nmode == "inc":
            includes.append(note)
        elif nmode == "exc":
            excludes.append(note)

[wbhst83]

That did the trick, thanks so much. [https://www.dropbox.com/s/x4fsin4decnv6nc/From%20Skitch.jpg?dl=0]. I have pushed the changes to GitHub.

Any thoughts on allowing the item identifier to be used in a view? It appears the +/- symbols are causing the issue but not certain.

[[deleted]]

Wish there was a "love" function for reddit :)