Skip to content

Some code clean ups. #3500

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

Merged
merged 1 commit into from
Nov 8, 2015
Merged

Some code clean ups. #3500

merged 1 commit into from
Nov 8, 2015

Conversation

dromato
Copy link

@dromato dromato commented Nov 7, 2015

Nothing that could change logic or application flow, just minor refactors to be consistent with good practices and clean code.
To sum up changes:

  • Simplified some conditions
  • Changed small l to L in long number, as l tends to look like 1 and might confuse
  • Removed "return" statements where those are not necessary (last instruction in function)
  • Inlined returns where there were no need for creating new variable (easier to read)
  • Deleted unnecessary colons

@@ -195,7 +195,7 @@ public void call(final Subscriber<? super T> child) {
final AtomicBoolean resumeBoundary = new AtomicBoolean(true);

// incremented when requests are made, decremented when requests are fulfilled
final AtomicLong consumerCapacity = new AtomicLong(0l);
final AtomicLong consumerCapacity = new AtomicLong(0L);
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary default value.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

@@ -112,8 +112,7 @@ public OperatorZip(Func9 f) {
final Zip<R> zipper = new Zip<R>(child, zipFunction);
final ZipProducer<R> producer = new ZipProducer<R>(zipper);
child.setProducer(producer);
final ZipSubscriber subscriber = new ZipSubscriber(child, zipper, producer);
return subscriber;
return new ZipSubscriber(child, zipper, producer);
Copy link
Member

Choose a reason for hiding this comment

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

This will conflict with a zip patch. Could you undo this?

Copy link
Author

Choose a reason for hiding this comment

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

Done :).

@akarnokd
Copy link
Member

akarnokd commented Nov 7, 2015

Could you also squash the commits?

@dromato
Copy link
Author

dromato commented Nov 7, 2015

Squashed :)

@akarnokd
Copy link
Member

akarnokd commented Nov 7, 2015

👍

} else {
return true;
}
return i++ != 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Programming on side effects 😿

Copy link
Author

Choose a reason for hiding this comment

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

Well I didn't want to change any existing logic there...
I don't know code base well enough to feel comfortable with that.
I could take that "i++" one separate line higher.

Copy link
Contributor

Choose a reason for hiding this comment

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

I could take that "i++" one separate line higher.

That's why I don't like such code, it's non-obvious (but short), in fact, "post ++" will be applied after the condition check, so correct transformation would be:

final boolean result = i != 2;
i++;
return result;

At least, we don't have undefined behavior here, so, you can leave it as is, but personally, I'd rewrite it.

Copy link
Author

Choose a reason for hiding this comment

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

i++;
return i != 3;

Should also be fine, right?
We increment both sides by 1.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep.

Copy link
Author

Choose a reason for hiding this comment

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

Changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

Copy link
Author

Choose a reason for hiding this comment

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

My pleasure 😃

@artem-zinnatullin
Copy link
Contributor

👍

Nothing that could change logic or application flow, just minor refactors to be consistent with good practices and clean code.
@abersnaze
Copy link
Contributor

looks good.

abersnaze added a commit that referenced this pull request Nov 8, 2015
@abersnaze abersnaze merged commit 36c50f9 into ReactiveX:1.x Nov 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants