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

__init__.py Suggested refactors. #1

Closed
wants to merge 7 commits into from

Conversation

ThePhar
Copy link

@ThePhar ThePhar commented Jul 1, 2023

PR might say a lot of files changes, but that's mainly cause I'm current with main on my branch, recommend merging ArchipelagoMW/main into Daivuk/doom to resolve that. Only actual changes are in the doom_1997/__init__.py file. There should be no actual logic changes, just a refactor.

Quick link to file: https://github.com/Daivuk/Archipelago/blob/0d021fd8f5750ba91fec06a35c892cd112015988/worlds/doom_1993/__init__.py

Here's my comments on why I did what I did per line, (some are nit picky and if you disagree you're fine to ignore those (e.g. range(3) instead of range(0, 3)):

(probably want word wrap off for this)

ORIG    MINE    COMMENT
========================================================================================================================
1-10    1-7     Optimizations and re-ordering of imports (my IDE does re-ordering, it's nice)

37      34-35   Reformat to fit within 120 characters per line, per AP style guide.

51-54   49-52   Add space and change parens to brackets for type hint. 
                Change `'` to `"` for consitency and match AP style guides for quotes.

58      56      Add space and change parens to brackets for type hint.

65      -       Remove blank line.

73      -       Remove blank line. PEP8 recommends 1 blank space between class method definitions.

77      -       Removed as an optimization can be made later in `create_items`.

80      -       Remove blank line.

83-88   77      Can use `functools.reducer` function to simplify this method.

92-93   81-82   Can remove initial starting value for range, implicitly starts at 0.
                Change string concat and str cast to format string.

99      -       Remove blank line.

102-103 90-93   Create the regions and save them to local variables for later use. Saves needing to lookup the regions 
                    again later.

105     95      Just reworded comment.

107-126 97-110  Simplify region, location, and entrance logic by using AP helper methods on `Region` to handle it behind
                    the scenes.
                We can also sum the locations all at once at the end instead of doing it iteratively.
                Use a dict comprehension for location creation to speed up creating locations. 
                Do everything we need in the first `for region_name in Region.regions` loop to optimize.

128-139 -       No need for this method since we're using `Region.add_exits` method.

142     112     Add type hint 

144     114     Move `not` around to match PEP8 guidelines.

153     -       Remove blank line.

157     126-130 Move `allow_death_logic` check here as it makes most sense in `set_rules`, imo.
                `set_rules` runs before the generator marks locations as excluded, so we want to do it no later than 
                    here anyway.
                Use `self.multiworld.exclude_locations` rather than setting `Location.progress_type`. Speeds up 
                    generation.

161-162 134     Don't need to store in local value if we're gonna return immediately, saves a line.

164-169 -       Remove `create_item_with_classification` since every time we call it, we end up pulling the 
                    classification from the item_table anyway. May as well just use `create_item` as it will do it on 
                    its own.
                Remove blank line.

176     -       Remove blank line.

178     145-146 Add type hint for `is_only_first_episode` since IDE thinks it could be an `int` too.
                Store itempool locally until we're done filling it. Allows us to optimize this method later and remove 
                    `self.item_count`.

184-196 152-160 Some line breaks just help me make it easier to read, but it's personal preference.
                Instead of checking `name == item1 and name == item2`, we can simplify it with `name in {item1, item2}`
                Move some conditionals around since we end up ignoring adding them to itempool if they're met anyway.
                Determine count with an if else inline.
                Add to itempool here using a list comprehension for optimization.

200     164     Move `not around to match PEP8 guidelines.

205-206 170-171 Use a `create_event` method since all events are progression to simplify line.

233-238 198-203 Pass `itempool` into function to allow method to add to itempool. (could also make itempool a class 
                        function, i didn't though)

240     -       Removed this variable as it's not needed later.

241-247 -       Remove filler list and move to `get_filler_item_name` method.

248-252 205-209 Can simplify filling remaining slots by just checking the length of our `itempool` against 
                    `self.location_count`.
                Uses `get_filler_item_name` to generate filler item names.
                Finally add our `itempool` into the `self.multiworld.itempool`

-       211-218 Create `get_filler_item_name` for generating a filler item. Needed for `item_links` that use 
                    `replacement_item: null` to generate a random filler item.
                Not using `create_item_with_classification` because all those items are already coded as "filler" in 
                    item table.

255-265 220-231 Simplify logic a bit and move comment to above as it goes over 120 char limit per style guide.
                Add `return` statement in conditional so we don't need to actually do the next line.

269-275 234     Since you're just putting all options in `slot_data`, can simplify with dict comprehension.

Going to post my review of the other files in the original PR.

@Daivuk
Copy link
Owner

Daivuk commented Jul 8, 2023

Integrated

@Daivuk Daivuk closed this Jul 8, 2023
@ThePhar ThePhar deleted the doom-adjustments branch October 25, 2023 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants