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

Remove Global Declarations from Movie #4088

Merged
merged 1 commit into from Aug 5, 2016

Conversation

RisingFog
Copy link
Member

@RisingFog RisingFog commented Aug 4, 2016

Removes all global declarations from Movie, and replaces them with getters and setters.


This change is Reviewable

@RisingFog RisingFog force-pushed the remove_globals_movie branch 3 times, most recently from 3fa4951 to ca3b6a7 Compare August 4, 2016 17:13
@lioncash
Copy link
Member

lioncash commented Aug 4, 2016

Review status: 0 of 10 files reviewed at latest revision, 2 unresolved discussions.


Source/Core/Core/Movie.cpp, line 413 [r1] (raw file):

}

void SetClearSave(bool bEnabled)

the parameters can just be regularly snake-cased like enabled, disc_changed, etc.


Source/Core/Core/Movie.cpp, line 433 [r1] (raw file):

}

void SetDiscChange(std::string sDiscChange)

This should probably take the string by const reference.

The naming also seems somewhat ambiguous here (not like the variables weren't, really). Considering there's already a function named SetChangedDisc

You may want to combine these into one and make it just do something like

void SignalDiscChange(const std::string& new_disc_filename)
{
  s_bDiscChange = true;
  s_discChange = new_disc_filename;
}

Since they're only ever used in conjunction with one another.


Comments from Reviewable

@RisingFog RisingFog force-pushed the remove_globals_movie branch 2 times, most recently from ef2b5b6 to 396fdaf Compare August 4, 2016 19:20
@RisingFog
Copy link
Member Author

Review status: 0 of 10 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.


Source/Core/Core/Movie.cpp, line 413 [r1] (raw file):

Previously, lioncash (Mat M.) wrote…

the parameters can just be regularly snake-cased like enabled, disc_changed, etc.

Done.

Source/Core/Core/Movie.cpp, line 433 [r1] (raw file):

Previously, lioncash (Mat M.) wrote…

This should probably take the string by const reference.

The naming also seems somewhat ambiguous here (not like the variables weren't, really). Considering there's already a function named SetChangedDisc

You may want to combine these into one and make it just do something like

void SignalDiscChange(const std::string& new_disc_filename)
{
  s_bDiscChange = true;
  s_discChange = new_disc_filename;
}

Since they're only ever used in conjunction with one another.

Done.

Comments from Reviewable

@lioncash
Copy link
Member

lioncash commented Aug 4, 2016

Source/Core/Core/Movie.cpp, line 421 [r3] (raw file):

{
  s_discChange = new_disc_filename;
  s_bDiscChange = true;

There's tabs instead of spaces here.


Comments from Reviewable

@RisingFog
Copy link
Member Author

Source/Core/Core/Movie.cpp, line 421 [r3] (raw file):

Previously, lioncash (Mat M.) wrote…

There's tabs instead of spaces here.

Done.

Comments from Reviewable

@lioncash
Copy link
Member

lioncash commented Aug 5, 2016

Reviewed 7 of 10 files at r1, 2 of 3 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@lioncash lioncash merged commit fa5a247 into dolphin-emu:master Aug 5, 2016
@RisingFog RisingFog deleted the remove_globals_movie branch August 5, 2016 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants