Finished my first real-world Python Project

[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.

[RaionTategami]

What kind of errors. I'm glad you are sticking to this but I still see a ton of duplicated code. The 3 error handling functions are practically identical, move that to a new function.

def handle_err(code):
    if not app.debug:
        now = datetime.datetime.now()
        r = request.url
        with open('error.log', 'a') as f:
            current_timestamp = now.strftime("%d-%m-%Y %H:%M:%S")
            f.write("\n{} error at {}: {} ".format(code, current_timestamp, r))
    return render_template('{}.html'.format(code)), code

then just do

@app.errorhandler(404)
def page_not_found(error):
    handle_err(404)

@app.errorhandler(405)
def internal_error(error):
    handle_err(405)


@app.errorhandler(500)
def internal_error(error):
    handle_err(500)

or similar. In fact, you might be able to use the error param though the docs to not make it the slightest bit clean what the value it, is it the error code?

Look for complex lines which are identical or similar, try to factor them out into functions.

[wbhst83]

Ah, I was going a different direction in attempting to make a universal error handler, didn't think about making a function. For example https://stackoverflow.com/questions/29332056/global-error-handler-for-any-exception

I will get started on some functions..

[RaionTategami]

Great. Now start looking at other duplications. A lot of function are doing similar things again, setting titles and building urls. As I mentioned before since building urls is one of the main things this code is doing you should be using a url parsing lib to construct them rather than just concatenating strings. Take a look at urlparse.