Cleanup items.cpp

[vlolteanu]

Marking it as draft for the moment.

The changes were all mechanical, and hence, error-prone when all done at once. An extra pair of eyes would be really needed here.

[vlolteanu]

I'll tick stuff that I've proofed/tested. Feel free to join.

• bc9d3b8 (HEAD -> refactor-items, dev/refactor-items) Cleanup SpawnBoy
• 02486cc Consolidate vendor inventory sorting
• 1a178ce Cleanup {Set,Put}ItemRecord
• ebd7fac Cleanup GetItemRecord
• 02420b4 Cleanup CreateMagicItem & friends
• 7382af1 Cleanup CreateSpellBook
• b80f861 Cleanup RecalcStoreStats
• 5538c94 Cleanup RecreateHealerItem
• 281690b Cleanup SpawnHealer
• af93bda Cleanup SortHealer
• c54bdd0 Cleanup SpawnWitch
• ad41173 Cleanup WitchBookLevel
• b564df8 Cleanup SortWitch
• acc91a6 Cleanup WitchItemOk
• c889be8 Cleanup SpawnPremium
• 4f0d491 Cleanup PremiumItemOk
• 9ca583c Cleanup SpawnSmith
• 88314c3 Fix DoRecharge
• a7ab3b5 Cleanup DoRecharge
• 67777e8 Replace BubbleSwapItem with std:swap
• 0d732fa Consolidate RndFooItem
• 7330ce0 Cleanup SmithItemOk
• 48a3679 Cleanup StoreStatOk
• 5227d74 Cleanup DoOil
• 18c8c42 Cleanup OilItem
• 8372a75 Cleanup GetItemStr
• f20718c Cleanup ProcessItems
• cf5f827 Cleanup SpawnMapOfDoom, SpawnRuneBomb, SpawnTheodore
• a033bbe Cleanup SpawnRewardItem
• 0ee0fdf Cleanup SpawnQuestItem
• d3a0389 Cleanup items_427ABA
• 20f8651 Cleanup items_427A72
• 977e9ed Cleanup CreateTypeItem
• 904eeb7 Cleanup CreateRndUseful
• e8cf1ec Cleanup CreateRndItem
• 21a9c7e Cleanup SetupBaseItem
• ea939ba Cleanup SpawnItem
• e3b1aab Cleanup CheckUnique
• 98e764a Cleanup RndTypeItems
• 8199083 Clean RndAllItems
• b5c044f Cleanup RndUItem
• 60fca38 Cleanup GetItemPower
• 183f922 Cleanup GetStaffPower
• d27098a Cleanup GetSuperItemSpace
• de0e8f2 Cleanup GetItemSpace
• 9d0d12a GetSuperItemLoc cleanup

[vlolteanu]

The vendors' inventories are now sorted using std::sort. The items are sorted by item type, so this change of algorithm may produce a slightly different item order from vanilla.

[AJenbo]

Looks good, but there where a couple of notes mostly related to code style.

[AJenbo]

Looks like the test is failing, several items are morphing. Do you need any help with getting feedback from the test?

[vlolteanu]

Please do. Or better yet, tell me how to run the tests myself. (I set RUN_TESTS to ON in CMakeLists.txt, but that apparently didn't do anything.)

[AJenbo]

If you are on linux or WSL:

apt-get install -y cmake curl g++ git lcov libgtest-dev libfmt-dev libsdl2-dev libsdl2-ttf-dev libsodium-dev
cmake .. -DRUN_TESTS=ON
make -j 4
./devilutionx

I usually create a build-test folder and run it from there.

I don't know what would be needed to do it on VC or Mac

[AJenbo]

Looks like you found the issue 👍 Did you also manage to get the tests running? What system are you developing on?

[vlolteanu]

Yes, it was a mechanical error in RndVendorItem. "-DRUN_TESTS=ON" did the trick.

I'm working on Linux.

[julealgon]

50 commits on a single PR seems excessive to me, and the fact that each commit is just "Cleanup X" is really, really bad imho. We should be specific on what we are doing, like "Use Point struct for X/Y/Z" instead of "Cleanup".

[vlolteanu]

I guess I'll open 50 PRs next time :)

I frankly expect this to get squashed.

I agree that the commits that did make a function use Point could have been worded differently, but it's a current policy to use Point wherever possible, so I'd count that as cleanup. Aside from that, all other Cleanup Foo commits were literally nothing more than cleanups of Foo, no matter how you slice it.

[AJenbo]

I frankly expect this to get squashed.

Oh, guess I don't have to bother with any rebasing then 👍

[AJenbo]

@vlolteanu you got an achievement:

The code now lives up to 3/10 out of ISO 25010 specifications for maintainable code.

[vlolteanu]

Haha that was unexpected.

If I understand that correctly, this PR pushed devilutionX over the edge into 3/10, right?

[AJenbo]

Correct, you removed the last 21 lines of duplication for us to hit 95% unique code. When we forked from Devilution it was at 89%

[AJenbo]

The next mile stone is a lot further in to the future. Most likely it would be reducing the amount of code in functions with many parameters. This clean up did make a small jump on that graph as well, with some code in the 5-6 parameters tier dropping to 3-4 instead, probably due to Point being used instead of an x, y pair. But we will need to do 120x better to tick that box fully.

Splitting up long functions (like what I did for TalkToTowner() and OperateShrine()) is probably what is going to make the biggest advances over all.

I don't know ho much access you guys have to the list, but if you like I can generate issues for some of the functions that are listed as prime candidates for refactoring.

Here's an examples of an issue generated for the 4 basic categories.
Long functions: #1995
Complex functions: #1996
Duplicate code: #1997
Large interfaces: #1998

There are also more advanced lists relating to code coupling. Dealing with that is usually much more complicated and it also tends to get worse as we work on the basic issues and code sharing inevitably leads to more coupling. In my experience dealing with this comes much later when the general issues are mostly death with and you can better start to reason about application structure.