App to list books from JSON data

Question

I recently submitted the code for this for an 2 hour interview but I was rejected.

The app shows book information with a image, title, and author. Each entry has a title but some entries have no image and/or author.

Here is a screenshot:

I was given this feedback:

There is a lack of proficiency from what we would expect, specifically the data side. We saw inconsistencies with structure, style, and spacing, as well as, some deprecated code (Though addressed in your review).

Of course, no response was given when questioned, so I'm here. Please review my code. If possible, please tell me how to improve it. To be honest, I don't think it's that bad as to reject me as the app fully works, but I don't really know what they were looking for.

Here is my relevant code:

MainActivity

import android.content.Context;
import android.net.ConnectivityManager;
import android.net.NetworkInfo;
import android.os.AsyncTask;
import android.os.Bundle;
import android.support.v7.app.AppCompatActivity;
import android.support.v7.widget.Toolbar;
import android.view.Menu;
import android.view.MenuItem;
import android.widget.ListView;

import org.apache.http.HttpEntity;
import org.apache.http.HttpResponse;
import org.apache.http.client.HttpClient;
import org.apache.http.client.methods.HttpGet;
import org.apache.http.impl.client.DefaultHttpClient;
import org.apache.http.util.EntityUtils;
import org.json.JSONArray;
import org.json.JSONException;
import org.json.JSONObject;
import org.json.JSONTokener;

import java.io.IOException;
import java.util.ArrayList;
import java.util.List;

public class MainActivity extends AppCompatActivity {
    private ListView mListView;
    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_main);
        Toolbar toolbar = (Toolbar) findViewById(R.id.toolbar);
        setSupportActionBar(toolbar);

        mListView = (ListView) findViewById(R.id.book_list_view);

        new GetBooksInformation().execute();
    }

    @Override
    public boolean onCreateOptionsMenu(Menu menu) {
        // Inflate the menu; this adds items to the action bar if it is present.
        getMenuInflater().inflate(R.menu.menu_main, menu);
        return true;
    }
    //run networking on the UI thread using AsyncTask
    private class GetBooksInformation extends AsyncTask<Void, Void, List<Book>>
    {

        @Override
        protected List<Book> doInBackground(Void... params) {
            try {
                //get book object list from JSON data
                return getBooks();
            } catch (IOException e) {
                return null;
            }
        }
        @Override
        protected void onPostExecute(List<Book> books)
        {
            if (books != null) {
                ItemAdapter itemAdapter = new ItemAdapter(getApplicationContext(), R.id.book_list_view, books);
                //set listview adapter
                mListView.setAdapter(itemAdapter);
            } else {
                //no books found... make toast??
            }
        }
    }
    public List<Book> getBooks() throws IOException
    {

        ConnectivityManager connMgr = (ConnectivityManager) getApplicationContext().getSystemService(Context.CONNECTIVITY_SERVICE);
        NetworkInfo networkInfo = connMgr.getActiveNetworkInfo();
        //if phone connected to Internet
        if (networkInfo != null && networkInfo.isConnected()) {
            // Create a new HttpClient
            HttpClient httpclient = new DefaultHttpClient();
            HttpGet httpGet = new HttpGet("http://de-coding-test.s3.amazonaws.com/books.json");

            // Execute HTTP Get Request
            HttpResponse response = httpclient.execute(httpGet);

            HttpEntity entity = response.getEntity();
            String result = EntityUtils.toString(entity);

            try {
                JSONArray json = (JSONArray) new JSONTokener(result).nextValue();
                List<Book> books = new ArrayList<Book>();
                int bookCount = json.length();
                String author = null; //some have no author!
                String imageURL = null; //some have no images!

                for (int i = 0; i < bookCount; i++)
                {
                    JSONObject o = json.getJSONObject(i);
                    //if the JSONObject has an imageURL field, get it
                    if (o.optString("imageURL", null) != null) {
                        imageURL = o.getString("imageURL");
                    }
                    if (o.optString("author", null) != null) {
                        author = o.getString("author");
                    }
                    books.add(new Book(o.getString("title"), imageURL, author));
                    //reset fields
                    imageURL = null;
                    author = null;
                }
                return books;
            } catch (JSONException e) {
                e.printStackTrace();
            }
        } else {   //can't do networking on device
            //TODO: ???
        }
        return null;
    }
    @Override
    public boolean onOptionsItemSelected(MenuItem item) {
        // Handle action bar item clicks here. The action bar will
        // automatically handle clicks on the Home/Up button, so long
        // as you specify a parent activity in AndroidManifest.xml.
        int id = item.getItemId();

        //noinspection SimplifiableIfStatement
        if (id == R.id.action_settings) {
            return true;
        }

        return super.onOptionsItemSelected(item);
    }
}

Book object

 public class Book {
    private String title;
    private String imageURL;
    private String author;
    public Book(String title, String imageURL, String author)
    {
        this.title = title;
        this.imageURL = imageURL;
        this.author = author;
    }
    public String getTitle() {
        return title;
    }

    public void setTitle(String title) {
        this.title = title;
    }

    public String getImageURL() {
        return imageURL;
    }

    public void setImageURL(String imageURL) {
        this.imageURL = imageURL;
    }

    public String getAuthor() {
        return author;
    }

    public void setAuthor(String author) {
        this.author = author;
    }

}

ItemAdapter (for the list view)

 import android.content.Context;
 import android.graphics.Color;
 import android.view.LayoutInflater;
 import android.view.View;
 import android.view.ViewGroup;
 import android.widget.*;
 import com.koushikdutta.urlimageviewhelper.*;   
 import java.util.List;



public class ItemAdapter extends ArrayAdapter<Book>{
    private List<Book> mBooks;
    private Context mContext;

    public ItemAdapter(Context context, int resource, List<Book> books) {
        super(context, resource, books);
        this.mBooks = books;
        this.mContext = context;
    }
    @Override
    public View getView(int position, View convertView, ViewGroup parent) {
        LayoutInflater inflater = LayoutInflater.from(mContext);
        //TODO: make viewholder or use recyclerview
        View view = inflater.inflate(R.layout.book_list, parent, false);
        view.setFocusable(false);
        view.setEnabled(false);
        Book book = getItem(position);

        String url = book.getImageURL();
        if (url != null) {
            ImageView imageView = (ImageView) view.findViewById(R.id.sourceIcon);
            UrlImageViewHelper.setUrlDrawable(imageView, book.getImageURL());
        }
        TextView text = (TextView)view.findViewById(R.id.title);
        text.setTextColor(Color.BLACK);
        text.setText(book.getTitle());
        String author = book.getAuthor();
        //don't show author if no author field
        if (author != null)
        {
            TextView authorText = (TextView)view.findViewById(R.id.author);
            authorText.setTextColor(Color.GRAY);
            authorText.setText(book.getAuthor());
        }
        return view;
    }
}

Answer 1

I see two main problems:

1) Formatting problems:

• Inconsistent formatting in the braces, which would drive me crazy if I were interviewing a candidate. It's sometimes in the same line and sometimes in the next.

• Unnecessary newlines here and there, while missing others (between closing a function and starting the next one).

This is either because you took code from somewhere else and did not adapt it, or your formatting is inconsistent. It can be fixed in almost any code editor with a simple command, and there is no excuse for not using it for code submitted for an interview.

2) You handle improperly errors:

• You left an important part unfinished (//can't do networking on device). This is important because in real-world apps, error managing of this type is what takes most of the time and effort.

• If for any reason the books cannot be retrieved, the code does nothing. You even left the TODO for showing a Toast. If you know you can show a Toast, why didn't you do it? This feels like laziness.

• You are using printStackTrace(); instead of logging. That's almost useless in Android.

And other stuff:

• I guess the deprecated code refers to HttpClient. It's been marked as deprecated for a while, and it was removed from Android 6. This means that, right now, this code is not production ready.

• Leaving URLs in the middle of the code ("http://de-coding-test.s3.amazonaws.com/books.json") is not nice, and it takes just some seconds to move it to a constant.

Answer 2

I see two main problems: the organization of your code and the lack of an object mapper library.

You have got a single getBooks() method, that does everything. Creates a network connection, fetches the data, tries to parse it, ... That method is too long and all over the place. Your methods should ideally do one thing only.

You should have one method to just get data from the internet. Give that method the URL as a parameter and have it return a String, or throw an exception if something went wrong. Later, if you want to extend the application to get something else from the internet, you can reuse this method, instead of, say, duplicating all the code to another place. Even this method could be split to two: initializing the connection and getting the data. This way you'd later, again, have a reusable method when you need to post data instead of getting it.

OK, so now you've got a String with some JSON data. Currently, you parse it using a JSONTokener and such. This yields a lot of code that's lengthy and unreadable, as well as error-prone with misspellings and missed null pointer exception possibilities. You also need to change a lot of code if the structure of the JSON changes. Enter object mappers.

Object mappers serialize and deserialize data from and to objects. There are a lot of options, but Jackson and Gson are among the most popular ones and would both be excellent choices. With Gson, you'd deserialize a single Book object like this: Book book = new Gson().fromJson(jsonString, Book.class); -- and that's it! Magic, huh? Gone are all the individual getString("title") calls and null checks. It's really that simple. Deserializing collections isn't much more difficult either.

So, that's what they meant when they said there's a lack of proficiency at the data side. Of course, you should've formatted the code too, since it's just a single shortcut in any IDE, but that would not have saved you.

PS. Basically your getBooks() method should look something like this:

public List<Book> getBooks() {
    final String json = loadFrom(URL);
    return parseBooks(json);
}

PPS. Miscellaneous issues:

• It's nasty if a method that should return a collection returns a null. Either return an empty collection or throw an exception in those cases.
• Sebastian has a very valid point about error handling in his answer
• Don't comment on obvious stuff: these are not helpful for anyone in general, nor for your attempt at getting the job:

// Create a new HttpClient
HttpClient httpclient = new DefaultHttpClient(); 

// Execute HTTP Get Request
HttpResponse response = httpclient.execute(httpGet);

If you're going to use comments, don't just duplicate what's already said in the code. If you think a part of code needs a comment, stop for a second and think: is the code hard to understand? If it is, you should primarily refactor it into something that's easier to understand: extract a method, or introduce or rename a variable, or use some appropriate pattern.