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

Code refactor for readability #339

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

GermanAizek
Copy link
Contributor

@darksylinc minor changes affect readability code, but clear() function cleans string correctly

@darksylinc
Copy link
Member

Although this has a moderate amount of subjectivity, not every change improves readability. In fact it can make it worse.

For example this:

response = "";
// or this:
binOptList["-O"] = "";

With no context tells me that response and binOptList["-O"] are very likely a string. And that we want to initialize them to an empty string. There's a slight chance it could be a custom class that overrides the = operator for strings, but this chance is very minor.

However this:

response.clear();
// or this:
binOptList["-O"].clear();

Doesn't tell me anything. These variables could be a vector, a map, a list, a string, a custom class. Without an IDE and further reading I can't tell. Is response supposed to hold a list of responses? Binary responses?

There are cases such as in OgreScriptLexer.cpp where the change may likely result in higher performance, and it is a place where performance is important, hence the change is welcome.

Likewise in D3D11HLSLProgram::setTarget it is clearly obvious from context that we are dealing with strings, so mTarget.clear(); is probably better (subjective).

Now variable.empty() vs variable != "" is far less controversial, because the information conveyed is already enough (the fact that variable is empty) and in most places the fact that it is a string is also inferred from context easily (e.g. assert( ( profileName != "" ) && ( "Profile name can't be an empty string" ) ); is obvious). Additionally, variable.empty() covers more cases than variable != "" (when the string is corrupt, and avoids bizarre errors such as zero-length spaces creeping into the source code)

I can extract some of these changes selectively from your PR and merge them manually; so that we can debate the rest.

@darksylinc darksylinc self-assigned this Oct 5, 2022
@GermanAizek
Copy link
Contributor Author

@darksylinc,
I understood about readability with ur examples and I worsened it. I don't mind if you accept some changes, I'm making an offer, and yours to accept or refuse.

@eugenegff eugenegff force-pushed the master branch 2 times, most recently from 3904aa1 to da6dbf9 Compare October 7, 2022 16:36
@darksylinc darksylinc force-pushed the master branch 4 times, most recently from 76d820e to 03fbfb6 Compare November 10, 2023 22:48
@eugenegff eugenegff force-pushed the master branch 2 times, most recently from 2ee1c83 to 8b3ddcc Compare September 20, 2024 21:22
@eugenegff eugenegff force-pushed the master branch 7 times, most recently from 49c451d to 84c07a0 Compare November 27, 2024 19:08
@eugenegff eugenegff force-pushed the master branch 2 times, most recently from a3c7671 to 67f9fdd Compare December 18, 2024 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants