Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

General refactor of MTA's code #3158

Closed

Conversation

TracerDS
Copy link
Contributor

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

@CrosRoad95
Copy link
Contributor

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

@TracerDS
Copy link
Contributor Author

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

@tederis
Copy link
Member

tederis commented Aug 16, 2023

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()).

@Pirulax
Copy link
Contributor

Pirulax commented Aug 22, 2023

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.

Copy link
Contributor

@Pirulax Pirulax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is too big of a PR....
And I've been reviewing it for a good 20-30 minutes

Shared/XML/CXMLNodeImpl.cpp Outdated Show resolved Hide resolved
Shared/XML/CXMLNodeImpl.cpp Outdated Show resolved Hide resolved
Client/core/CSettings.cpp Outdated Show resolved Hide resolved
Server/core/CCrashHandler.cpp Show resolved Hide resolved
Server/core/CCrashHandler.cpp Outdated Show resolved Hide resolved
Server/core/CDynamicLibrary.cpp Outdated Show resolved Hide resolved
Server/core/CDynamicLibraryInterface.h Outdated Show resolved Hide resolved
Server/core/CServerImpl.cpp Outdated Show resolved Hide resolved
Server/core/CServerImpl.cpp Outdated Show resolved Hide resolved
@github-actions github-actions bot added the stale Inactive for over 90 days, to be closed label Apr 23, 2024
Copy link
Contributor

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.

Copy link
Contributor

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

@github-actions github-actions bot closed this May 23, 2024
@TracerDS TracerDS deleted the 140823_RefactorServerCode branch August 7, 2024 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor stale Inactive for over 90 days, to be closed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants