Skip to content

Conversation

@MoonPadUSer
Copy link
Contributor

Basically just doing a little bit of refactoring by getting rid of all loop macros in src/engine/menus.cpp

@MoonPadUSer MoonPadUSer requested review from a user and TheAssassin July 28, 2020 18:38
@robalni
Copy link
Contributor

robalni commented Jul 28, 2020

You're using a different code style, '{' on same line and space after if and for.

@MoonPadUSer
Copy link
Contributor Author

@robalni Indeed, I do. Because:

for (int i = 0; i < 2; i++) {
    printf("This is fine\n");
}

saves space and is just as easy to read as

for (int i = 0; i < 2; i++)
{
    printf("This is great too!\n");
}

I only like to do that when there's only one line inside the curly brackets (I hope I didn't violate that rule in here)

@robalni
Copy link
Contributor

robalni commented Jul 28, 2020

I prefer to have everything the same, otherwise it will look like a mess after a while.

@MoonPadUSer
Copy link
Contributor Author

Hum, ok, can't hurt. :) will change it

@MoonPadUSer
Copy link
Contributor Author

alright, now it should be good.

@blue-nebula blue-nebula locked and limited conversation to collaborators Jul 28, 2020
@blue-nebula blue-nebula unlocked this conversation Jul 28, 2020
g.text("the following settings have changed:");
g.pushfont("little");
loopv(needsapply) g.text(needsapply[i].desc, 0xFFFFFF, "point");
for (int i = 0; i < needsapply.length(); i++)
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest not to just stubbornly replace all occurrences of the macro, but just go for the more modern iterator-based loops where applicable. How about

Suggested change
for (int i = 0; i < needsapply.length(); i++)
for (const auto& i : needsapply)

If you can not use this approach, please don't use the deprecated length() but the STL-compatible size().

Applies to pretty much all loops you modified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'll do that

@TheAssassin TheAssassin added this to the 1.7.0 milestone Dec 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants