refactoring week - Poobslag/turbofat GitHub Wiki
Refactoring week occurs every two months. It's good for code to have time to rest, where we can clean up technical debt.
Refactoring backlog
- Source code is in
/puzzle/levelbut assets are in/puzzle/levels. Does this make sense? Why should one be pluralized? test-level-settings-upgrader.gdshould contain the backwards compatibility tests currently intest-level-settings.gd- CareerRegion could use a utility method like
all_level_ids()which includes the boss level and intro level, i think a few places could use it. - Redesign 'RankCriteria' to be less crazy -- it currently calls to 'copy_from', 'fill_missing_thresholds' and 'calculate_rank' just for common use cases.
- LevelHistory.level_names() definitely returns level IDs, and a few places who call it have variables which should be something like 'level_id' instead too. Levels have names, but level history doesn't care about those
- Some
.wavfiles are in/src. We should detect this in our delint script - We use "BossLevelButtonScene" in some places and "BossLevelSelectButtonScene" in others, we should be consistent
- A few shaders are indented with spaces instead of tabs.
- "Career" mode should technically be called "Adventure" mode now... Does that affect all our paths and constants?
- We use the terms "boss level" and "goal" somewhat inconsistently, and progress-board-spots.gd even refers to a "finish". We should choose a consistent term, or at least rethink when we use these two terms.
level-rank-achievement.gdshould userank_meets_gradeinstead ofrequired_rank_for_gradeto reduce the amount of code needed.- Improve the method by which the 'dll' and 'so' files are included in your steam build. Right now they're expected to pre-exist in the /export/ folder which is strange.
- FreeRoamUi's SettingsButton can connect to
SettingsMenu._on_Settings_pressedrather than needing a second redundantFreeRoamUi._on_SettingsButton_pressedmethod preset-keybind-row.gdshould use onready instead of referring to$Valuein its methods- PuzzleDemo prints a warning message about "level doesn't define rank criteria"; this is referring to the blank level which PuzzleDemo defaults to.
results-hud-blueprint.gdcalculates goals in advance, but there's no need anymore. It used to calculate them in advance because the rank calculation process was slow.value: true->value: (bool) true- Instead of "SplashScreen" and "MainMenu" needing to edit SystemButtons's children to hack in their own quit text, SystemButtons should let them specify something similar to SettingsMenu's
quit_typeparameter: "main menu" or "quit" - Our project version is in the
.pottranslation templates. We should automate this. - A lot of the assertions in
test-level-settings.gdare backwards:assert_eq(0, settings.rank.rank_criteria.thresholds_by_grade.size()) - If ResultsHud just tweened its y coordinate instead of its position, we would have avoided a few small bugs when rearranging its horizontal position.
- Extract a method from
training_menu.gdto encompass all of the things you do when a level is loaded. We didn't refresh the level description which caused a bug. - We have the 'recalculate ranks on startup' logic disabled for tests because it's too slow, but ideally it would just be faster. Maybe we could cache all of the level settings in memory or something like that.
- 'career_mode_finish' is lower-case in our SteamAchievements.tscn definitions. The rest are all upper-case.
- We've had a few bugs stem from
yield(get_tree(), "idle_frame")calls. Some of these were resolved by swapping over tocall_deferred, if it matters, so maybe we should just change all of them. - The words "Spanish" and "English" keep hopping around in localizables-extracted.py. Can we sort them?
- The steam
prepare_level_listenersandremove_listener_listenerswould be more intuitively namedconnect_level_listeners/disconnect_level_listeners - Every steam achievement calls
SteamUtils.is_achievement_achieved(achievement_id), this could be streamlined with anis_achieved()utility method. - Simplify the steam delivery process. Currently it involves 4-5 steps. It might be nice if the 'package' process created a 'steam-content' folder with filenames and contents exactly mirroring the
steamworks-sdk\tools\ContentBuilder\content\folder. - ResourceCache.substitute_singletons() has some heavily nested logic which could be pulled out until utility methods, like
substitute_singletonandinitialize_singleton settings-keybinds-control.gdshould initialize its child controls asonreadyinstead of repeatedly calling get_node- Change 'SettingsDialogs.confirm_new_save_slot()
toSettingsDialogs.show_new_save_slot_confirmation()`. The verb "confirm" is ambiguous, since it could mean the player has already confirmed it, or that we're prompting the player to confirm it. - More places can use the constants such as
SettingsMenu.QUIT_TO_DESKTOPinstead ofSettingsMenu.QuitType.QUIT_TO_DESKTOP - Update
pybabel-extract.shautomation to preserveneeds-reviewflags. career-data.reset()assignments are in a random order; they should be alphabetized like other places- alphabetize
show_difficulty_menu,show_give_up_confirmation,show_adventure_mode_introin all contexts. Thereenable-tips.gdscript has them in a different order - Warning
...does not define 'M' rank criteriashows up when giving up on a level. Suppress this warning in some way -- ideally not by squelching the message, but by changing the code's behavior so that the criteria is not calculated, or is calculated based on the level before it's cleared - If one of your save slots has an old file, PlayerSave will keep upgrading it every time a new scene is loaded. It should only need to do this upgrade once (and it should save the results)
PlayerSave.save_scheduledis out of order (public fields should be before private)- Some code depends on the puzzle tile map cell dimensions of (72, 64) and they currently hardcode it. Extract a constant for
72, 64dimensions that are used byspear.gdandtomato.gdfor example. CellCritterManager's class name does not match its filename,critter-manager.gdallele-buttons.gdandfatness-changer.gdboth define a_find_operation_buttonmethod.- Whitespace in
puzzle-trace.gd:PieceManager=should bePieceManager = - The header comment for
global.gdis insufficient, it does more than "preserving state when loading different scenes". - Use Godot's built-in "ResourcePreloader" instead of rolling our own.
- SystemSaveUpgrader methods should be static, so that people don't have to call
SystemSaveUpgrader.new().new_save_item_upgrader()