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

Changed just() function in Observable class to varargs. #3798

Closed
wants to merge 6 commits into from

Conversation

princebansal
Copy link

Just functions in Observable class were overloaded unnecessarily with increasing arguments from 1 to 10.
Marked all those methods as deprecated and defined one more overloaded just function with varargs as:
@SuppressWarnings("unchecked") public static <T> Observable<T> just(T... t) { return from(t); }

This will reduce the code redundancy and make it more elegant.

@zsxwing
Copy link
Member

zsxwing commented Mar 26, 2016

@princebansal please take a look at #686 for previous discussion about varargs

@princebansal
Copy link
Author

@zsxwing Actually I was facing some build errors before. But now I have modified the code and using varargs. I think varargs is the better option to use rather than Iterator or multiple arguments.

@JakeWharton
Copy link
Contributor

👎

The arity versions bind more highly in overload resolution so you will make every usage of just below 11 values (which is surely 100% of them) now display as deprecated at every call site–presumably without a way to select the correct overload.

That aside, arity overloads traditionally exist for a two reasons: to provide specific implementations which are more performant for the common cases and to avoid the Object[] allocation of calling a varargs method. RxJava is an allocation party already so the latter isn't too big of a deal (plus the 2+ versions allocate the Object[] explicitly anyway). The single arity overload, however, provides an optimized implementation which you would not want to replace with the unsized iterator version.

@akarnokd
Copy link
Member

I'm sorry, but this is PR is no good. There are several problems:

  • Android studio file
  • large amounts of space changes that make it difficult to review Observable
  • space changes in other files
  • varargs now conflict with 2-9 arg just which is bad; you had to change tests which is also an indication of problems to come to others.
  • varargs usually requires SuppressWarnings in JDK 6 code which was not required with the 2-9 arg common use cases.
  • you can now have an empty just() which is odd.

👎

@princebansal
Copy link
Author

I think keeping such high arity overloaded methods is not needed and appears unprofessional. Varargs with generics though displays warnings but is friendly for developers to code using IDE as it doesn't show all methods in predictions. Also an amount of code can be cut down. The warning can be Suppressed at user side or even if not, it is not going to cause any errors.
@JakeWharton @zsxwing Just think and give a final verdict. If appears unsuitable, I'll close the PR.

@JakeWharton
Copy link
Contributor

Guava and JDK 9's factory methods disagree with you. Individual arity
methods are a sign of experienced, performance-minded developers behind an
API.

On Sat, Mar 26, 2016, 2:49 PM Prince Bansal [email protected]
wrote:

I think keeping such high arity overloaded methods is not needed and
appears unprofessional. Varargs with generics though displays warnings but
is friendly for developers to code using IDE as it doesn't show all methods
in predictions. Also an amount of code can be cut down. The warning can be
Suppressed at user side or even if not, it is not going to cause any
errors.
@JakeWharton https://github.com/JakeWharton @zsxwing
https://github.com/zsxwing Just think and give a final verdict. If
appears unsuitable, I'll close the PR.


You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#3798 (comment)

@princebansal
Copy link
Author

Okay. As per experts guidance I am closing this PR and will surely contribute to this project in future with better enhancements.

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.

4 participants