mtasa-blue icon indicating copy to clipboard operation
mtasa-blue copied to clipboard

General refactor of MTA's code

Open TracerDS opened this issue 2 years ago • 5 comments

I changed most of #ifdef WIN32 to #ifdef _WIN32 because WIN32 was not always defined in some files. _WIN32 however is always active

TracerDS avatar Aug 14 '23 16:08 TracerDS

i took a look at what did you do and i have few things to say:

-    reply << (unsigned char)(temp.str().length() + 1);
+   reply << (uchar)(temp.str().length() + 1);

we should probably use types from https://en.cppreference.com/w/cpp/types/integer if we are going to refactor something, so in this case uchar should be uint8_t

I'm not good at windows headers so i have a bit concern about changes you have made regarding that

If function has depth of 1, maybe 2 levels we probably should keep it as is.

Even if you compiled it, and it works, you should probably try to run some bigger gamemode to test more scenarious

I think if you are doing such big refactor, split it into smaller parts, sections or however you name them, so in one pr you refactor all types like "unsigned short" to "ushort", in the next you decreasing depth of functions ect. it's a general good advice so reviewers have easier time

CrosRoad95 avatar Aug 14 '23 17:08 CrosRoad95

i took a look at what did you do and i have few things to say:

-    reply << (unsigned char)(temp.str().length() + 1);
+   reply << (uchar)(temp.str().length() + 1);

we should probably use types from https://en.cppreference.com/w/cpp/types/integer if we are going to refactor something, so in this case uchar should be uint8_t

I'm not good at windows headers so i have a bit concern about changes you have made regarding that

If function has depth of 1, maybe 2 levels we probably should keep it as is.

Even if you compiled it, and it works, you should probably try to run some bigger gamemode to test more scenarious

I think if you are doing such big refactor, split it into smaller parts, sections or however you name them, so in one pr you refactor all types like "unsigned short" to "ushort", in the next you decreasing depth of functions ect. it's a general good advice so reviewers have easier time

well, then just a quick change from all utype to u/int8-64 would fix that

TracerDS avatar Aug 14 '23 17:08 TracerDS

The derefencing such as for (const auto& pWater : *g_pGame->GetWaterManager()) is counterintuitive and makes it difficult to read the code. It's better to add a dedicated method for getting inner objects like for (const auto& pWater : g_pGame->GetWaterManager()->GetWaters()).

tederis avatar Aug 16 '23 15:08 tederis

The derefencing such as for (const auto& pWater : *g_pGame->GetWaterManager()) is counterintuitive and makes it difficult to read the code. It's better to add a dedicated method for getting inner objects like for (const auto& pWater : g_pGame->GetWaterManager()->GetWaters()).

I agree on this one, it's not clear 100% clear what *g_pGame->GetWaterManager() is going to be.

Pirulax avatar Aug 22 '23 00:08 Pirulax

This draft pull request is stale because it has been open for at least 90 days with no activity. Please continue on your draft pull request or it will be closed in 30 days automatically.

github-actions[bot] avatar Apr 23 '24 01:04 github-actions[bot]

This draft pull request was closed because it has been marked stale for 30 days with no activity.

github-actions[bot] avatar May 23 '24 01:05 github-actions[bot]