Expected Indents #3

janine9vn · 2023-09-28T15:43:28Z

No description provided.

Initialize empty source files

…height() methods

random word api

Update game.py

made dis into string from list

Update game.py

home_page

Update game.py

update dev-requirements

added win & lose features

added SVD and winning and loosing functionality

Updated words.json, minor bug fix in game.py - Game._get_random_word()

final fixes before welcome image

Docstring update + welcome image

quick readme

README and LICENSE update

Update README.md

slight change in readme

…81667954' git-subtree-dir: expected-indents git-subtree-mainline: 4bae50a git-subtree-split: 24e2711

etrotta · 2023-10-01T20:56:12Z

expected-indents/src/static/script_game.js

+				/*
+				I forgot exactly how we decided the API response should be... so I assume it's like this:
+				{
+					"word": "w_r_",
+					"lives": 5,
+					"image": "https://example.com/image.png"
+				}
+				*/


This comment is a bit worrying, both it existing in first place as well as it still being there in the PR

etrotta · 2023-10-01T21:16:59Z

Some notes that are not specific to any files, but overall recommendations:
(not requirements from the code jam, but personal recommendations from me)

use unit tests
use type hints on function definitions
always use absolute imports over relative imports

Also, it might look like I reviewed extremely fast, but I took notes before officially starting the review - much of it was copy/pasting from the notes

etrotta

forgot to actually Submit the review, sorry

etrotta · 2023-10-01T20:57:10Z

expected-indents/src/game.py

+        str: A random uid.
+    """
+    characters = string.ascii_letters + string.digits
+    chars = [secrets.choice(characters) for _ in range(length)]


Why secrets.choice instead of random.choices?

Personally I'd probably just use an uuid.UUID4 instead to be honest

etrotta · 2023-10-01T20:57:47Z

expected-indents/src/game.py

+    def __init__(self, lives=6):
+        self.word = self._get_random_word()
+        # print(self.word)
+        self.total_lives = lives  # const


Unused variable? (total_lives)

etrotta · 2023-10-01T20:58:54Z

expected-indents/src/game.py

+        script_dir = os.path.dirname(os.path.abspath(__file__))
+        file_path = os.path.join(script_dir, 'nouns-clear.txt')


Use pathlib over os.path.
Arguably personal preferences, specially since it should work in any OS using either, but I still recommend pathlib over it

etrotta · 2023-10-01T20:59:16Z

expected-indents/src/game.py

+        """
+        script_dir = os.path.dirname(os.path.abspath(__file__))
+        file_path = os.path.join(script_dir, 'nouns-clear.txt')
+        with open(file_path) as nounfile:


Preferably specify the mode and encoding whenever you're opening a file

The encoding doesn't matters much when all of the text in the file is ASCII, but the default encoding is currently platform dependent, the default encoding for Windows may change in the future, and I personally believe that it's better to explicit state the mode even when using the default ('r')

etrotta · 2023-10-01T21:00:20Z

expected-indents/src/game.py

+
+
+class Game:
+    """game class"""


May as well not have a docstring over just the name of the class. The comments in init could be a bit tidier, but are passable

etrotta · 2023-10-01T21:09:21Z

expected-indents/src/svd.py

+        image = image.resize((self.size, self.size))
+        self.A = np.array(image)
+
+    def reduce(self, terms, type=np.uint8):


avoid using built-in names such as type as a variable name, for example you could use dtype here matching numpy's terminology

etrotta · 2023-10-01T21:09:54Z

expected-indents/src/svd.py

+            type (numpy type, optional): Type of the returned array. Defaults to np.uint8.
+
+        Returns:
+            _type_: _description_


forgot to change this? (line 76)

etrotta · 2023-10-01T21:11:39Z

expected-indents/src/server.py

+    Raises:
+        AssertionError: Game must exist, game_id must be a key the games dictionary.


In multiple docstrings you have "Raises AssetionError", but you do not actually use assert in any of these places?
Side note: overall you might want to avoid using assert (not that you have any in this project rn) and normally raise exceptions instead, in part since assert is skipped if you use optimization command line arguments when running python (-OO), in part so that you and other people using your code can catch them based on their type with except:

etrotta · 2023-10-01T21:13:02Z

expected-indents/src/server.py

+
+app = Flask(__name__)
+
+games = dict()


Avoid using global variables, use Flask's Application Context instead.

Global variables are bug prone overall, specially in multi-threaded contexts
Also, you do not need to use the global keyword to modify global variables, only to overwrite them. Do not use global variables either way though.

etrotta · 2023-10-01T21:33:59Z

expected-indents/README.md

+- Run `src/server.py` using python
+- In your browser, go to [`http://127.0.0.1:5000`](http://127.0.0.1:5000)
+### OR Visit the website:
+[http://picturepuzzlers.pythonanywhere.com/](http://picturepuzzlers.pythonanywhere.com/ "http://picturepuzzlers.pythonanywhere.com/")


No need to use []() as far as I know? Just put it normally

BUSH222 and others added 30 commits August 28, 2023 21:49

Initial commit

6ae4ab3

testcommit

55aa653

Initialize empty source files

f8b8d23

Merge pull request #6 from BUSH222/zike01

4e14ba9

Initialize empty source files

imageprocessor: Image class skeleton

5128881

Import Pillow, change class name to 'Im' and remove get_width(), get_…

1aee09c

…height() methods

Update game.py

0940970

Update game.py

184a734

Update game.py

5196cc9

random word api

547c0f6

Merge pull request #12 from BUSH222/bush-flask

90439a7

random word api

Update game.py

6504ac8

Merge pull request #11 from BUSH222/CodeRulerNo1-patch-6

ac82f95

Update game.py

home_page

13d24f8

Update game.py

454ae2b

made dis into string from list

Merge pull request #14 from BUSH222/CodeRulerNo1-patch-1

8884895

Update game.py

Merge branch 'BUSH222:main' into main

fdecd06

fixed_lint

8ef6462

Merge branch 'main' of https://github.com/PeithonKing/pdcj

8176da7

Merge pull request #13 from PeithonKing/main

470cb9c

home_page

Update game.py

8e26fc3

Update game.py

0f578d9

Update game.py

624a967

Update game.py

e3f43e0

Update game.py

7dd1699

Update game.py

ab23842

Merge pull request #15 from BUSH222/CodeRulerNo1-patch-2

8d3de91

Update game.py

Update game.py

e2f2169

Merge pull request #16 from BUSH222/CodeRulerNo1-patch-3

59c7fcc

Update game.py

Update game.py

42598e0

dhananjaylatkar and others added 27 commits September 9, 2023 21:18

Merge pull request #42 from BUSH222/reqs

24d513c

update dev-requirements

added win & lose features

44fe8b1

Merge pull request #43 from PeithonKing/main

63acc65

added win & lose features

finally complete

88234e8

Merge branch 'main' into SVD

be1acaf

Merge pull request #41 from PeithonKing/SVD

f2dc39c

added SVD and winning and loosing functionality

Minor bug fix, added words.json

fdac1a6

Merge branch 'main' into bush-flask

05b91ab

Merge pull request #44 from BUSH222/bush-flask

cac461c

Updated words.json, minor bug fix in game.py - Game._get_random_word()

requirements update

f7750e2

fixed requirements, removed useless comments

077e1bd

final fixes

911930a

Merge pull request #45 from BUSH222/bush-flask

35424c4

final fixes before welcome image

Docstring update + welcome image

1eb3a7c

Merge pull request #46 from BUSH222/bush-flask

6a2cdee

Docstring update + welcome image

quick readme

22c800b

Merge pull request #47 from BUSH222/bush-flask

f203584

quick readme

README and LICENSE update

b1b1461

newline

658042c

Merge pull request #49 from BUSH222/bush-flask

9ef8e81

README and LICENSE update

Update README.md

e6f1050

Merge pull request #50 from BUSH222/bush-flask-1

1491e05

Update README.md

slight change

070d2fa

Merge pull request #51 from BUSH222/bush-flask

eb2a847

slight change in readme

Update README.md

b9ee059

Presentation

24e2711

Add 'expected-indents/' from commit '24e2711328ebff8ee6528b180e725a76…

685b6d8

…81667954' git-subtree-dir: expected-indents git-subtree-mainline: 4bae50a git-subtree-split: 24e2711

etrotta reviewed Oct 1, 2023

View reviewed changes

etrotta reviewed Oct 10, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expected Indents #3

Expected Indents #3

janine9vn commented Sep 28, 2023

etrotta Oct 1, 2023

etrotta commented Oct 1, 2023 •

edited

Loading

etrotta left a comment

etrotta Oct 1, 2023

etrotta Oct 1, 2023

etrotta Oct 1, 2023

etrotta Oct 1, 2023

etrotta Oct 1, 2023

etrotta Oct 1, 2023

etrotta Oct 1, 2023

etrotta Oct 1, 2023

etrotta Oct 1, 2023

etrotta Oct 1, 2023

		script_dir = os.path.dirname(os.path.abspath(__file__))
		file_path = os.path.join(script_dir, 'nouns-clear.txt')

		Raises:
		AssertionError: Game must exist, game_id must be a key the games dictionary.

Expected Indents #3

Are you sure you want to change the base?

Expected Indents #3

Conversation

janine9vn commented Sep 28, 2023

Choose a reason for hiding this comment

etrotta commented Oct 1, 2023 • edited Loading

etrotta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etrotta commented Oct 1, 2023 •

edited

Loading