add multiple sensors observing feature #37

catalinghita8 · 2019-02-17T15:48:52Z

Improvement that fixes #33.

Added the possibility of observing and getting data flow from multiple sensors.

pwittchen

Thanks for your PR and your effort. Unfortunately, you reformatted the whole file with different code style than the one, which is used in the project, so I cannot see your exact changes within PR without analyzing the whole file. Please, use code style described here: https://github.com/pwittchen/ReactiveSensors#code-style. Moreover, I don't see any unit tests related to the feature you introduced.

Regards,
Piotr

catalinghita8 · 2019-02-17T19:10:06Z

Hi @pwittchen,
I completely missed the coding style - my bad on that - thus adjusted the changes properly now so you can review them.

Regarding adding unit tests, I am aware that I haven't added any, mainly because currently there are no unit tests (beside the basic one) and I don't want to add new dependencies (like Mockito) into the project. It usually requires some responsability and I believe that the creator should decide upon adding new dependencies as it might have repercussions on the library.

Also you might decide enforcing some style for the tests as well. Therefore, once you review the code and decide on including some basic tests, I will be more than glad to write some - following your pattern - for this feature as well.

All the best,
Catalin

pwittchen · 2019-02-17T19:16:47Z

Hi,

Thanks for update. Can you squash your commits into one? It'll be easier to view changes in the PR and git history will be organized better. You can use this tool: http://rebaseandsqua.sh/.

Regarding unit tests, you're right. This project unfortunately has poor tests. I usually add them in my projects, but I didn't have more time to work on that in this case, so we can skip tests in your PR and test infrastructure/configuration can be improved later.

catalinghita8 · 2019-02-17T20:00:11Z

Thanks for the feedback. Just rebased and squashed that redundant commit. Now it should be easier to read.

Catalin

pwittchen · 2019-02-17T20:06:07Z

library/src/main/java/com/github/pwittchen/reactivesensors/library/ReactiveSensors.java

+   * @param sensorTypes sensor type from Sensor class from Android SDK
+   * @return RxJava Observable with ReactiveSensorEvent
+   */
+  public Flowable<ReactiveSensorEvent> observeSensors(final int samplingPeriodInUs,


I'd rename this method to e.g. observeManySensors - observeSensor and observeSensors looks similar and it's easy to make a mistake while using such API.

pwittchen · 2019-02-17T20:11:02Z

library/src/main/java/com/github/pwittchen/reactivesensors/library/ReactiveSensors.java

+    StringBuilder errorMessage = null;
+    for (int sensorType : sensorTypes) {
+      if (!hasSensor(sensorType)) {
+        if (errorMessage == null) {


I think, it can be simplified like that:

if(errorMessage == null) { errorMessage = new StringBuilder("Following sensors are not available on current device: "); } errorMessage.append(sensorType); errorMessage.append(" ");

In such case, else block could be skipped.

pwittchen · 2019-02-17T20:14:17Z

library/src/main/java/com/github/pwittchen/reactivesensors/library/ReactiveSensors.java

+   * @param sensorTypes sensor types array from Sensor class from Android SDK
+   * @return RxJava Observable with ReactiveSensorEvent
+   */
+  public Flowable<ReactiveSensorEvent> observeSensors(final int samplingPeriodInUs,


This method has around ~50 lines of code. It's too many. Please, try to split it into smaller methods, so the max length of the single method will have 20 lines of code or less.

pwittchen · 2019-02-17T20:16:46Z

Thanks for that. I had a few issues in the code review. Please have a look at them.

catalinghita8 · 2019-04-27T14:27:35Z

@pwittchen apologies for the late response and cheers for the constructive suggestions!

Refactored the multiple sensor observing method naming as well as the the method's content in cause.
Extracted the error message creation outside of the main method, thus reducing the size from ~50 to ~30 lines.

Extra:
As you requested the method to be under ~20, I will have to insist in keeping it to the actual size. Any further trimming by extracting essential pieces of the code that make sense in the method's context, would result in external methods that have no modular actions or even meaning.
While I agree methods should not be large and should be kept as minimal as possible, please note that the observeSensor has ~30 lines itself, thus I see no valid points in reducing the observeMultipleSensors to under 30 lines.

pwittchen · 2019-04-27T20:14:12Z

Thanks, I'll have a look at it in one week from now. I'm away from home without my laptop now. :-)

…

-- Piotr Wittchen, http://wittchen.io sob., 27 kwi 2019, 15:27 użytkownik catalinghita8 <notifications@github.com> napisał:

@pwittchen <https://github.com/pwittchen> apologies for the late response and cheers for the constructive suggestions! Refactored the multiple sensor observing method naming as well as the the method's content in cause. Extracted the error message creation outside of the main method, thus reducing the size from ~50 to ~30 lines. Extra: As you requested the method to be under ~20, I will have to insist in keeping it to the actual size. Any further trimming by extracting essential pieces of the code that make sense in the method's context, would result in external methods that have no modular actions or even meaning. While I agree methods should not be large and should be kept as minimal as possible, please note that the observeSensor has ~30 lines itself, thus I see no valid points in reducing the observeMultipleSensors to under 30 lines. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#37 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFJYFZKY32RBXMSOG52JDDPSRPFRANCNFSM4GX6QKJQ> .

catalinghita8 · 2019-06-28T15:16:19Z

Thanks, I'll have a look at it in one week from now. I'm away from home without my laptop now. :-)
…
-- Piotr Wittchen, http://wittchen.io sob., 27 kwi 2019, 15:27 użytkownik catalinghita8 notifications@github.com napisał:
@pwittchen https://github.com/pwittchen apologies for the late response and cheers for the constructive suggestions! Refactored the multiple sensor observing method naming as well as the the method's content in cause. Extracted the error message creation outside of the main method, thus reducing the size from ~50 to ~30 lines. Extra: As you requested the method to be under ~20, I will have to insist in keeping it to the actual size. Any further trimming by extracting essential pieces of the code that make sense in the method's context, would result in external methods that have no modular actions or even meaning. While I agree methods should not be large and should be kept as minimal as possible, please note that the observeSensor has ~30 lines itself, thus I see no valid points in reducing the observeMultipleSensors to under 30 lines. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#37 (comment)>, or mute the thread https://github.com/notifications/unsubscribe-auth/AAFJYFZKY32RBXMSOG52JDDPSRPFRANCNFSM4GX6QKJQ .

@pwittchen Let me know if anything is needed.

pwittchen · 2019-06-30T15:02:03Z

Ok

…

-- Piotr Wittchen, http://wittchen.io pt., 28 cze 2019, 17:16 użytkownik catalinghita8 <notifications@github.com> napisał:

Thanks, I'll have a look at it in one week from now. I'm away from home without my laptop now. :-) … <#m_8316246545055525711_> -- Piotr Wittchen, http://wittchen.io sob., 27 kwi 2019, 15:27 użytkownik catalinghita8 ***@***.*** napisał: @pwittchen <https://github.com/pwittchen> https://github.com/pwittchen apologies for the late response and cheers for the constructive suggestions! Refactored the multiple sensor observing method naming as well as the the method's content in cause. Extracted the error message creation outside of the main method, thus reducing the size from ~50 to ~30 lines. Extra: As you requested the method to be under ~20, I will have to insist in keeping it to the actual size. Any further trimming by extracting essential pieces of the code that make sense in the method's context, would result in external methods that have no modular actions or even meaning. While I agree methods should not be large and should be kept as minimal as possible, please note that the observeSensor has ~30 lines itself, thus I see no valid points in reducing the observeMultipleSensors to under 30 lines. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#37 (comment) <#37 (comment)>>, or mute the thread https://github.com/notifications/unsubscribe-auth/AAFJYFZKY32RBXMSOG52JDDPSRPFRANCNFSM4GX6QKJQ . @pwittchen <https://github.com/pwittchen> Let me know if anything is needed. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#37?email_source=notifications&email_token=AAFJYFZPEBYFVDKMVELE54LP4YTMHA5CNFSM4GX6QKJ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODY2LQVQ#issuecomment-506771542>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFJYF6WTGNUB7HCOPF7BOLP4YTMHANCNFSM4GX6QKJQ> .

catalinghita8 · 2019-12-29T09:58:22Z

@pwittchen As a follow-up, the changes you requested have been applied, I am still waiting for your actions.

pwittchen · 2019-12-29T14:45:11Z

Sorry, I've been quite busy, so I didn't take care of it. I'll try to push it forward soon.

pwittchen · 2019-12-29T14:50:16Z

Changes merged. They'll be available in the next release.

I haven't released new versions of my libraries from some time.
I'll try to release them quite soon due to new updates and fresh dependencies.

catalinghita8 · 2019-12-29T16:41:13Z

@pwittchen thanks!

catalinghita8 mentioned this pull request Feb 17, 2019

Observing multiple sensors together #33

Closed

pwittchen requested changes Feb 17, 2019

View reviewed changes

add multiple sensors observing feature

08252e5

catalinghita8 force-pushed the RxJava2.x branch from b9f2584 to 08252e5 Compare February 17, 2019 19:57

pwittchen requested changes Feb 17, 2019

View reviewed changes

refactor multiple reactive sensors feature

0742774

pwittchen approved these changes Dec 29, 2019

View reviewed changes

pwittchen merged commit c599768 into pwittchen:RxJava2.x Dec 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add multiple sensors observing feature #37

add multiple sensors observing feature #37

catalinghita8 commented Feb 17, 2019

pwittchen left a comment •

edited

catalinghita8 commented Feb 17, 2019 •

edited

pwittchen commented Feb 17, 2019 •

edited

catalinghita8 commented Feb 17, 2019 •

edited

pwittchen Feb 17, 2019

pwittchen Feb 17, 2019

pwittchen Feb 17, 2019

pwittchen commented Feb 17, 2019

catalinghita8 commented Apr 27, 2019

pwittchen commented Apr 27, 2019 via email

catalinghita8 commented Jun 28, 2019

pwittchen commented Jun 30, 2019 via email

catalinghita8 commented Dec 29, 2019

pwittchen commented Dec 29, 2019

pwittchen commented Dec 29, 2019

catalinghita8 commented Dec 29, 2019

add multiple sensors observing feature #37

add multiple sensors observing feature #37

Conversation

catalinghita8 commented Feb 17, 2019

pwittchen left a comment • edited

Choose a reason for hiding this comment

catalinghita8 commented Feb 17, 2019 • edited

pwittchen commented Feb 17, 2019 • edited

catalinghita8 commented Feb 17, 2019 • edited

pwittchen Feb 17, 2019

Choose a reason for hiding this comment

pwittchen Feb 17, 2019

Choose a reason for hiding this comment

pwittchen Feb 17, 2019

Choose a reason for hiding this comment

pwittchen commented Feb 17, 2019

catalinghita8 commented Apr 27, 2019

pwittchen commented Apr 27, 2019 via email

catalinghita8 commented Jun 28, 2019

pwittchen commented Jun 30, 2019 via email

catalinghita8 commented Dec 29, 2019

pwittchen commented Dec 29, 2019

pwittchen commented Dec 29, 2019

catalinghita8 commented Dec 29, 2019

pwittchen left a comment •

edited

catalinghita8 commented Feb 17, 2019 •

edited

pwittchen commented Feb 17, 2019 •

edited

catalinghita8 commented Feb 17, 2019 •

edited