Android UI code for a test job

Question

I wrote my first Android UI application for a test job, but it was declined as employer said that he did't like "the code quality". He didn't specify what he meant. But I'm very interesting to know what's wrong with the code.

Here is the link to the Android Studio project.

Here is the text of this test:

Design and build android activity for serving full screen html ads.

When Activity is shown request for ad by sending HTTP POST request with parameter id with value of sim card IMSI (subscriber id) to http://www.505.rs/adviator/index.php

Example:

POST /adviator/index.php HTTP/1.1 
Host: www.505.rs 
Cache-Control: no-cache 
Postman-Token: abd93bb8-2857-2fd0-7679-0b25087e1d35
Content-Type: application/x-www-form-urlencoded

id=85950205030644900

{ "status":"OK", "message":"display full screen ad",
"url":"http://www.505.rs/adviator/ad.html" }

If status is equal "OK" use returned "url" and load ad into Activity webView.

If status is not equal "OK" show dialog with 'message' text and OK button. Clicking OK button will dismiss both dialog and activity.

While requesting for ad and loading ad html show spinner in center of screen and transparent background.

Once html ad is loaded hide spinner and show ad.

When user clicks ad link close activity and open native android browser with clicked url.

Activity should work in both portrait and landscape mode.

AdActivity.java:

package ru.cityads.test.activities;

import android.app.AlertDialog;
import android.content.Context;
import android.content.Intent;
import android.net.Uri;
import android.os.Bundle;
import android.support.annotation.StringRes;
import android.telephony.TelephonyManager;
import android.view.View;
import android.webkit.WebResourceError;
import android.webkit.WebResourceRequest;
import android.webkit.WebView;
import android.webkit.WebViewClient;
import android.widget.ProgressBar;

import com.trello.rxlifecycle.components.RxActivity;

import ru.cityads.test.R;
import ru.cityads.test.services.AdResponse;
import ru.cityads.test.services.AdsService;
import rx.Observable;
import rx.android.schedulers.AndroidSchedulers;
import rx.schedulers.Schedulers;

/**
 * Activity that shows ads.
 */
public class AdActivity extends RxActivity
{
  @Override
  protected void onCreate(Bundle savedInstanceState)
  {
    super.onCreate(savedInstanceState);
    setContentView(R.layout.activity_ad);
    setupWebView();
    requestAd();
    getDeviceID();
  }

  //region Private behaviour methods

  private void finishDisplayAd()
  {
    this.finish();
  }

  //endregion

  //region Private ad request methods

  private void requestAd()
  {
    showProgress();
    final AdsService adsService = new AdsService();
    final Observable<AdResponse> adRequest = adsService.requestAd(getDeviceID());
    adRequest.compose(bindToLifecycle())
      .subscribeOn(Schedulers.newThread())
      .observeOn(AndroidSchedulers.mainThread())
      .subscribe(adResponse -> {  acceptAdResponse(adResponse); }, error -> handleAdRequestError(error));
  }

  private void handleAdRequestError(Throwable e)
  {
    showErrorMessage(R.string.error_dialog_unknown_error_message);
  }

  private void acceptAdResponse(AdResponse adResponse)
  {
    if(adResponse.checkStatus())
    {
      loadUrl(adResponse.getUrl());
    }
    else
    {
      showErrorMessage(adResponse.getMessage());
    }
  }

  //endregion

  //region Private view helpers

  private void setupWebView()
  {
    getWebView().setWebViewClient(new WebViewClient()
    {
      @Override
      public void onPageFinished(WebView view, String url)
      {
        AdActivity.this.showWebView();
      }

      @Override
      public void onReceivedError(WebView view, WebResourceRequest request, WebResourceError error)
      {
        AdActivity.this.showErrorMessage(R.string.error_dialog_web_load_fail_message);
      }

      @Override
      public boolean shouldOverrideUrlLoading(WebView view, String url)
      {
        final Uri uri = Uri.parse(url);
        final Intent intent = new Intent(Intent.ACTION_VIEW, uri);
        startActivity(intent);
        finishDisplayAd();
        return true;
      }
    });
  }

  private void showProgress()
  {
    getProgressBar().setVisibility(View.VISIBLE);
    getWebView().setVisibility(View.GONE);
  }

  private void showErrorMessage(@StringRes int errorMessageId)
  {
    final String message = getResources().getString(errorMessageId);
    showErrorMessage(message);
  }

  private void showErrorMessage(String errorMessage)
  {
    final AlertDialog.Builder dialog  = new AlertDialog.Builder(this);
    dialog.setMessage(errorMessage);
    dialog.setTitle(R.string.error_dialog_title);
    dialog.setPositiveButton(R.string.error_dialog_button_text, (a, b) -> finishDisplayAd());
    dialog.create();
    dialog.show();
  }

  private void loadUrl(String url)
  {
    getWebView().loadUrl(url);
  }

  private void showWebView()
  {
    getProgressBar().setVisibility(View.GONE);
    getWebView().setVisibility(View.VISIBLE);
  }

  //endregion

  //region Private view getters

  private ProgressBar getProgressBar()
  {
    return (ProgressBar)findViewById(R.id.progressBar);
  }

  private WebView getWebView()
  {
    return (WebView)findViewById(R.id.webView);
  }

  //endregion

  //region Private device id getter method

  public String getDeviceID()
  {
    String deviceID = null;
    try
    {
      String serviceName = Context.TELEPHONY_SERVICE;
      TelephonyManager m_telephonyManager = (TelephonyManager) getSystemService(serviceName);
      deviceID = m_telephonyManager.getDeviceId();
    }
    catch(Throwable e)
    {
    }

    if(deviceID == null)
    {
      deviceID = "000000000000000";
    }

    return deviceID;
  }

  //endregion
}

AdsService.java:

package ru.cityads.test.services;

import com.squareup.okhttp.OkHttpClient;

import java.util.concurrent.TimeUnit;

import retrofit.GsonConverterFactory;
import retrofit.Retrofit;
import retrofit.RxJavaCallAdapterFactory;
import retrofit.http.Field;
import retrofit.http.FormUrlEncoded;
import retrofit.http.POST;
import ru.cityads.test.BuildConfig;
import rx.Observable;

/**
 * Service used to request ads.
 *
 * @see AdResponse
 */
public class AdsService
{
  public AdsService()
  {
    final OkHttpClient httpClient = new OkHttpClient();
    httpClient.setConnectTimeout(BuildConfig.HTTP_CLIENT_CONNECT_TIMEOUT, TimeUnit.SECONDS);
    httpClient.setWriteTimeout(BuildConfig.HTTP_CLIENT_WRITE_TIMEOUT, TimeUnit.SECONDS);
    httpClient.setReadTimeout(BuildConfig.HTTP_CLIENT_READ_TIMEOUT, TimeUnit.SECONDS);

    final Retrofit retrofit = new Retrofit.Builder()
      .addCallAdapterFactory(RxJavaCallAdapterFactory.create())
      .addConverterFactory(GsonConverterFactory.create())
      .baseUrl(BuildConfig.API_BASE_URL)
      .client(httpClient)
      .build();

    mRemoteInterface = retrofit.create(RemoteInterface.class);
  }

  public Observable<AdResponse> requestAd(String requesterId)
  {
    return mRemoteInterface.requestAd(requesterId);
  }

  //region Interface representing remote ad service. Implemented by Retrofit.

  private interface RemoteInterface
  {
    @FormUrlEncoded
    @POST("/adviator/index.php")
    Observable<AdResponse> requestAd(@Field("id") String id);
  }

  //endregion

  //region Private data

  private final RemoteInterface mRemoteInterface;

  //endregion
}

AdResponse.java:

package ru.cityads.test.services;

import com.google.gson.annotations.SerializedName;

/**
 * Represents response to ad request, made by {@link AdsService}
 *
 * @see AdsService
 */
public class AdResponse
{
  public String getStatus()
  {
    return mStatus;
  }

  public String getMessage()
  {
    return mMessage;
  }

  public String getUrl()
  {
    return mUrl;
  }

  public boolean checkStatus()
  {
    return getStatus().equals("OK");
  }

  //region Private data

  @SerializedName("status")
  private String mStatus;

  @SerializedName("message")
  private String mMessage;

  @SerializedName("url")
  private String mUrl;

  //endregion
}

Answer 1

You seem to write your code C-style. You prefix your instance variables with m, like mStatus and mMessage. You have things like //region Private data and //endregion.

Java tends to follow a different style guide, where, most of the time, the following things apply...

• No prefixing of instance variables
• 4-space tabs
• class definition, then instance variables, then constructor, then methods
• 1 class/interface per file

What you wrote works, that's for sure. But what you seem to be doing is writing C or C#-styled Java code.

I recommend you look up some Code Conventions of Java. That way you'd be able to write code which is more "natural".

---

As for getDeviceID():

  public String getDeviceID()
  {
    String deviceID = null;
    try
    {
      String serviceName = Context.TELEPHONY_SERVICE;
      TelephonyManager m_telephonyManager = (TelephonyManager) getSystemService(serviceName);
      deviceID = m_telephonyManager.getDeviceId();
    }
    catch(Throwable e)
    {
    }

    if(deviceID == null)
    {
      deviceID = "000000000000000";
    }

    return deviceID;
  }

From what I can see from the documentation, neither getSystemService nor getDeviceId() will throw any exceptions. As such, I think you could get rid of the try-catch you've got:

  public String getDeviceID()
  {
    String serviceName = Context.TELEPHONY_SERVICE;
    TelephonyManager m_telephonyManager = (TelephonyManager) getSystemService(serviceName);
    String deviceID = m_telephonyManager.getDeviceId();

    if(deviceID == null)
    {
      deviceID = "000000000000000";
    }

    return deviceID;
  }

serviceName seems only extra here, to me, so we can remove that...

  public String getDeviceID()
  {
    TelephonyManager m_telephonyManager = (TelephonyManager) getSystemService(Context.TELEPHONY_SERVICE);
    String deviceID = m_telephonyManager.getDeviceId();

    if(deviceID == null)
    {
      deviceID = "000000000000000";
    }

    return deviceID;
  }

You already use m_variable or mVariable for your instance variables, so if you're going to do that, at least be consistent and don't name method-scoped variables like they are instance scoped...

  public String getDeviceID()
  {
    TelephonyManager telephonyManager = (TelephonyManager) getSystemService(Context.TELEPHONY_SERVICE);
    String deviceID = telephonyManager.getDeviceId();

    if(deviceID == null)
    {
      deviceID = "000000000000000";
    }

    return deviceID;
  }

Additionally, according to the documentation, if TelephonyService is not available, then you will get back null. To deal with this, add a check:

  public String getDeviceID()
  {
    TelephonyManager telephonyManager = (TelephonyManager) getSystemService(Context.TELEPHONY_SERVICE);
    if(telephonyManager == null){
      return "000000000000000";
    }

    String deviceID = telephonyManager.getDeviceId();

    if(deviceID == null)
    {
      deviceID = "000000000000000";
    }

    return deviceID;
  }

Now, I don't know about how you wanna deal with bad device id's, but if you want to provide a default implementation, then 000000000000000 is fine. If that's the case, I'd recommend putting it in a separate constant so that you don't make mistakes with the amount of zeros. If you don't want to handle a default case, you should just return null straight away. In that case, you can reduce the function getDeviceID() to this:

  public String getDeviceID()
  {
    TelephonyManager telephonyManager = (TelephonyManager) getSystemService(Context.TELEPHONY_SERVICE);
    if(telephonyManager == null){
      return null;
    }
    return telephonyManager.getDeviceId();
  }

Lastly...

  @Override
  protected void onCreate(Bundle savedInstanceState)
  {
    super.onCreate(savedInstanceState);
    setContentView(R.layout.activity_ad);
    setupWebView();
    requestAd();
    getDeviceID();
  }

Here you call getDeviceID(), but it does nothing! You get the deviceId and then throw it away again!

You should remove the unneeded function call.