Hot answers tagged

13

I would suggest to separate the concerns of coloring and animating. Now, in particular theater_chase_rainbow duplicates code from other methods. Instead, have theater_chase take a color array as parameter. If you then add some animation styles and color schemes you can have \$O(n^2)\$ amount of fun trying all combinations. It seems to me that in this code i +...


12

Style Python has a style guide. It's called PEP8, and normally if you follow it you get easier to read code. It's a fairly small read, and you can learn some new things from it. It's recommended in PEP8 to limit the character width of scripts to 79 characters. It has a few benefits: People on StackExchange don't need to use the horizontal scroll. It's ...


11

Models - General The comments on the model definitions don't add much value. I could figure out the relationships from the db schema; tell me instead something about what the classes are meant to represent, any invariants they have, anything not made explicit in the code. Models - User When defining a backreference, as you do in User, I like to go to the ...


8

Your imports and module structure seems like an incredible mess. Yet your application only contains 3 files. Let's try and improve that. Value your namespaces import this Preferably in an interactive session, will give you an insight of how you should approach problems in Python. In your case, it is the last line Namespaces are one honking great idea -- ...


8

Structure Nothing! The code seems fairly straightforward and clear, and I was able to understand what you’re doing even if I didn't study flask too much. So I don’t think there’s anything wrong with the structure. Code style One small nitpick: some of your lines are too long (>79 characters). From PEP8: Limit all lines to a maximum of 79 characters. ...


6

the idea is nice, but I have few concerns. Firs of all, trying to call importlib every time you are accessing the API endpoint is a bit expensive. It will be a way better if you will introduce some cache first. You can use 2 approaches to build that cache: Lazy, where you lookup cache first and if you cannot find the proper handler, you are trying to load ...


6

As Reinderien mentions, you can decrease some of the complexity of your code by inverting your if-statements. While I cannot test this because I do not have your whole code, here is how it could be done: def get_image(width, height, collection_sku, order_id, name, handle): """image processing pipeline""" # Check-1 image exception should be checked ...


6

Disclaimer: I haven't done authentication in Flask before. Tl;dr: Don't do authentication yourself, it's VERY hard to do right (and using a global ain't right); also, separate the user's authentication from their key store: the former you should ONLY be able to verify, not decode, and the latter should be inaccessible to your server without some secret ...


5

Exception itself actually inherits from a class called BaseException so: class BaseException(Exception): Seems backwards.


5

You don't need dynamic type creation I don't quite understand why you're creating new types dynamically. I think you just need to define a constructor for your FindExact or DictionaryResource classes, and then pass the keyword argument resource_class_args to add_resource: resource_class_args (tuple) – args to be forwarded to the constructor of the ...


5

You could break your main function into multiple functions. Doing that will not even make main() much more clearer but also you will get some re-useable functions. Having these tiny functions allow you to mock them easily in tests and also allows them to be tested them separately. I have also added response.raise_for_status() calls to raise an HTTPError if ...


5

Document with docstrings. Your functions and ORM models are currently not providing any useful documentation regarding their respective function within your program. Don't initialize the database on module level: You should put that into a function: Base.metadata.create_all(engine) Base.metadata.bind = engine Session = (sessionmaker(bind=engine)) #...


4

It's a bit difficult to read this code, because it violates many Pythonic coding practices. The base url http://api.steampowered.com/ISteamUser is duplicated in many places. It would be better to extract to a global variable. You might want to do likewise to all the API urls, for example: API_BASEURL = 'http://api.steampowered.com/ISteamUser' ...


4

Just because you don't have many people using this API now, this does not mean that this will always be the case. In addition, you might want to discriminate between the users of the API for various reasons: To know how many there actually are To know which user/application is generating all those requests suddenly To rate-throttle some of them if needed ...


3

Move the os.environ and sys.path calls into a new setUpClass method instead of have them at the global level? pass at the end of setUp should be removed, similarly the commented out print. I'd put the constants for the URL and the headers into fields or methods (self.url() / self.url) to reduce duplication. The docstrings are a bit much. Ideally the code ...


3

How about this? def log_exception(self, exc_info): msg = '' msg += 'Time: %s\n' % datetime.now() msg += 'Path: %s\n' % request.path msg += 'HTTP Method: %s\n' % request.method msg += 'POST_ARGS: %s\n' % request.form msg += 'GET_ARGS: %s\n' % request.args msg += '...


3

You may want to use a different template for error messages. You can still reuse the overall design by inheriting from a base template. You replace variable values a lot, making it hard to follow what the meaning of variables like num, number and strnumber are throughout the code. Instead write functions which create the various transformations of the input ...


3

In this code, at first I didn't see the "danger" until I scrolled to the right by accident: def menu(parent_id=0, menutree=None): menutree = menutree or [] cur = g.db.execute('select id, parent, alias, title, ord from static where parent="'+ str(parent_id) +'" and ord>0 order by ord') fetch = cur.fetchall() if not fetch: return ...


3

Opening files using with It's not a great idea to open a file just using something like this: dtd = open(Config.DTDPATH, 'r') If your program unexpectedly exits, or crashes, resources that are taken up by the opened file aren't freed. This can be bad in some situations. The proper way to open a file would to be using a context manager, or with, like this: ...


3

Building on @Drewverlee's answer, I would consider renaming BaseException to APIException, or something of the like. Also, I don't think passing app as an argument is best practice, I would recommend using the function as a decorator on your endpoints instead: # Your imports from application import app # Or whatever your non-relative reference is def ...


3

The MongoClient was designed to be instantiated once per app life cycle, so this is redundant. You should try to reuse MongoClient instances as much as possible. Basically the only reason to create a new instance is if you want to configure it in a different way. Also you've mentioned your API is supposed to be RESTful, so a few notes here: 1) To deal with ...


3

You probably want to move the session to a global variable(or simply use requests.get) else at function level it is not helping in re-using any connections right now and is adding an extra level of indentation to your code. This is mostly opinionated but grouping from x import y based imports below import z makes the imports bit easier to read. Plus keep the ...


3

Well, I shall start from the top. According to the style guide, PEP 8 imports should be in the following order: standard library imports related third party imports local application/library specific imports Plus, add a single blank line to separate each group. So, something like the following: import subprocess import threading import logging ...


3

There are a few things bothering me in one version or an other: the error=error or None is unnecessary as error is either assigned 'Invalid Email or Password' or None, stick to the simpler error=error; the ): on its own line in the third version is more than disturbing, it feels like a syntax error and force the reader to go through the code again to ...


2

Why not take advantage of the RESTful architecture of the internet? I suggest you have the code generate a static page and set the expires header on that resource (or set of resources) to the end of that day (considering your audience's time zones, I guess). This might help. Sorry, if I had experience with heroku or flask I would provide code or more ...


2

I'm not familiar with that framework, but if there is more than one school and you are going to use Python to access those values (such as the school's address), I'd use a class. Think of it this way: Classes are usually meant to be instantiated, or at least to be a group of functions for a specific purpose. Instead of creating a dict for each school you ...


2

def get_lines(admin): try: data = admin.pages.get_one({'site': ADMIN_SITE, 'page': SOURCE_PAGE})['content'] except: flash('Error: unable to fetch the production-wiki page with mappings') return You should definitely throw an exception rather than flash here. My approach is to define a UserError exception. When those are caught they ...


2

Since the Fibonacci algorithm is so nice and clean there is one little nitpicky thing to add. You don't need to directly return 0 and 1 in the base cases, you can just return n. So the function can be rewritten (in Javascript, my experience with Python is extremely subpar) as: var fib = function(n) { if(n === 0 || n === 1) { return n; } ...


2

Some comments: Instead of using print statements, the logging module could be used The return value of get_google_content should be a boolean value (True on success, False on failure) Regarding ways to make the refresh_content method a little bit more pythonic, I would probably use a for loop and the all built-in function to make sure that all the calls to ...


Only top voted, non community-wiki answers of a minimum length are eligible