Issue 29 Network State #34

Mecharyry · 2014-09-10T09:39:18Z

This pull request addresses issue #29 Provide the Current Network State.

Just adds two methods into Merlin class which will retrieve the network info from a given context and return the network status.

I was tempted to create a dummy Application in the android manifest and have it static so that you would not be required to pass a context in order to retrieve a network status, but there wouldn't be any guarantee that the onCreate would be called before someone tried to access the isConnected method.

^^ Me forgetting that this is a library 👅

blundell · 2014-09-10T09:41:48Z

@Mecharyry onCreate of a DummyApplication would never be called. This is a library, so it's Application class is never instantiated .. as it is not an application. It is used by another application.

Mecharyry · 2014-09-10T09:42:38Z

Oh yea of course. Just as well I didn't go for that option then 👅

blundell · 2014-09-10T09:47:04Z

This Merlin object appears to have another responsibility and can't be newed up directly by a client.

The methods are static, so what about a new class? Also a from(Context context) method allows easy statics but with the creation of a class.

MerlinSleeve.from(context).isConnected();

Example useage

private MerlinSleeve merlinSleeve;

onCreate() {
 merlinSleeve =  MerlinSleeve.from(this);
}

onClick() {
   if(merlinSleeve.isConnected()) {
      // waaat
   }
}

Debate away @Mecharyry @ouchadam

blundell · 2014-09-10T09:47:53Z

or MerlinsBeard lol

ouchadam · 2014-09-10T09:50:54Z

core/src/main/java/com/novoda/merlin/Merlin.java

@@ -42,6 +46,16 @@ public void registerBindable(Bindable bindable) {
        registerer.registerBindable(bindable);
    }

+    public static boolean isConnected(Context context) {


❌ the whole point of Merlin is to provide a connected state which has done a network ping to guarantee the network is available. It should use the state attained by the MerlinService

yeah so if we move this to another class and have it javadoc'd correctly we can have this quicker method with the caveat that it is not as accurate. (this is what the github issue defines?)

what I mean is that this logic is duplicating what the MerlinService does. The isConnected state is fetched in callbacks.

The issue meant the state within the service, which can be created via the builder with or without host pinging (if you want a less reliable result)

but that's a lot of setup and service binding where this is 1 synchronous call?

Thought I should probably comment here 👅 Does this issue want a network connectivity method, which is the solution above. Or does the issue call for knowledge on whether the endpoint is active, in my opinion these are two separate issues. @ouchadam We create two separate issues to handle the two use cases?

xrigau · 2014-09-10T10:02:54Z

…work status.

…atus.

ouchadam · 2014-09-10T15:14:13Z

core/src/main/java/com/novoda/merlin/MerlinsBeard.java

+        return new MerlinsBeard(context);
+    }
+
+    private MerlinsBeard(Context context) {


👎 would prefer a newInstance which takes a context but a constructor which takes a ConnectivityManager!

good point @ouchadam (from is more client facing friendly than newInstance tho)

and lets not forget the tests 👻

…twork status.

blundell · 2014-09-11T09:53:31Z

LGTM @ouchadam the constructor comment needs addressing. Then as we discussed the NoTils change, can that be in another PR. As the public interface here looks good?

ouchadam · 2014-09-11T09:54:54Z

👍 that's good with me

Mecharyry · 2014-09-11T11:33:52Z

Sorry @ouchadam should have said that I ran the tests again, they all passed.

ouchadam · 2014-09-11T11:39:55Z

👍 good stuff, but it also means there's an untested class 😉

Mecharyry · 2014-09-12T08:13:56Z

Sorry @ouchadam didn't notice this comment. Which class are you talking about? The MerlinService? That one is tested, it couldn't be tested as well as it could have been due to the constructor. I am happy to write a test if you let me know which one you are talking about.

ouchadam · 2014-09-12T08:57:14Z

The CurrentNetworkStatusFetcher seems to have 0 tests

Mecharyry · 2014-09-12T09:00:28Z

Right, I'm on it. 😄

Mecharyry · 2014-09-12T09:49:20Z

@ouchadam Tests added 😃

blundell · 2014-09-12T10:04:50Z

unit-tests/src/test/java/com/novoda/merlin/service/CurrentNetworkStatusFetcherShould.java

+    }
+
+    @Test
+    public void getPerformsHostPingerPingWhenIsConnectedReturnsTrue() {


it's preferential to write these method names givenwhenthen rather than thenwhengiven. Just easier to think about going from A - B rather than backwards B - A

i.e. whenNetworkIsConnectedHostPingerPings()

I'll add that to my notes 👅

devisnik · 2014-09-12T10:57:50Z

core/src/main/java/com/novoda/merlin/service/CurrentNetworkStatusFetcher.java

-    public void get(Context context) {
-        NetworkInfo activeNetworkInfo = getNetworkInfo(context);
-        if (isConnectable(activeNetworkInfo)) {
+    public void get() {


A public get() method that is void? There must be a better name for that method.

Any suggestions? getAndReturnInCallback?

fetch() going once......twice.......sold

fetchViaPing?

Doesn't always perform a ping though, only if available.

Do you not think that we should highlight that the result is retrieved via a callback?

blundell · 2014-09-15T09:13:16Z

core/src/main/java/com/novoda/merlin/service/CurrentNetworkStatusFetcher.java

-    public void get(Context context) {
-        NetworkInfo activeNetworkInfo = getNetworkInfo(context);
-        if (isConnectable(activeNetworkInfo)) {
+    public void fetch() {


maybe fetchWithPing() makes more sense now I read the other methid

fetchWithPing fetchWithoutPing? both should follow the same prefix

no because fetch and get are two different commands.

fetch is go fetch this and come back to me when you have done it - callback

get is GET THIS NOW - return value

makes sense, but the class needs a new name in that case!

Sorry, will change it back then....

Retreiver? haha i think the class name is fine for now

💯 @Mecharyry after this change to Retreiver I'll be happy to merge

blundell · 2014-09-15T09:14:07Z

LGTM

@ouchadam 👀

ouchadam · 2014-09-15T09:46:32Z

demo/src/main/res/layout/main.xml

+  <Button
+    android:id="@+id/currentStatus"


this id should be underscored not camelCase!

Sorry thought I had changed this....

Issue 29 Network State

xrigau · 2014-09-15T13:49:41Z

👍 nice one @Mecharyry

xrigau · 2014-09-15T14:36:39Z

@ouchadam @blundell @Mecharyry has this been released?

blundell · 2014-09-15T14:37:09Z

that is on @Mecharyry 's //TODO list

ouchadam · 2014-09-15T15:18:27Z

@xrigau #12

Ryan Feline added 6 commits September 10, 2014 09:24

Creates isConnectedMethod to retrieve the current network status.

bf4baa5

Splits the getNetworkInfo and isConnected methods for readability.

b099b9c

Adds null check to activeNetworkInfo.

6430920

Changes onConnectedOrConnecting to onConnected only.

350ee02

Refactors layout to use a relative layout.

96ed58e

Adds network status use case to demo.

cd03db9

ouchadam reviewed Sep 10, 2014
View reviewed changes

Ryan Feline added 5 commits September 10, 2014 11:42

Performs auto-formatting and organisation of imports on merlin project.

ef6eb0a

Creates MerlinsBeard which checks the connectivityManager for the net…

8823be9

…work status.

Removes network status implementation from Merlin.

a3c8414

Refactors CurrentNetworkStatusFetcher to use MerlinsBeard.

c746fa1

Refactors DemoActivity to use MerlinsBeard to retrieve the network st…

7f4de44

…atus.

ouchadam reviewed Sep 10, 2014
View reviewed changes

Ryan Feline added 3 commits September 10, 2014 16:35

Refactors MerlinsBeard to pass in a connectivityManager.

7f1478d

Refactors id of button to match text it displays.

a316aba

Demo refactored to use toast notification when retrieveing current ne…

81465de

…twork status.

Ryan Feline added 2 commits September 11, 2014 11:27

Passes MerlinsBeard as a dependency.

f44a47b

Makes MerlinsBeard final in Fetcher.

de3a1fc

Adds unit tests for CurrentNetworkStatusFetcher.

f010899

blundell reviewed Sep 12, 2014
View reviewed changes

Refactors test names to follow given when then format.

84f2cc9

devisnik reviewed Sep 12, 2014
View reviewed changes

Refactors CurrentNetworkStatusFetcher get method to fetch.

0366e70

blundell reviewed Sep 15, 2014
View reviewed changes

ouchadam reviewed Sep 15, 2014
View reviewed changes

Ryan Feline added 4 commits September 15, 2014 10:53

CurrentNetworkStatusFetcher methods consistently named.

1df66cc

Refactors layout ids to use underscores.

6fef411

Refactors CurrentNetworkStatusFetcher FetchWithoutPing back to get.

1a38caa

Refactors CurrentNetworkStatusFetcher to CurrentNetworkStatusRetriever.

0b4017e

ouchadam added a commit that referenced this pull request Sep 15, 2014

Merge pull request #34 from novoda/Issue_29_NetworkState

08f1a00

Issue 29 Network State

ouchadam merged commit 08f1a00 into master Sep 15, 2014

ouchadam deleted the Issue_29_NetworkState branch September 15, 2014 11:11

Issue 29 Network State #34

Issue 29 Network State #34

Conversation

Mecharyry commented Sep 10, 2014

blundell commented Sep 10, 2014

Mecharyry commented Sep 10, 2014

blundell commented Sep 10, 2014

blundell commented Sep 10, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xrigau commented Sep 10, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blundell commented Sep 11, 2014

ouchadam commented Sep 11, 2014

Mecharyry commented Sep 11, 2014

ouchadam commented Sep 11, 2014

Mecharyry commented Sep 12, 2014

ouchadam commented Sep 12, 2014

Mecharyry commented Sep 12, 2014

Mecharyry commented Sep 12, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blundell commented Sep 15, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xrigau commented Sep 15, 2014

xrigau commented Sep 15, 2014

blundell commented Sep 15, 2014

ouchadam commented Sep 15, 2014