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

refactor: use isEmpty to make intention clear #2468

Closed
wants to merge 1 commit into from
Closed

refactor: use isEmpty to make intention clear #2468

wants to merge 1 commit into from

Conversation

zielint0
Copy link
Contributor

No description provided.

@pvojtechovsky
Copy link
Collaborator

I personally prefer positive size() > 0. so it is ok for me like it is now. If others thinks that !isEmpty() is better, them I can live with that too :-)

@zielint0
Copy link
Contributor Author

I have fixed previous defects like this.
This PR fixes some new ones.

@monperrus
Copy link
Collaborator

size() > 0 is slightly more explicit (you don't have to know the meaning and contracts of isEmpty).

If Pavel prefers it, let's keep it.

Thanks @zielint0 for the proposal.

@monperrus monperrus closed this Sep 17, 2018
@zielint0
Copy link
Contributor Author

@pvojtechovsky
@monperrus
@surli

I fixed a lot of them in:
#2362
#2348
#2328
#2282

These are the last ones to fix.
It was merged by @surli so I think that this is useful.

Please consider reopening this pull request.

@zielint0
Copy link
Contributor Author

@monperrus
To make reviewers life easy, I created a list of merged PRs.
If you want to see rationale or fact if something similar was merged or not, please take a look:
#2105

@pvojtechovsky
Copy link
Collaborator

pvojtechovsky commented Sep 17, 2018

I checked #2362 #2348 #2328 #2282 and I agree that it is more readable only in this case:

		if (splitRange.length == 1 || splitRange[1].length() == 0) {
change to
		if (splitRange.length == 1 || splitRange[1].isEmpty()) {

because it is positive expression: "If empty then do something", but in all other cases I need more time to understand new expressions, because they often contain negative condition. E.g.
if (!clone.getBody().getStatements().isEmpty()) { is not better then before

and the worst case is:

		assertTrue(factory.Type().getAll().size() > 0);
changed to
 		assertFalse(factory.Type().getAll().isEmpty());

Before I read it like "Type getAll contains something". Now I have to read: "It is not true that type get all is empty". My mind needs three time more effort to understand that ...
So I would not merge these other PRs.
Tomasz I very appreciate your cleaning effort, but in this particular case, I would not agree with these changes.

@monperrus
Copy link
Collaborator

reopened per @zielint0 's request in #2475

@monperrus monperrus reopened this Sep 17, 2018
@zielint0
Copy link
Contributor Author

zielint0 commented Sep 17, 2018

@monperrus
Mercii!

@pvojtechovsky
Thank you for this explanation. I will rework it! :-)

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.

3 participants