Ctest #2
Ctest #2
Conversation
Add files via upload
# Conflicts: # my-WOPI.py
There was a problem hiding this comment.
Overall a really great solution!
You managed to keep the implementation mostly quite simple.
I have a few general comments:
- Good code structure
- Good use of docstrings
- PEP8: It's worth reading up about pep8, which is the style guide for Python. It helps make code readable by setting rules, such as a max line length of 79 characters, and naming conventions. there is also a command line tool called pycodestyle that you can run on your code to check that it is pep8 compliant.
- Meaningful names: Meaningful names for functions/variables can go a long way to helping other people to read and understand your code (and yourself if you have to go back and understand some code you wrote years ago and have forgotten how it all works).
A good thing to keep in mind is that you spend more time reading code than writing it. Making your code easily readable and maintainable with the use of is really important.
| <component name="VcsDirectoryMappings"> | ||
| <mapping directory="$PROJECT_DIR$" vcs="Git" /> | ||
| </component> | ||
| </project> |
There was a problem hiding this comment.
I suspect this file was created by your IDE.
It is usually best to keep these sort of files out of your git repo either by not ever adding it, i.e. don't do:
$ git add .idea/vcs.xml
Or by including the '.idea' directory in a .gitignore file.
| import matplotlib | ||
| from math import pi | ||
| import numpy as np | ||
| import datetime |
There was a problem hiding this comment.
The suggested order for imports is to group your imports into the following categories:
- standard library imports
- other library imports
- imports from your local library
Also, each category should be alphabetical
So your imports would look like:
import argparse
import datetime
import json
from math import pi
import matplotlib
import matplotlib.animation as animation
import matplotlib.pyplot as plt
import numpy as np
| import datetime | ||
|
|
||
|
|
||
| def parseCommands(): |
There was a problem hiding this comment.
PEP8 prefers functions to be written in lowercase with underscores inbetween the words (see PEP8)
So here your function would be called parse_commands
| ax.legend(loc='lower right') | ||
| plt.draw() | ||
|
|
||
| # initialise the first frame of the animation if animated, plot if otherwise |
There was a problem hiding this comment.
Comments like these can be really helpful! They help the person reading the code to understand what is going on
So don't be afraid to use them!
| patch_color = config['groups'][group]['color'] | ||
| values = config['groups'][group]['data'] | ||
| polygon_coords = np.array(list(zip(angles, values))) | ||
| polygon = matplotlib.patches.Polygon(polygon_coords, color=patch_color, alpha=0.4, |
There was a problem hiding this comment.
I can't find an import for matplotlib.patches? It should ideally be added to your imports at the top of the file.
It may be that by importing matplotlib, it also imported matplotlib.patches under the hood, but you can't always rely on this!
As an example, there was a submodule in iris that also imported when you would import iris. This happened because, at the time, it was being used in the __init__.py file of iris. However, a later release of iris removed the import of the submodule as it was no longer needed and that broke a bunch of people's code that were relying on this.
Basically, it is usually best practice to import every submodule you make use of.
| """ | ||
| with open(config_file) as json_file: | ||
| config = json.load(json_file) | ||
| cleanConfig(config) |
There was a problem hiding this comment.
It's a common practice to order your functions such that they are defined above the place they are used. That means that iwere you to read through the module from start to end, when you get to a function, you would have already seen everything it uses.
So here you use cleanConfig but it is defined later on in the file, so it would be better to move this below cleanConfig
| return config | ||
|
|
||
|
|
||
| def checkType(dict_, key, type_): |
There was a problem hiding this comment.
This name could be misleading, on first glance it may seem like a general type checking function, when it specifically look at values of the given dictionary. Perhaps could be renamed as check_config_item_type or if you want it to be more general to work with any dictionary you could do: check_dict_item_type
| * key (string) | ||
| * type_ (type) | ||
| """ | ||
| if type(dict_[key]) is not type_: |
There was a problem hiding this comment.
It is usually better practice to use isinstance as it is aware of inheritance of classes.
| raise Exception("{0} does not have type {1}. {0} had type {2}".format(key, type_, type(dict_[key]))) | ||
|
|
||
|
|
||
| def cleanConfig(config): |
There was a problem hiding this comment.
This is quite a large function, you could look at breaking it down into smaller parts.
A problem with large functions, amongst others, is that they make unit testing more difficult.
| if 'min' not in config: | ||
| config['min'] = int(min(data_set) - 1) | ||
| if config['min'] > 0: | ||
| config['min'] = 0 |
There was a problem hiding this comment.
It doesn't seem necessary to include min in the config if you set it to be 0 anyway?
Unless you are also trying to handle negative data values, although I would have assumed that that would cause issues for your plots?
This should highlight everything added so far.