refactor: efficiency of manage constants - #1436
Conversation
There was a problem hiding this comment.
5 issues found across 3 files
Confidence score: 4/5
- In
scripts/scr_equipment_struct/scr_equipment_struct.gml,item_attribute_stringduplicates the stat→format switch already used initem_tooltip_desc_gen, which creates a drift risk where new stats render inconsistently across tooltips—extract a shared formatter or reuse the existing mapping. - In
scripts/scr_equipment_struct/scr_equipment_struct.gml,item_attribute_stringhas missing Feather/JSDoc tags plus unreachablebreakstatements and no explicit unknown-attribute fallback, which weakens maintainability and can produce ambiguous output paths—add@desc/@param/@returns, remove dead code, and return""by default. - In
scripts/scr_ui_manage/scr_ui_manage.gml, switching toset_attribute_stringchanges manage-screen tooltip ordering and DR sign presentation, so players may see different stat emphasis than before—confirm this behavior is intentional and align ordering/format with prior UI expectations if not. - In
scripts/scr_unit_equip_functions/scr_unit_equip_functions.gml,set_attribute_stringcan leave a trailing newline that surfaces as an extra blank line in HP tooltip text—trim the final newline or append separators conditionally.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/scr_unit_equip_functions/scr_unit_equip_functions.gml">
<violation number="1" location="scripts/scr_unit_equip_functions/scr_unit_equip_functions.gml:702">
P3: `set_attribute_string` appends `\n` after every non-empty item string, so when the last present item contributes, the returned string ends with a trailing newline. In the `hp` tooltip (`_hp_tool += _equip_data.set_attribute_string("hp_mod");` in scr_ui_manage) nothing is appended afterward, so the tooltip renders with a trailing blank line. Join with the separator between entries (and skip the round-trip through `_m_string`) instead of appending it after each one.</violation>
</file>
<file name="scripts/scr_ui_manage/scr_ui_manage.gml">
<violation number="1" location="scripts/scr_ui_manage/scr_ui_manage.gml:176">
P3: Replacing the fixed-loop with `set_attribute_string` silently changes the manage-screen tooltips: contributing items are now listed in weapon-1/weapon-2/armour/gear/mobi order instead of armour-first, and positive DR/armour values now render with a '+' prefix (e.g. "5%" becomes "+5%"). If the reorder or the added '+' is not intended, preserve the old slot order and plain formatting; otherwise call out the change explicitly.</violation>
</file>
<file name="scripts/scr_equipment_struct/scr_equipment_struct.gml">
<violation number="1" location="scripts/scr_equipment_struct/scr_equipment_struct.gml:322">
P2: This refactor duplicates the stat->format mapping that already lives in `item_tooltip_desc_gen` in the same constructor. A maintainer adding a new mod stat now has to update both switch statements, which works against the PR's stated goal of making constants easier to maintain. Consider having `item_attribute_string` produce the formatted value only (or drive both from a shared mapping) so new stats are added in one place.</violation>
<violation number="2" location="scripts/scr_equipment_struct/scr_equipment_struct.gml:322">
P2: Custom agent: **Code Quality Review**
The newly added `item_attribute_string` function is missing required JSDoc/Feather tags (`@desc`, `@param`, `@returns`), and the `attribute` parameter is untyped. The Code Quality rule requires JSDoc or Feather tags on every newly added function. Add `/// @desc`, `/// @param {string} attribute`, and `/// @returns {string}` documentation above the function.</violation>
<violation number="3" location="scripts/scr_equipment_struct/scr_equipment_struct.gml:331">
P3: The `break;` after each `return _str;` inside `item_attribute_string` is dead code that can never execute. Remove the unreachable `break;` lines, add `return "";` as a default for unknown attributes, and terminate `var _str = $"{name}: "` with a semicolon so the refactor does not ship sloppy code.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| return item_desc_tooltip; | ||
| }; | ||
|
|
||
| static item_attribute_string = function(attribute){ |
There was a problem hiding this comment.
P2: This refactor duplicates the stat->format mapping that already lives in item_tooltip_desc_gen in the same constructor. A maintainer adding a new mod stat now has to update both switch statements, which works against the PR's stated goal of making constants easier to maintain. Consider having item_attribute_string produce the formatted value only (or drive both from a shared mapping) so new stats are added in one place.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/scr_equipment_struct/scr_equipment_struct.gml, line 322:
<comment>This refactor duplicates the stat->format mapping that already lives in `item_tooltip_desc_gen` in the same constructor. A maintainer adding a new mod stat now has to update both switch statements, which works against the PR's stated goal of making constants easier to maintain. Consider having `item_attribute_string` produce the formatted value only (or drive both from a shared mapping) so new stats are added in one place.</comment>
<file context>
@@ -319,6 +319,33 @@ function EquipmentStruct(item_data = undefined, core_type = "", quality_request
return item_desc_tooltip;
};
+ static item_attribute_string = function(attribute){
+ var _str = $"{name}: "
+ switch(attribute){
</file context>
| static item_attribute_string = function(attribute){ | ||
| var _str = $"{name}: " | ||
| switch(attribute){ | ||
| case "damage_resistance_mod": | ||
| if (damage_resistance_mod == 0){ | ||
| return ""; | ||
| } | ||
| _str += $"{string_format_percentage(damage_resistance_mod)}"; | ||
| return _str; | ||
| break; | ||
| case "hp_mod": | ||
| if (hp_mod == 0){ | ||
| return ""; | ||
| } | ||
| _str += $"{string_format_percentage(hp_mod)}"; |
There was a problem hiding this comment.
P2: Custom agent: Code Quality Review
The newly added item_attribute_string function is missing required JSDoc/Feather tags (@desc, @param, @returns), and the attribute parameter is untyped. The Code Quality rule requires JSDoc or Feather tags on every newly added function. Add /// @desc, /// @param {string} attribute, and /// @returns {string} documentation above the function.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/scr_equipment_struct/scr_equipment_struct.gml, line 322:
<comment>The newly added `item_attribute_string` function is missing required JSDoc/Feather tags (`@desc`, `@param`, `@returns`), and the `attribute` parameter is untyped. The Code Quality rule requires JSDoc or Feather tags on every newly added function. Add `/// @desc`, `/// @param {string} attribute`, and `/// @returns {string}` documentation above the function.</comment>
<file context>
@@ -319,6 +319,33 @@ function EquipmentStruct(item_data = undefined, core_type = "", quality_request
return item_desc_tooltip;
};
+ static item_attribute_string = function(attribute){
+ var _str = $"{name}: "
+ switch(attribute){
</file context>
| static item_attribute_string = function(attribute){ | |
| var _str = $"{name}: " | |
| switch(attribute){ | |
| case "damage_resistance_mod": | |
| if (damage_resistance_mod == 0){ | |
| return ""; | |
| } | |
| _str += $"{string_format_percentage(damage_resistance_mod)}"; | |
| return _str; | |
| break; | |
| case "hp_mod": | |
| if (hp_mod == 0){ | |
| return ""; | |
| } | |
| _str += $"{string_format_percentage(hp_mod)}"; | |
| /// @desc Returns a formatted attribute string for the item | |
| /// @param {string} attribute The attribute key to format (e.g. "hp_mod", "damage_resistance_mod", "armour_value") | |
| /// @returns {string} The formatted attribute string, or empty string if the attribute value is 0 | |
| static item_attribute_string = function(attribute){ | |
| var _str = $"{name}: " | |
| switch(attribute){ | |
| case "damage_resistance_mod": | |
| if (damage_resistance_mod == 0){ | |
| return ""; | |
| } | |
| _str += $"{string_format_percentage(damage_resistance_mod)}"; | |
| return _str; | |
| break; | |
| case "hp_mod": | |
| if (hp_mod == 0){ | |
| return ""; | |
| } | |
| _str += $"{string_format_percentage(hp_mod)}"; | |
| return _str; | |
| break; | |
| case "armour_value": | |
| if (armour_value == 0){ | |
| return ""; | |
| } | |
| _str += $"{format_number_with_sign(armour_value)}"; | |
| return _str; | |
| break; | |
| } | |
| } |
| var _str = ""; | ||
| for (var i = 0; i < array_length(present_items); i++){ | ||
| var _item = equipment[$ present_items[i]]; | ||
| var _m_string = _item.item_attribute_string(attribute); |
There was a problem hiding this comment.
P3: set_attribute_string appends \n after every non-empty item string, so when the last present item contributes, the returned string ends with a trailing newline. In the hp tooltip (_hp_tool += _equip_data.set_attribute_string("hp_mod"); in scr_ui_manage) nothing is appended afterward, so the tooltip renders with a trailing blank line. Join with the separator between entries (and skip the round-trip through _m_string) instead of appending it after each one.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/scr_unit_equip_functions/scr_unit_equip_functions.gml, line 702:
<comment>`set_attribute_string` appends `\n` after every non-empty item string, so when the last present item contributes, the returned string ends with a trailing newline. In the `hp` tooltip (`_hp_tool += _equip_data.set_attribute_string("hp_mod");` in scr_ui_manage) nothing is appended afterward, so the tooltip renders with a trailing blank line. Join with the separator between entries (and skip the round-trip through `_m_string`) instead of appending it after each one.</comment>
<file context>
@@ -695,6 +695,16 @@ function UnitEquipment(equipment_set, _unit = noone) constructor {
+ var _str = "";
+ for (var i = 0; i < array_length(present_items); i++){
+ var _item = equipment[$ present_items[i]];
+ var _m_string = _item.item_attribute_string(attribute);
+ _str += _m_string != "" ? _m_string + "\n" : "";
+ }
</file context>
| _res_tool += $"{name}: {dr}%\n"; | ||
| } | ||
| } | ||
| _res_tool += _equip_data.set_attribute_string("damage_resistance_mod"); |
There was a problem hiding this comment.
P3: Replacing the fixed-loop with set_attribute_string silently changes the manage-screen tooltips: contributing items are now listed in weapon-1/weapon-2/armour/gear/mobi order instead of armour-first, and positive DR/armour values now render with a '+' prefix (e.g. "5%" becomes "+5%"). If the reorder or the added '+' is not intended, preserve the old slot order and plain formatting; otherwise call out the change explicitly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/scr_ui_manage/scr_ui_manage.gml, line 176:
<comment>Replacing the fixed-loop with `set_attribute_string` silently changes the manage-screen tooltips: contributing items are now listed in weapon-1/weapon-2/armour/gear/mobi order instead of armour-first, and positive DR/armour values now render with a '+' prefix (e.g. "5%" becomes "+5%"). If the reorder or the added '+' is not intended, preserve the old slot order and plain formatting; otherwise call out the change explicitly.</comment>
<file context>
@@ -173,44 +173,7 @@ function reset_manage_unit_constants(unit) {
- _res_tool += $"{name}: {dr}%\n";
- }
- }
+ _res_tool += _equip_data.set_attribute_string("damage_resistance_mod");
_res_tool += $"CON: {round(unit.constitution / 2)}%";
</file context>
| break; | ||
| case "hp_mod": | ||
| if (hp_mod == 0){ | ||
| return ""; | ||
| } | ||
| _str += $"{string_format_percentage(hp_mod)}"; | ||
| return _str; | ||
| break; | ||
| case "armour_value": | ||
| if (armour_value == 0){ | ||
| return ""; | ||
| } | ||
| _str += $"{format_number_with_sign(armour_value)}"; | ||
| return _str; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| static special_value = function(special) { | ||
| if (is_struct(specials)) { | ||
| var _specials = struct_get_names(specials); |
There was a problem hiding this comment.
P3: The break; after each return _str; inside item_attribute_string is dead code that can never execute. Remove the unreachable break; lines, add return ""; as a default for unknown attributes, and terminate var _str = $"{name}: " with a semicolon so the refactor does not ship sloppy code.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/scr_equipment_struct/scr_equipment_struct.gml, line 331:
<comment>The `break;` after each `return _str;` inside `item_attribute_string` is dead code that can never execute. Remove the unreachable `break;` lines, add `return "";` as a default for unknown attributes, and terminate `var _str = $"{name}: "` with a semicolon so the refactor does not ship sloppy code.</comment>
<file context>
@@ -319,6 +319,33 @@ function EquipmentStruct(item_data = undefined, core_type = "", quality_request
+ }
+ _str += $"{string_format_percentage(damage_resistance_mod)}";
+ return _str;
+ break;
+ case "hp_mod":
+ if (hp_mod == 0){
</file context>
Summary by cubic
Consolidates and speeds up unit manage constants tooltip generation by moving equipment-attribute aggregation into shared helpers. Previously,
scr_ui_managebuilt DR/HP/Armor lines with per-slot switches; now it calls_equip_data.set_attribute_string(attr)which iteratespresent_itemsand uses each item’sitem_attribute_string(attr). Percentages for HP and Damage Resistance now usestring_format_percentagefor consistent display; zero-value attributes remain omitted.EquipmentStruct.item_attribute_string(attribute)returning "Name: value" ("" when 0) fordamage_resistance_mod,hp_mod, andarmour_value.UnitEquipment.set_attribute_string(attribute)to aggregate lines across equipped items.reset_manage_unit_constantswith_equip_data.set_attribute_string("damage_resistance_mod"|"hp_mod"|"armour_value").string_format_percentageforhp_mod,damage_resistance_mod,ranged_mod, andmelee_mod.Review notes
present_itemsincludes all slots that can affect these attributes and that the display order is acceptable.Written for commit 80b6af9. Summary will update on new commits.