• Post Reply Bookmark Topic Watch Topic
  • New Topic

what can I improve in this small class?  RSS feed

 
Ranch Hand
Posts: 434
7
Android Open BSD Slackware
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
How can I improve this code to make it more readable and testable? I am on the right path?

Also I would like to know if I call a private extraction method can I  THEN extract <from this private metod just extracted> another private method?
And should I make avoid to call the method as private, because I have to use change of visibility/reflection for testing it?

NOTE for the non Android experts: onCreateView is the method where everything should fit in Android because of its architecture and Retrofit is a library that facilitates to consume REST services.


 
Sheriff
Posts: 11603
187
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Well, one thing I can say is that this code is very opaque, i.e. difficult to understand. Even after looking up OkHttpClient and Retrofit, the intent of the code is still unclear to me.

One thing that keeps giving off a smell is your passing around of an OkHttpClient.Builder instead of just a fully built OkHttpClient. Why do that? Seems to me there's no need for it.

Another smell is the package private fields bodyBezinning and view.  Why give these package private (default) access, why not private? And fact that you're setting the value of bodyBezinning in an anonymous inner Callback<Bezinnings> class (line 32) is even more smelly to me.

The way you declare those fields and the way they are set and used just makes we wonder what kind of concurrency issues you might run into with this.
 
Giovanni Montano
Ranch Hand
Posts: 434
7
Android Open BSD Slackware
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Junilu Lacar wrote:Well, one thing I can say is that this code is very opaque, i.e. difficult to understand. Even after looking up OkHttpClient and Retrofit, the intent of the code is still unclear to me.

One thing that keeps giving off a smell is your passing around of an OkHttpClient.Builder instead of just a fully built OkHttpClient. Why do that? Seems to me there's no need for it.

Another smell is the package private fields bodyBezinning and view.  Why give these package private (default) access, why not private? And fact that you're setting the value of bodyBezinning in an anonymous inner Callback<Bezinnings> class (line 32) is even more smelly to me.

The way you declare those fields and the way they are set and used just makes we wonder what kind of concurrency issues you might run into with this.



Thanks Junil, and sorry for the delay.

1) OkHttpClient.Builder

This is they way Retrofit is made, is complex library made on top of OKHttp. I am not sure there is another way, and is not relevant if I can find another snippet of code because this kind of unreadability I hunch is due to the complex android libraries third part structure( the ones from Jack Wharton in particulary)


2)bodyBezinning and view.( default instead of private


Great tip, I did not know that, this is the way in future I will declare the instance variables always as private if do not need to be used in the package. Thanks

3)value of bodyBezinning in an anonymous inner Callback<Bezinnings> class (line 32)

eh eh is not a case that RXjava has been developed to manage the so called callback hell. So in this case should I declare the anonymous class into a separate one?

thank you I am learning a lot

 
Junilu Lacar
Sheriff
Posts: 11603
187
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
My general impression of the code is that it's messy and not coherent.  Let's look at this part:

A quick look at the Android API documentation for onCreateView says this method is optional. It also states, however, "If you return a View from here, you will later be called in onDestroyView() when the view is being released."  So, my first question is: Why are you not implementing onDestroyView()? What are the consequences of not doing so? Mind you, I ask this because I'm not very familiar with Android development (although my goal for the next few months is to change that).

The comment "TODO apply butterknife" seems like a nonsensical comment at first and anyone who is uninitiated in Android development and isn't infinitely curious like me would likely just look at that with a puzzled look on their face. If you had at least used proper capitalization, it would have hinted at the fact that ButterKnife is a class:
 
Junilu Lacar
Sheriff
Posts: 11603
187
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Giovanni Montano wrote:
1) OkHttpClient.Builder
This is they way Retrofit is made, is complex library made on top of OKHttp. I am not sure there is another way


Sure there is.

There's really no point in passing a Builder around if you don't have anything along the execution path through multiple methods that it passes through calling different methods on the builder. As it is, everything you want to do with it happens in the one method, so why not just call its build() method right there and return the object that is being built? The way you had it before, the builder was created in one method, then passed around, then its build() method called out of the blue in some other method. That seems very disjointed to me.  The way I show above is tighter, IMO.

Notice that I renamed a couple of the methods I copied from your original code. I feel like that callRetrofit() could be given a better name but I don't understand your code enough right now to know what a better name would be. "Retrofit" seems like an implementation detail so I'd try to keep it out of the method names as much as possible.  Prefer to use names that reveal intent, not implementation.
 
Giovanni Montano
Ranch Hand
Posts: 434
7
Android Open BSD Slackware
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

A quick look at the Android API documentation for onCreateView says this method is optional. It also states, however, "If you return a View from here, you will later be called in onDestroyView() when the view is being released."  So, my first question is: Why are you not implementing onDestroyView()? What are the consequences of not doing so? Mind you, I ask this because I'm not very familiar with Android development (although my goal for the next few months is to change that).



I am really happy to share something can be useful to you exploiting my experience on Android.
onDestroyView is best practice that is called AFAIK. Is important because is where the Fragment view is built. In android Fragments are a really important tool


The comment "TODO apply butterknife"


Sure you could not be more right, may fault. the book you told me to buy clearly says that TODO and not necessary //comments should be avoided, with GIT you can really control the previous commits
 
Giovanni Montano
Ranch Hand
Posts: 434
7
Android Open BSD Slackware
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Prefer to use names that reveal intent, not implementation.



This is another really important point, ConnectRestApi would have been far better

Thanks, I am going to try to compile your snippet next week, if does not work I will reply here.

This post was helpful for me, cool stuff
 
Giovanni Montano
Ranch Hand
Posts: 434
7
Android Open BSD Slackware
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
was forgetting about onDestroy.
this link is a great starting point.
Basically since you open and close continuosly app, you want to save some instruction in particular moment, as when you switch between apps, you close it etc, and also the memory on the first Android devices was not enough, and so opening dozens of app could cause block to the system. Instead Android has a specific architecture to manage and  let say kinda garbage collect apps that are not used anymore if required.

Regarding fragments is different.
a fragment does not have let's say a proper life but is like a vampire that live litteraly upon the Activity class that usually call.
And bad news, the android lifecycle is different by the one of the activity!  the method you mention refers to the Fragment lifecycle that is parallel to the Activity, please look at this amazing and complex salivating pavlovian speaking image

To reply your question please have a look to this
is best practice to create the view of the Fragment in onCreateView. The view created by the method AFAIK will be (improperly) used by the xml layout that call the fragment, so this affect the layoutinflater, a complex method that work in the API.

As you can see writing readable android code is relative, because there are a lot of rules to follow, that is why Jake Wharton is making my life impossible creating a lot of new libraries with annotations, that avoid all the boiler plate code as Butterknife.

Another particularity really interesting is how Fragments communicate with Activities.
Usually you gotta use an interface, but now there is anoter library called Otto bus or RXJava( the functional thing in Android)

Is really cool that is sucha  live language, but you have to dedicate all your time to master it, that is why I was asking for more advanced topics in the Android group in javaranch
 
Giovanni Montano
Ranch Hand
Posts: 434
7
Android Open BSD Slackware
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Finally I tried it, sorry was busy with a gigantic deadline.
It works as a charm and  it is really beautiful now the code, apart for the inner class
I will treasure your refactoring suggestions, I am even moving the anonymous class in another class !thanks
 
Junilu Lacar
Sheriff
Posts: 11603
187
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Glad the refactoring actually turned out to be truly a refactoring. Just remember, refactoring changes the design of your code without changing its functionality. The intent is to make the design better in terms of simplicity, clarity, testability, and a whole other qual-"ities".  Excluded from refactoring is making it more performant. Many people mistakenly think that "efficiency and performance" is a goal of refactoring. It's not.
 
Giovanni Montano
Ranch Hand
Posts: 434
7
Android Open BSD Slackware
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Junilu Lacar wrote:Glad the refactoring actually turned out to be truly a refactoring. Just remember, refactoring changes the design of your code without changing its functionality. The intent is to make the design better in terms of simplicity, clarity, testability, and a whole other qual-"ities".  Excluded from refactoring is making it more performant. Many people mistakenly think that "efficiency and performance" is a goal of refactoring. It's not.


I see about refactoring, and for the sake of the clarity and single principle responsibility, I did another transformation but I am not sure I did well.
Basically i took out the anonymous class in another class.
so basically instead of calling an istance of

new Callback<Bezinnings>(){ all the callbacks here, please notice I am not using lambda, functional paradigm just to understand}


I changed the two instance variables from private to protected( so basically I cancelled the private keyword)
and I moved the method on anotherClass, declaring the method static.

Now I am in a situation that the two classes are more readable but making the method static will be challenging to test them.
 
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!