what can I improve in this small class?

[Giovanni Montano]

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.

package ivano.android.com.bezinning;

import (...)//cutted the


public class BezinningFragment extends Fragment {


    String bodyBezinning;
    View view;
    private static final String API_BASE_URL ="http://myapi/api/v1.0/article/" ;

    @Nullable
    @Override
    public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState) {
        //TODO apply butterknife
        view = inflater.inflate(R.layout.bezinning_fragment, container);
        callRetrofit();

        return view;

    }


    private void callRetrofit() {
        OkHttpClient.Builder httpClient = temporaryLogInterceptor();
        BezinningAPI bezinningAPI = InstantiateRetrofit(httpClient);
        Call<Bezinnings> call = bezinningAPI.loadBezinnings("132");
        call.enqueue(new Callback<Bezinnings>() {
            @Override
            public void onResponse(Call<Bezinnings> call, Response<Bezinnings> response) {
                bodyBezinning = response.body().data.get(0).attributes.getBody();
                Spanned bodyBezinningParsedHtml = Html.fromHtml(bodyBezinning);
                setTextBodyInView(bodyBezinningParsedHtml);

            }

            @Override
            public void onFailure(Call<Bezinnings> call, Throwable t) {
                Log.d("IVO", "onFailure: ");

            }
        });

    }

    private BezinningAPI InstantiateRetrofit(OkHttpClient.Builder httpClient) {
        Retrofit retrofit = new Retrofit.Builder()
        .baseUrl(API_BASE_URL)
                .addConverterFactory(GsonConverterFactory.create()).client(httpClient.build())
                .build();

        return retrofit.create(BezinningAPI.class);
    }

    private void setTextBodyInView(Spanned bodyBezinningParsedHtml) {
        TextView labelt = (TextView) view.findViewById(R.id.bezinning);
        labelt.setText(bodyBezinningParsedHtml);
    }

    @NonNull
    private OkHttpClient.Builder temporaryLogInterceptor() {
        HttpLoggingInterceptor logging = new HttpLoggingInterceptor();
        logging.setLevel(HttpLoggingInterceptor.Level.BODY);
        OkHttpClient.Builder httpClient = new OkHttpClient.Builder();
        httpClient.addInterceptor(logging);
        return httpClient;
    }

}

[Junilu Lacar]

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]

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]

My general impression of the code is that it's messy and not coherent.  Let's look at this part:
   String bodyBezinning;    View view;    private static final String API_BASE_URL ="http://myapi/api/v1.0/article/" ;    @Nullable    @Override    public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState) {        //TODO apply butterknife        view = inflater.inflate(R.layout.bezinning_fragment, container);        callRetrofit();        return view;    }
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:

// TODO apply ButterKnife

or better yet:

// TODO apply Jake Wharton's ButterKnife

or even better, explain WHY:

// TODO apply Jake Wharton's ButterKnife for ... so that ...

[Junilu Lacar]

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.

private void callRetrofit() {
    OkHttpClient logInterceptorClient = createLogInterceptorClient();
    BezinningAPI bezinningAPI = createBezinningAPI(logInterceptorClient);

    ...
}

private OkHttpClient createLogInterceptorClient() {
    HttpLoggingInterceptor logInterceptor = new HttpLoggingInterceptor();
    logInterceptor.setLevel(HttpLoggingInterceptor.Level.BODY);
    return new OkHttpClient.Builder().addInterceptor(logInterceptor).build();
}

private BezinningAPI createBezinningAPI(OkHttpClient httpClient) {
    Retrofit retrofit = new Retrofit.Builder()
        .baseUrl(API_BASE_URL)
        .addConverterFactory(GsonConverterFactory.create())
        .client(httpClient)
        .build();

    return retrofit.create(BezinningAPI.class);
}

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]

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]

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]

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]

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]

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]

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.