Rename stores.cpp global variables #7425

kphoenix137 · 2024-09-20T19:24:38Z

Precursor to #7424 to make review of this linked PR easier.

StephenCWills · 2024-09-20T20:00:30Z

I see that the time demo is failing, but I think it's failing as a result of #7417 being merged.

Timedemo isn't failing on master.

StephenCWills · 2024-09-20T20:07:29Z

Source/stores.h

 /** Currently active store */
-extern TalkID stextflag;
+extern TalkID activeStore;

-/** Current index into storehidx/storehold */
-extern DVL_API_FOR_TEST int storenumh;
+/** Current index into playerItemIndexes/playerItem */
+extern DVL_API_FOR_TEST int currentItemIndex;
 /** Map of inventory items being presented in the store */
-extern int8_t storehidx[48];
+extern int8_t playerItemIndexes[48];
 /** Copies of the players items as presented in the store */
-extern DVL_API_FOR_TEST Item storehold[48];
+extern DVL_API_FOR_TEST Item playerItem[48];

 /** Items sold by Griswold */
-extern Item smithitem[SMITH_ITEMS];
+extern Item smithItem[SMITH_ITEMS];
 /** Number of premium items for sale by Griswold */
-extern int numpremium;
+extern int numPremiumItems;
 /** Base level of current premium items sold by Griswold */
-extern int premiumlevel;
+extern int premiumItemLevel;
 /** Premium items sold by Griswold */
-extern Item premiumitems[SMITH_PREMIUM_ITEMS];
+extern Item premiumItem[SMITH_PREMIUM_ITEMS];

 /** Items sold by Pepin */
-extern Item healitem[20];
+extern Item healerItem[20];

 /** Items sold by Adria */
-extern Item witchitem[WITCH_ITEMS];
+extern Item witchItem[WITCH_ITEMS];

 /** Current level of the item sold by Wirt */
-extern int boylevel;
+extern int boyItemLevel;
 /** Current item sold by Wirt */
-extern Item boyitem;
+extern Item boyItem;


We should switch to PascalCase, and collections should be plural. You might also want to use, for example, PremiumItemCount instead of NumPremiumItems as that matches the convention for the ActiveMonsters array.

I opted to not use PascalCase because these variables will be class members in the next PR I make after this one gets merged, which will create more diffs. I guess that's fine as long as there are fewer diffs before it reaches #7424

What is the goal of turning these into class variables? If you are making a Store class, then the variables should be named more generically, like Store::items and Store::itemCount. If you are just throwing all of these variables into a class to group them for no reason, I think it would be better if you didn't.

Plus, regardless, it will create more diffs because you will have to reference variables by store.witchItems[i] instead of just witchItems[i].

EDIT: I just looked at #7424, and I guess you did just throw everything into a StoreSession class. I don't really understand what benefit that provides.

What is the goal of turning these into class variables? If you are making a Store class, then the variables should be named more generically, like Store::items and Store::itemCount. If you are just throwing all of these variables into a class to group them for no reason, I think it would be better if you didn't.

Plus, regardless, it will create more diffs because you will have to reference variables by store.witchItems[i] instead of just witchItems[i].

The main idea behind moving these variables into a class is to improve organization and maintainability by grouping the store related variables together, instead of just leaving them as separate globals. This approach makes it pretty obvious that these variables are meant for use with store related code. From what I learned about C++, encapsulation through classes is considered good OOP practice, and this seemed like an excellent candidate for it. So by keeping related functions and variables together, I believe that it would not only follow best practices but also make the codebase more scalable and easier to manage, especially for when we get the lua platform established.

It definitely adds more diffs, but I would see it as an initial pain for a more long term payoff. I think the benefits of improved code clarity, reduced risk of bugs and easier maintenance will outweigh the inconvenience of a larger PR

Additionally, I haven't thoroughly examined all of the store variables and their usage yet, so it's entirely possible that the member variables of my suggested StoreSession class can be entirely overhauled. I still have a lot of work to do yet to make the code more cohesive and well-structured.

EDIT: I just looked at #7424, and I guess you did just throw everything into a StoreSession class. I don't really understand what benefit that provides.

Yes, it's a draft :')

Opening this back up so the conversation is visible.

The main idea behind moving these variables into a class is to improve organization and maintainability by grouping the store related variables together, instead of just leaving them as separate globals.

From my perspective they are already grouped together in the store.h header. The main thing you gain from a struct or class grouping like this is the ability to pass the shops around as a parameter into the functions that need it instead of just referencing it as a global. Whether that's a useful idea or not typically depends on whether you're going to have multiple instances of the class or struct. It may also be useful if you want to mock the class or struct for testing.

... especially for when we get the lua platform established.

I have my doubts. It seems to me that you want to be able to just kind of stick objects from the codebase into Lua, but I personally think the Lua API should be more carefully crafted than that, particularly so we have the option to make things user-friendly for modders. The PR you made where you tried to model the whole Player struct in Lua made me feel even more strongly that we shouldn't just be exposing C++ structs and classes as-is.

Exposing things in Lua means locking it down and not allowing any changes to it in the future, so we should do it carefully, better only a few things and then expand as a need arises.

And yeah objects are really best for representing things that there can be more then one instance of.

kphoenix137 · 2024-09-20T21:04:24Z

I see that the time demo is failing, but I think it's failing as a result of #7417 being merged.

Timedemo isn't failing on master.

I saw it fail running the tests locally. Not sure why it's not failing on master.

AJenbo · 2024-09-20T21:40:15Z

Maybe you can PascalCase the variables that shouldn't end up in a class and fix the plural forms of containers :)

Source/DiabloUI/credits.cpp

Source/items.cpp

AJenbo · 2024-09-20T21:50:35Z

Source/stores.cpp

 		item.clear();
 	}

 	const Player &myPlayer = *MyPlayer;

 	for (int8_t i = 0; i < myPlayer._pNumInv; i++) {
-		if (storenumh >= 48)
+		if (CurrentItemIndex >= 48)


Reminder to self, this is how fare I am in the review.

kphoenix137 · 2024-09-20T21:56:36Z

Maybe you can PascalCase the variables that shouldn't end up in a class and fix the plural forms of containers :)

I can just correct the variable cases in the next PR I open, although it might not be productive to open a PR with just the class addition itself since that would involve a mass copy and paste of things into the class, which would put the codebase in a gross state if the refactor isn't merged soon after. I think this PR itself might be enough to make reviewing the refactor easier.

AJenbo · 2024-09-20T22:08:29Z

you already did it, as fare as i can see :)

Rename stores.cpp global variables

321e738

kphoenix137 force-pushed the rename-stores-globals branch from a86a366 to 321e738 Compare September 20, 2024 19:28

StephenCWills reviewed Sep 20, 2024

View reviewed changes

PascalCase

447fc0c

AJenbo reviewed Sep 20, 2024

View reviewed changes

Source/DiabloUI/credits.cpp Outdated Show resolved Hide resolved

Fix mistake

9816f47

AJenbo reviewed Sep 20, 2024

View reviewed changes

Source/DiabloUI/credits.cpp Outdated Show resolved Hide resolved

Fix mistake 2

ba39efa

AJenbo reviewed Sep 20, 2024

View reviewed changes

Source/items.cpp Outdated Show resolved Hide resolved

kphoenix137 added 2 commits September 20, 2024 17:48

Fix error 3

3cd94bc

Fix error 4

b96c55a

AJenbo reviewed Sep 20, 2024

View reviewed changes

AJenbo approved these changes Sep 20, 2024

View reviewed changes

AJenbo merged commit e90855b into diasurgical:master Sep 20, 2024
23 checks passed

kphoenix137 deleted the rename-stores-globals branch September 20, 2024 23:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename stores.cpp global variables #7425

Rename stores.cpp global variables #7425

kphoenix137 commented Sep 20, 2024 •

edited

Loading

StephenCWills commented Sep 20, 2024

StephenCWills Sep 20, 2024

kphoenix137 Sep 20, 2024

StephenCWills Sep 20, 2024 •

edited

Loading

kphoenix137 Sep 20, 2024

kphoenix137 Sep 20, 2024

kphoenix137 Sep 20, 2024

kphoenix137 Sep 20, 2024

StephenCWills Sep 21, 2024

AJenbo Sep 21, 2024

kphoenix137 commented Sep 20, 2024

AJenbo commented Sep 20, 2024

AJenbo Sep 20, 2024

kphoenix137 commented Sep 20, 2024

AJenbo commented Sep 20, 2024

Rename stores.cpp global variables #7425

Rename stores.cpp global variables #7425

Conversation

kphoenix137 commented Sep 20, 2024 • edited Loading

StephenCWills commented Sep 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenCWills Sep 20, 2024 • edited Loading

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

kphoenix137 commented Sep 20, 2024

AJenbo commented Sep 20, 2024

Choose a reason for hiding this comment

kphoenix137 commented Sep 20, 2024

AJenbo commented Sep 20, 2024

kphoenix137 commented Sep 20, 2024 •

edited

Loading

StephenCWills Sep 20, 2024 •

edited

Loading