Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wandering Warriors #54

Merged
merged 134 commits into from Jan 26, 2020
Merged

Wandering Warriors #54

merged 134 commits into from Jan 26, 2020

Conversation

@n0remac
Copy link
Contributor

n0remac commented Jan 26, 2020

Here is our abacus. Beads can be moved by clicking them. You can draw cuneiform symbols on the sand pad which will automatically be read onto the ledger. Press the equals button to see the result on the abacus!

A5rocks and others added 30 commits Jan 18, 2020
Set up development.
initial kivy skeleton
Base Application Layout
…till bugs to be worked out.
Basic Abacus Widget Design
Future note: Using Line for a border is bad because Line-s curve.
Instead set a background color with Rectangle and canvas.before, and then do whatever you want on top
Cuneiform Ledger Canvas
Cameron drawing pad
conform new features to same folder structure
rgables and others added 18 commits Jan 26, 2020
Improved cuneiform gfx
Settings
Help Popups
UX Improvements
Fix
lemonsaurus pushed a commit that referenced this pull request Jan 26, 2020
Fixing Issue #54.  Adding toolbars to training screens.
Fix2
@lemonsaurus

This comment has been minimized.

Copy link
Member

lemonsaurus commented Jan 26, 2020

Lint is failing:

./wandering-warriors/main.py:8:1: E402 module level import not at top of file
./wandering-warriors/main.py:9:1: E402 module level import not at top of file
./wandering-warriors/main.py:10:1: E402 module level import not at top of file
./wandering-warriors/modules/gesture_db.py:6:101: E501 line too long (117 > 100 characters)
./wandering-warriors/modules/gesture_db.py:7:101: E501 line too long (115 > 100 characters)
./wandering-warriors/modules/gesture_db.py:9:101: E501 line too long (114 > 100 characters)
./wandering-warriors/modules/gesture_db.py:10:101: E501 line too long (118 > 100 characters)
./wandering-warriors/modules/gesture_db.py:14:101: E501 line too long (116 > 100 characters)
./wandering-warriors/modules/gesture_db.py:15:101: E501 line too long (105 > 100 characters)
./wandering-warriors/modules/gesture_db.py:16:101: E501 line too long (114 > 100 characters)
./wandering-warriors/modules/gesture_db.py:25:101: E501 line too long (103 > 100 characters)
./wandering-warriors/modules/gesture_db.py:26:101: E501 line too long (117 > 100 characters)
./wandering-warriors/modules/gesture_db.py:31:101: E501 line too long (118 > 100 characters)
./wandering-warriors/modules/gesture_db.py:32:101: E501 line too long (115 > 100 characters)
./wandering-warriors/modules/gesture_db.py:35:101: E501 line too long (106 > 100 characters)
./wandering-warriors/modules/gesture_db.py:36:101: E501 line too long (109 > 100 characters)
./wandering-warriors/modules/gesture_db.py:37:101: E501 line too long (115 > 100 characters)
./wandering-warriors/modules/gesture_db.py:38:101: E501 line too long (118 > 100 characters)
./wandering-warriors/modules/ledger.py:50:101: E501 line too long (104 > 100 characters)
./wandering-warriors/modules/ledger.py:168:101: E501 line too long (106 > 100 characters)
./wandering-warriors/modules/ledger.py:169:101: E501 line too long (103 > 100 characters)
./wandering-warriors/modules/abacus.py:34:1: W293 blank line contains whitespace
./wandering-warriors/modules/abacus.py:53:60: W291 trailing whitespace
./wandering-warriors/modules/abacus.py:170:101: E501 line too long (135 > 100 characters)
./wandering-warriors/modules/abacus.py:272:101: E501 line too long (123 > 100 characters)
./wandering-warriors/modules/abacus.py:326:37: E203 whitespace before ':'
./wandering-warriors/modules/abacus.py:328:33: E203 whitespace before ':'
./wandering-warriors/modules/abacus.py:350:24: W291 trailing whitespace
@lemonsaurus lemonsaurus merged commit c334221 into python-discord:master Jan 26, 2020
1 check failed
1 check failed
Code Jam 6 - Linting Build #20200126.254 failed
Details
Copy link
Member

aeros left a comment

Thanks for the submission @n0remac (and to the rest of the Wandering Warriors team)!

A few general points:

  1. The README should have included an informative description of what the project is, setup, and cover general usage. This project seems to only have the generic starting README for the Code Jam.

  2. Screenshots of the running application are very helpful, to give an overview of its core functionality at a glance.

  3. While a decent number of the commit messages were fairly informative (other than some, such as temp commit, fix and fix2), there seems to be a very large number of unnecessary merge commits. These can be interactively cleaned up through usage of git rebase -i. See the git docs for more info: https://git-scm.com/docs/git-rebase.

  4. There seems to be a significant lack of documentation in the form of docstrings and code comments throughout the project. Preferably, any function that exceeds more than a few lines or so should include at least a basic docstring (if not every function), which summarizes the purpose of the function, parameters, and the output. Also, inline comments should be used to explain anything that may not be obvious to those reading the source code of the project.

  5. While type hints are not required by any means, they're quite useful to include in function/method signatures; both for documentation purposes and for type checking. I would definitely consider using them, particularly for builtin types and those included by the typing module. However, if you do use them, the entire project should make use of them. For example, they were not used in abacus.py, but used extensively for ledger.py.

  6. The overall code and organizational quality of different parts of the project vary rather drastically. To me, this indicates that perhaps more thorough internal review could have been done by more experienced members of the team to help guide the less experienced members.

With the more generally applicable part out of the way, I'll proceed with line-by-line code review; noting any sections that I have constructive feedback for. I tried to avoid repeating similar feedback across multiple areas, unless it particularly stood out. Also, I focused my critique more on general Python programming feedback, as my experience with using Kivy is rather minimal.

Note: I plan on doing a follow-up review that will include installing the application and providing feedback from a user perspective (if/when I get the chance). This one is more focused on the implementation details and code review.


<ClearButton>:
Button:
size: [min(min(self.parent.parent.width, self.parent.parent.height) / 8, 96)] * 2

This comment has been minimized.

Copy link
@aeros

aeros Jan 28, 2020

Member

[min(min(self.parent.parent.width, self.parent.parent.height) / 8, 96)] * 2 seems to be used repeatedly throughout calculator.kv; this should probably be an inherited property or constant.





Comment on lines +218 to +221

This comment has been minimized.

Copy link
@aeros

aeros Jan 28, 2020

Member

The amount of whitespace between sections and general formatting in this file is rather inconsistent. At the end of the day, the format chosen isn't a huge deal, but it should at the least aim to be as consistent as possible within the same file.

size: self.parent.size
allow_stretch: True
source: self.parent.parent.parent.button_image('plus')
Button:

This comment has been minimized.

Copy link
@aeros

aeros Jan 28, 2020

Member

Many of these buttons in particular seem to have a lot of repeated properties. Is it possible to use single default button for this section (such as OperationsButton), inherit from it, and then override the properties that need to be changed (such as pos_hint, on_release, and the Image source)?

This may not seem significant for individual repeated sections, but it adds a lot of additional "white noise" that distracts from the core logic. It also makes it rather cumbersome to have to redefine all these one-by-one if you want to change even a single property for all of buttons that use the same setup.

class TopRightButton(Widget):
pass


class ClearButton(Widget):
pass


class HelpButton(Widget):
pass


class SettingsButton(Widget):
pass
Comment on lines +37 to +50

This comment has been minimized.

Copy link
@aeros

aeros Jan 28, 2020

Member

This definitely seems like it's under-utilizing inheritance by having all of these buttons inherit from Widget instead some form of parent button class, as many of these buttons seem to share the same properties (see my above comments in calculator.kv).

from .abacus import Abacus
from .draw_pad import DrawPad
from .ledger import Ledger

__all__ = ('Abacus', 'DrawPad', 'Ledger')
Comment on lines +1 to +5

This comment has been minimized.

Copy link
@aeros

aeros Jan 28, 2020

Member

Good usage of __all__ and relative imports here. It's commonly underutilized, but very useful for organizing a project's local modules.

self.df = pd.DataFrame(columns=['x', 'y', 'op', 'z'])
self.new_row()

# def add_cuneiform(self, b10_number: int):

This comment has been minimized.

Copy link
@aeros

aeros Jan 28, 2020

Member

Instead of leaving a large commented-out TODO section like this, I'd recommend making use git stash to store work that's in progress, and in the meantime have it raise a NotImplemented exception (or simply leave it out).

"""
Ledger data structure:
x: first number in equation (float)
y: second number in equation (float)
op: operator ['+', "-", '*', '/'] (str)
z: result (float)
"""
Comment on lines +9 to +15

This comment has been minimized.

Copy link
@aeros

aeros Jan 28, 2020

Member

Excellent usage of a docstring here to describe the layout of the class. Many of the other classes in the project could benefit greatly from something similar.

The only thing I would additionally recommend is to include a brief summary that describes the purpose of the class.

@@ -0,0 +1,114 @@
"""
Generate full set of cuneiform graphics from just the 1 and 10 symbol

This comment has been minimized.

Copy link
@aeros

aeros Jan 28, 2020

Member

This file is rather creative, it's quite impressive that all of the graphics below were able to be generated from reusing two symbols.

# one
out = blank_image_gen()
out.paste(ONE, (100, 100), mask=ONE)
out.save(GENERATED_DIR / 'c1.png')

# two
out = blank_image_gen()
[out.paste(ONE, (50 + 100 * i, 100), mask=ONE) for i in range(2)]
out.save(GENERATED_DIR / 'c2.png')

# three
out = blank_image_gen()
[out.paste(ONE, (100 * i, 100), mask=ONE) for i in range(3)]
out.save(GENERATED_DIR / 'c3.png')

# four
out = blank_image_gen()
[out.paste(ONE, (100 * i, 50), mask=ONE) for i in range(3)]
out.paste(ONE, (100, 150), mask=ONE)
out.save(GENERATED_DIR / 'c4.png')

# five
out = blank_image_gen()
[out.paste(ONE, (100 * i, 50), mask=ONE) for i in range(3)]
[out.paste(ONE, (50 + 100 * i, 150), mask=ONE) for i in range(2)]
out.save(GENERATED_DIR / 'c5.png')

# six
out = blank_image_gen()
for i in range(3):
for j in range(2):
out.paste(ONE, (100 * i, 100 * j + 50), mask=ONE)
out.save(GENERATED_DIR / 'c6.png')

# seven
out = blank_image_gen()
for i in range(3):
for j in range(3):
out.paste(ONE, (100 * i, 100 * j), mask=ONE)
out.paste(ONE, (100, 200), mask=ONE)
out.save(GENERATED_DIR / 'c7.png')

# eight
out = blank_image_gen()
for i in range(3):
for j in range(3):
out.paste(ONE, (100 * i, 100 * j), mask=ONE)
[out.paste(ONE, (50 + 100 * i, 200)) for i in range(2)]
out.save(GENERATED_DIR / 'c8.png')

# nine
out = blank_image_gen()
for i in range(3):
for j in range(3):
out.paste(ONE, (100 * i, 100 * j), mask=ONE)
out.save(GENERATED_DIR / 'c9.png')

# ten
out = blank_image_gen()
out.paste(TEN, (100, 100), mask=TEN)
out.save(GENERATED_DIR / 'c10.png')

# twenty
out = blank_image_gen()
out.paste(TEN, (25, 100), mask=TEN)
out.paste(TEN, (150, 0), mask=TEN)
out.save(GENERATED_DIR / 'c20.png')

# thirty
out = blank_image_gen()
out.paste(TEN, (25, 100), mask=TEN)
out.paste(TEN, (150, 0), mask=TEN)
out.paste(TEN, (150, 200), mask=TEN)
out.save(GENERATED_DIR / 'c30.png')

# forty
out = blank_image_gen()
out.paste(TEN, (25, 100), mask=TEN)
out.paste(TEN, (100, 50), mask=TEN)
out.paste(TEN, (100, 150), mask=TEN)
out.paste(TEN, (200, 0), mask=TEN)
out.save(GENERATED_DIR / 'c40.png')

# fifty
out = blank_image_gen()
out.paste(TEN, (25, 100), mask=TEN)
out.paste(TEN, (100, 50), mask=TEN)
out.paste(TEN, (100, 150), mask=TEN)
out.paste(TEN, (200, 0), mask=TEN)
out.paste(TEN, (200, 100), mask=TEN)
out.save(GENERATED_DIR / 'c50.png')
Comment on lines +24 to +114

This comment has been minimized.

Copy link
@aeros

aeros Jan 28, 2020

Member

So this isn't necessary by any means, but if you wanted to structure this in a way that uses less repeated code you could use something like the following:

paste_arg_sets = {
    # Can use functools.partial() if you want to preserve keyword args (such as *mask*)
    1: [(ONE, (100, 100), ONE)],
    2: [(ONE, (50 + 100 * i, 100), ONE) for i in range(2)],
    3: [(ONE, (100 * i, 100), ONE) for i in range(3)],
    4: [[(ONE, (100 * i, 50), ONE) for i in range(3)], 
        [(ONE, (100, 150), ONE)]],
    # ...
    # Nested list comp
    9: [[(ONE, (100 * i, 100 * j), ONE) for j in range(3)] for i in range(3)],
    # ...
    50: [
        (TEN, (25, 100), TEN),
        (TEN, (100, 50), TEN),
        (TEN, (100, 150), TEN),
        (TEN, (200, 0), TEN),
        (TEN, (200, 100), TEN),
    ],
}

def run_paste_commands(out, arg_sets):
    for args in arg_sets:
        if type(args) == list:
            run_paste_commands(out, args)
        else:
            # print(f"out.paste({args})")
            out.paste(args)

if __name__ == '__main__':
    for num, arg_sets in paste_arg_sets.items():
        out = blank_image_gen()
        run_paste_commands(out, arg_sets)
        out.save(GENERATED_DIR / f'c{num}.png')
self.get_row()

def clear(self):
"""clear ledger and backing dataframe"""

This comment has been minimized.

Copy link
@aeros

aeros Jan 28, 2020

Member

Overall, great usage of docstrings and type hints in this file.

The only thing I can add is that some of the descriptions could use a bit more detail, and it is typically encouraged to use full sentences in docstrings.

self.sm = ScreenManager()
self.sm.add_widget(Calculator(name='calculator'))
self.sm.add_widget(Settings(name='settings'))
Comment on lines +55 to +57

This comment has been minimized.

Copy link
@SebastiaanZ

SebastiaanZ Feb 2, 2020

Member

I don't see any compelling reason for using an abbreviation here. The general recommendation for names in Python is to use descriptive names and I think self.screenmanager or self.screen_manager would have been better for readability.

def get_manager(self):
return self.sm
Comment on lines +59 to +60

This comment has been minimized.

Copy link
@SebastiaanZ

SebastiaanZ Feb 2, 2020

Member

These type of "empty getters" are typically considered non-idiomatic in Python. The Python data model, with its descriptors in the form of @Property, makes them unnecessary, as you can always add custom getter logic later without changing the basic API of the object. This ensures that we don't need custom getter/setter names and we can write classes that follow the "Uniform access principle" (Meyer, 1988/1997, in Object-Oriented Software Construction).

For a more in-depth discussion, see chapter 19 and, especially, chapter 20 of Fluent Python.

def operation(self, op: str):
"""update operator column of current row and advance selection"""
if op not in ['+', '-', '*', '/']:
print(f"[ WARNING ] Invalid operator: {op}")

This comment has been minimized.

Copy link
@SebastiaanZ

SebastiaanZ Feb 2, 2020

Member

This print seems to imitate what a logger powered by Python's logging module would output. I'd recommend you to look into the logging module and start utilizing it for these type of tasks. This will allow you to specify logging levels (which means you don't even have to fully remove debug information since those debug message actually be useful during debugging!) and save your logs to file instead of STDERR/STDOUT if you so wish. Once you get used to it, it's a very flexible system that integrates well with broader logging frameworks.

Copy link
Member

SebastiaanZ left a comment

Code Review

Commit Quality

The first thing that's important to me when I look at a commit history is if I'm able to understand what each commit does based on the summary line and the message body of the commit. While there are commits with informative summary lines, there are no commits with an informative message body. This makes it difficult to make out what each commit does without having to study the underlying code that was committed. There are also some commits with generic commit messages like "Update ledger.py", "refresh test", and "fix" that make it more difficult to follow along what's happening to the code base.

When it comes to the commits themselves, I think the history could have benefited from having cleaner commits for each atomic change. It would be a good idea to use tools like git rebase -i and git commit --amend to make sure the local, work-in-progress git history gets cleaned before it gets pushed to a remote repository. A perfect commit history is one where each commit represents an atomic change, has very few unnecessary commits (to fix linting/style issues), and in which the commit messages serve as documentation for the project as they contain not only what was changed, but also why and how.

That said, as far as the code jam goes, I'd say this commit history is falls within or is slightly above the average category.

Since developing good commit habits isn't easy, here are a few resources on writing good commit messages:

The README

The README contains basic information on how to install the project using pipenv, but not much else. There's no general description of the project and there are no screenshots to advertise the application to people browsing the project. A good README is appealing, as it's often the first thing potential users see of a project, and provides documentation on how to use the application. Overall, I think this README is very meager.

Code Quality

The main thing that jumps out at me is the lack of documentation. While the code mostly isn't very complex, the lack of docstrings and comments makes it more difficult to understand what's going on on without having to study the code instead of just reading it. Take a method like AbacusColumn.shift_up: It returns either None or False and I have no clear idea of what it does by just looking at the method in isolation. After looking at the rest of the code, I think it moves beads to an upper position, but I'm not quite sure if self.up and self.down represent locations in the column or a general direction. Even a short docstring describing the action, optionally combined with slightly more descriptive attribute names, would probably have meant that the purpose of the method was clear within a single read of the code.

I also think the code would benefit from being refactored into smaller units. The Abacus.update method is massive and it's very difficult for me to follow what it's doing. Combined with a lot of shortened variable names (e.g., bead_w; what's w?), it makes for difficult reading.

I do like the object-oriented approach; having an Abacus with AbacusColumns and Beads makes sense on a conceptual level and that makes it easier to understand what's going on. There are also a couple of really great ideas and implementations in the code, like the code generating the cuneiform numerals. I really think the ideas and techniques are there, but I do think the readability of the code hasn't received the attention it deserves.

Summary

Overall, in terms of code quality and readability, I'd rate this project as below average for the Code Jam. The ideas and techniques are there, but the lack of documentation and readability do have an impact on the overall quality. The commit history is decent and probably on par for the code jam. The README is very minimal.

Pros

  • Great techniques and ideas (e.g., cuneiform numeral generation)

  • An object-oriented style that makes conceptual sense

  • Decent commit history

Cons

  • Use of shortened and short variables names

  • Almost no docstrings and comments

  • Very minimal README

  • Parts of the code could use a bit of refactoring

def __init__(self, **kwargs):
super(Abacus, self).__init__(**kwargs)

self.bar_w = self.MAX_BAR_W

This comment has been minimized.

Copy link
@SebastiaanZ

SebastiaanZ Feb 3, 2020

Member

As with the constants defined in the class, I don't know what w means here. I think it means "width", but it would have been much clearer if it wasn't abbreviated.

@lemonsaurus lemonsaurus added the Team PR label Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.