9
\$\begingroup\$

I was asked to create a test APK file by a company HR when I applied for an Android developers job,

Specification:

The goal is to develop a simple application which finds the local restaurants. The application will contain 2 screens:

A list of nearby restaurants within 1 mile radius A map that shows the restaurants and their location The application should be able to work in an offline mode showing the latest places found so far (even if these are outside the 1 mile radius). In addition, the user must have an option to refresh the data regardless of the screen he currently sees.

In order to get restaurant information please use the Google Places API.

I put in my best effort and created the app and when I emailed them the source code, I was given the following feedback:

The team felt you didn't have enough knowledge about Android in general - you use some slow ways to send data inside the application, don't know the design guidelines for Android and provided an application that looks like an IOS clone. We are not asking people to do beautiful applications but implementing things that Google says never do on Android is certainly a negative point during review.

I think that the above comments are a bit rude and unreasonable. Could an experienced Android developer PLEASE review my code and let me know if these comments are true and where have I done wrong!

This has completely ruined my day, and I am feeling very depressed that my code and knowledge of Android has been rated that bad.

RestaurantsActivity.java

RestaurantsActivity extends FragmentActivity implements
RestaurantListFragment.Contract {
private final static String TAG = RestaurantsActivity.class.getSimpleName();

private static final String TAG_LEFT_FRAG = "info_headers";

private RestaurantListFragment left_frag = null;

private DatabaseHelper mDBHelper = null;
private Boolean isInternetPresent = false;
private ConnectionDetector connectionDetector;
GooglePlaces googlePlaces;
GPSTracker gpsTracker;
ProgressDialog pDialog;

@Override
public void onCreate(Bundle savedInstanceState) {
    super.onCreate(savedInstanceState);

    getWindow().requestFeature(Window.FEATURE_ACTION_BAR);

    setContentView(R.layout.info_sub_system_main);

    ActionBar actionBar = getActionBar();
    actionBar.setDisplayShowTitleEnabled(true);
    actionBar.setDisplayHomeAsUpEnabled(false);

    mDBHelper = new DatabaseHelper(RestaurantsActivity.this);

    connectionDetector = new ConnectionDetector(getApplicationContext());

    // Check if Internet present
    isInternetPresent = connectionDetector.isConnectingToInternet();
    if (!isInternetPresent) {
        Toast.makeText(this, "Internet connection unavailable", Toast.LENGTH_SHORT).show();
    }
    gpsTracker = new GPSTracker(this);

    if (gpsTracker.canGetLocation()) {
        Log.d("Your Location", "latitude:" + gpsTracker.getLatitude() + ", longitude: "
                + gpsTracker.getLongitude());

        new LoadPlaces().execute();
    } else {
        Toast.makeText(
                RestaurantsActivity.this,
                "Couldn't get location information. Please enable GPS \nWill use cached data from previous search if available",
                Toast.LENGTH_LONG).show();
    }

    left_frag = (RestaurantListFragment)getSupportFragmentManager().findFragmentByTag(
            TAG_LEFT_FRAG);

    if (left_frag == null) {
        left_frag = new RestaurantListFragment();
        getSupportFragmentManager().beginTransaction()
        .add(R.id.info_header_mainfrag, left_frag, TAG_LEFT_FRAG).commit();
    }

}

@Override
public void onSaveInstanceState(Bundle outState) {
    super.onSaveInstanceState(outState);
}

@Override
public void onRestoreInstanceState(Bundle savedInstanceState) {
    super.onRestoreInstanceState(savedInstanceState);
}

@Override
public void onRestaurantSelected(Place place) {

    if (place != null) {
        Intent detailsIntent = new Intent(getApplicationContext(),
                RestaurantDetailActivity.class);

        detailsIntent.putExtra("reference", place.getRef());
        detailsIntent.putExtra("name", place.getName());
        detailsIntent.putExtra("formatted_address", place.getFormattedAddress());
        detailsIntent.putExtra("formatted_phone_number", place.getPhone());
        detailsIntent.putExtra("lat", place.getLatitudeAsString());
        detailsIntent.putExtra("lng", place.getLongitudeAsString());
        startActivity(detailsIntent);
    }
}

@Override
public boolean isPersistentSelection() {
    return false;
}

@Override
public void onDestroy() {
    super.onDestroy();
    mDBHelper.close();
}

@Override
public boolean onOptionsItemSelected(MenuItem item) {
    switch (item.getItemId()) {
        case R.id.ab_map:
            startActivity(new Intent(RestaurantsActivity.this, ShowPlacesOnMapActivity.class));
            return true;
        case R.id.ab_search:
            new LoadPlaces().execute();
            return true;
    }
    return false;

}

class LoadPlaces extends AsyncTask<String, String, Boolean> {

    @Override
    protected void onPreExecute() {
        super.onPreExecute();
        pDialog = new ProgressDialog(RestaurantsActivity.this);
        pDialog.setMessage(Html.fromHtml("<b>Search</b><br/>Loading Places..."));
        pDialog.setIndeterminate(false);
        pDialog.setCancelable(false);
        pDialog.show();
    }

    @Override
    protected Boolean doInBackground(String... args) {
        // creating Places class object
        googlePlaces = new GooglePlaces();

        try {
            String types = "cafe|restaurant";

            double radius = 1600; // 1 mile (in meters)

            List<Place> listPlace = googlePlaces.search(gpsTracker.getLatitude(),
                    gpsTracker.getLongitude(), radius, types);

            if (listPlace != null && listPlace.size() > 0) {

                for (Place p : listPlace) {
                    Place place = googlePlaces.getPlaceDetails(p.getRef());
                    ContentValues cv = new ContentValues();
                    cv.put(DatabaseHelper.UNIQUEID, place.getId());
                    cv.put(DatabaseHelper.REFERENCE, place.getRef());
                    cv.put(DatabaseHelper.NAME, place.getName());
                    cv.put(DatabaseHelper.ADDRESS, place.getFormattedAddress());
                    cv.put(DatabaseHelper.PHONE, place.getPhone());
                    cv.put(DatabaseHelper.LAT, place.getLatitude());
                    cv.put(DatabaseHelper.LNG, place.getLongitude());

                    mDBHelper.getWritableDatabase().replace(DatabaseHelper.TABLE, null, cv);
                }
            }
            return true;

        } catch (Exception e) {
            Log.d(TAG, e.toString());
        }
        return false;
    }

    @Override
    protected void onPostExecute(Boolean success) {
        pDialog.dismiss();
        if (success) {
            if (left_frag != null && left_frag.isVisible()) {
                Log.d(TAG, "Calling fillListView after FETCHING");
                left_frag.fillListView();
            }

        } else {
            Toast.makeText(RestaurantsActivity.this, "Sorry error occured.", Toast.LENGTH_SHORT)
            .show();
        }
    }
}

RestaurantListFragment.java

public class RestaurantListFragment extends ContractListFragment<RestaurantListFragment.Contract> {
static private final String STATE_CHECKED = "com.example.restaurantsnearby.STATE_CHECKED";

private PlacesAdapter adapter = null;

private String TAG = RestaurantListFragment.class.getSimpleName();


@Override
public void onActivityCreated(Bundle state) {
    super.onActivityCreated(state);
    setHasOptionsMenu(true);

    getListView().setSelector(R.drawable.list_selector);
    getListView().setDrawSelectorOnTop(false);

    fillListView();

    if (state != null) {
        int position = state.getInt(STATE_CHECKED, -1);
        Log.d(TAG + "List View Item Position = ", String.valueOf(position));
        if (position > -1) {
            getListView().setItemChecked(position, true);
        }
    }
}

@Override
public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState) {
    View view = inflater.inflate(R.layout.info_header_listview, container, false);
    return view;
}

@Override
public void onDestroyView() {
    super.onDestroyView();
    setListAdapter(null);
}

@Override
public void onResume() {
    Log.d(TAG, "onResume Called");
    super.onResume();
    //fillListView();
}

public void fillListView() {
    Log.d(TAG, "fillListView Called");

    if (getListView().getAdapter() == null) {
        Log.d(TAG, "SETTING ADAPTER");
        adapter = new PlacesAdapter(fetchFromDB());
        setListAdapter(adapter);
    } else {
        Log.d(TAG, "REFILLING ADAPTER");
        adapter.refill(fetchFromDB());
    }
}

@Override
public void onListItemClick(ListView l, View v, int position, long id) {
    if (getContract().isPersistentSelection()) {
        getListView().setChoiceMode(ListView.CHOICE_MODE_SINGLE);
        l.setItemChecked(position, true);
    } else {
        getListView().setChoiceMode(ListView.CHOICE_MODE_NONE);
    }

    Log.d(TAG, ((Place) this.getListAdapter().getItem(position-1)).getRef());
    getContract().onRestaurantSelected(((Place) this.getListAdapter().getItem(position-1)));
}

@Override
public void onSaveInstanceState(Bundle state) {
    super.onSaveInstanceState(state);

    if (getView() != null) {
        state.putInt(STATE_CHECKED, getListView().getCheckedItemPosition());
    }
}

@Override
public void onCreateOptionsMenu(Menu menu, MenuInflater inflater) {
    inflater.inflate(R.menu.ab_view, menu);
}

private List<Place> fetchFromDB() {
    Cursor cursor = null;
    List<Place> list = new ArrayList<Place>();
    try {
        DatabaseHelper dbHelper = new DatabaseHelper(getActivity());
        cursor =  dbHelper.getReadableDatabase().rawQuery(
                "SELECT * FROM  " + DatabaseHelper.TABLE, null);
        if (cursor != null && cursor.moveToFirst()) {
            do {
                Place place = new Place();
                place.setId(cursor.getString(cursor.getColumnIndex(DatabaseHelper.UNIQUEID)));
                place.setRef(cursor.getString(cursor.getColumnIndex(DatabaseHelper.REFERENCE)));
                place.setName(cursor.getString(cursor.getColumnIndex(DatabaseHelper.NAME)));
                place.setFormattedAddress(cursor.getString(cursor.getColumnIndex(DatabaseHelper.ADDRESS)));
                place.setPhone(cursor.getString(cursor.getColumnIndex(DatabaseHelper.PHONE)));
                place.setLatitude(cursor.isNull(cursor.getColumnIndex(DatabaseHelper.LAT)) ? 0 : cursor.getDouble(cursor.getColumnIndex(DatabaseHelper.LAT)));
                place.setLongitude(cursor.isNull(cursor.getColumnIndex(DatabaseHelper.LNG)) ? 0 : cursor.getDouble(cursor.getColumnIndex(DatabaseHelper.LNG)));
                list.add(place);
            } while (cursor.moveToNext());
        }
    } catch (Exception e) {
        Log.d(TAG, e.toString());
    } finally {
        if (cursor != null && !cursor.isClosed()) {
            cursor.close();
        }
    }
    return list;
}

// LIST ADAPTER BEGIN
class PlacesAdapter extends ArrayAdapter<Place> {

    PlacesAdapter(List<Place> placesList) {
        super(getActivity(), R.layout.info_header_row, R.id.restaurant_name, placesList);
        Log.d(TAG + " PlacesAdapter List Size = ", String.valueOf(placesList.size()));
    }

    @Override
    public View getView(int position, View convertView, ViewGroup parent) {
        PlacesViewHolder wrapper = null;
        if (convertView == null) {
            convertView = LayoutInflater.from(getActivity()).inflate(R.layout.info_header_row,
                    null);
            wrapper = new PlacesViewHolder(convertView, getActivity());
            convertView.setTag(wrapper);
        } else {
            wrapper = (PlacesViewHolder)convertView.getTag();
        }
        wrapper.populateFrom(getItem(position));
        return (convertView);
    }

    @TargetApi(11)
    public void refill(List<Place> placesList) {
        adapter.clear();
        if (placesList != null) {
            Log.d(TAG + "REFILL ADAPTER List Size = ", String.valueOf(placesList.size()));
            // If the platform supports it, use addAll
            if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.HONEYCOMB) {
                adapter.addAll(placesList);
            } else {
                for (Place place : placesList) {
                    adapter.add(place);
                }
            }
        } else{
            Log.d(TAG , "PLACES LIST IS NULL");
        }
        notifyDataSetChanged();
        Log.d("notifyDataSetChanged();", "CALLED");
    }
}

interface Contract {
    void onRestaurantSelected(Place place);

    boolean isPersistentSelection();
}
}

ShowPlacesOnMapActivity.java

public class ShowPlacesOnMapActivity extends Activity {

// Google Map
private GoogleMap googleMap;

private DatabaseHelper mDBHelper = null;

@SuppressWarnings("unused")
private Boolean isInternetPresent = false;

private ConnectionDetector connectionDetector;

GooglePlaces googlePlaces;

GPSTracker gpsTracker;

ProgressDialog pDialog;

private final String TAG = ShowPlacesOnMapActivity.class.getSimpleName();

@Override
protected void onCreate(Bundle savedInstanceState) {
    super.onCreate(savedInstanceState);
    getWindow().requestFeature(Window.FEATURE_ACTION_BAR);
    setContentView(R.layout.map_places);
    ActionBar actionBar = getActionBar();
    actionBar.setDisplayShowTitleEnabled(true);
    actionBar.setDisplayHomeAsUpEnabled(false);

    mDBHelper = new DatabaseHelper(ShowPlacesOnMapActivity.this);

    connectionDetector = new ConnectionDetector(getApplicationContext());

    isInternetPresent = connectionDetector.isConnectingToInternet();

    gpsTracker = new GPSTracker(this);

    try {

        initilizeMap();

        googleMap.setMapType(GoogleMap.MAP_TYPE_NORMAL);

        googleMap.setMyLocationEnabled(true);

        googleMap.getUiSettings().setZoomControlsEnabled(true);

        googleMap.getUiSettings().setMyLocationButtonEnabled(true);

        googleMap.getUiSettings().setCompassEnabled(true);

        googleMap.getUiSettings().setRotateGesturesEnabled(true);

        googleMap.getUiSettings().setZoomGesturesEnabled(true);

        List<Place> placeList = fetchFromDB();
        if (placeList.size() <= 0) {
            finish();
        }

        Double lastLat = 0.0;
        Double lastLng = 0.0;
        for (Place place : placeList) {
            lastLat = place.getLatitude();
            lastLng = place.getLongitude();
            if (lastLat + lastLng != 0) {
                MarkerOptions marker = new MarkerOptions()
                .position(new LatLng(lastLat, lastLng)).title(place.getName())
                .snippet(place.getFormattedAddress());

                marker.icon(BitmapDescriptorFactory
                        .defaultMarker(BitmapDescriptorFactory.HUE_BLUE));
                googleMap.addMarker(marker);
            }
        }

        CameraPosition cameraPosition = new CameraPosition.Builder()
        .target(new LatLng(lastLat, lastLng)).zoom(15).build();

        googleMap.animateCamera(CameraUpdateFactory.newCameraPosition(cameraPosition));

    } catch (Exception e) {
        Log.d(TAG, e.toString());
    }

}

@Override
protected void onResume() {
    super.onResume();
    initilizeMap();
}

@SuppressLint("NewApi")
private void initilizeMap() {
    if (googleMap == null) {
        googleMap = ((MapFragment)getFragmentManager().findFragmentById(R.id.map)).getMap();

        // check if map is created successfully or not
        if (googleMap == null) {
            Toast.makeText(getApplicationContext(), "Sorry! unable to create maps",
                    Toast.LENGTH_SHORT).show();
        }
    }
}

@Override
public boolean onCreateOptionsMenu(Menu menu) {
    new MenuInflater(this).inflate(R.menu.ab_view_map, menu);
    return (super.onCreateOptionsMenu(menu));
}

@Override
public boolean onOptionsItemSelected(MenuItem item) {
    switch (item.getItemId()) {
        case R.id.ab_search:
            // check if GPS location can get
            if (gpsTracker.canGetLocation()) {
                Log.d("Your Location", "latitude:" + gpsTracker.getLatitude() + ", longitude: "
                        + gpsTracker.getLongitude());
                new LoadPlaces().execute();
            } else {
                // Can't get user's current location
                Toast.makeText(
                        ShowPlacesOnMapActivity.this,
                        "Couldn't get location information. Please enable GPS \nWill use cached data from previous search if available",
                        Toast.LENGTH_LONG).show();
            }
            return true;
    }
    return false;

}

private List<Place> fetchFromDB() {
    Cursor cursor = null;
    List<Place> list = new ArrayList<Place>();
    try {
        DatabaseHelper dbHelper = new DatabaseHelper(ShowPlacesOnMapActivity.this);
        cursor = dbHelper.getReadableDatabase().rawQuery(
                "SELECT * FROM  " + DatabaseHelper.TABLE, null);
        if (cursor != null && cursor.moveToFirst()) {
            do {
                Place place = new Place();
                place.setId(cursor.getString(cursor.getColumnIndex(DatabaseHelper.UNIQUEID)));
                place.setRef(cursor.getString(cursor.getColumnIndex(DatabaseHelper.REFERENCE)));
                place.setName(cursor.getString(cursor.getColumnIndex(DatabaseHelper.NAME)));
                place.setFormattedAddress(cursor.getString(cursor
                        .getColumnIndex(DatabaseHelper.ADDRESS)));
                place.setPhone(cursor.getString(cursor.getColumnIndex(DatabaseHelper.PHONE)));
                place.setLatitude(cursor.isNull(cursor.getColumnIndex(DatabaseHelper.LAT)) ? 0
                        : cursor.getDouble(cursor.getColumnIndex(DatabaseHelper.LAT)));
                place.setLongitude(cursor.isNull(cursor.getColumnIndex(DatabaseHelper.LNG)) ? 0
                        : cursor.getDouble(cursor.getColumnIndex(DatabaseHelper.LNG)));

                list.add(place);
            } while (cursor.moveToNext());
        }
    } catch (Exception e) {
        Log.d(TAG, e.toString());
    } finally {
        if (cursor != null && !cursor.isClosed()) {
            cursor.close();
        }
    }
    return list;
}

@Override
public void onDestroy() {
    super.onDestroy();
    mDBHelper.close();
}


class LoadPlaces extends AsyncTask<String, String, Boolean> {

    @Override
    protected void onPreExecute() {
        super.onPreExecute();
        pDialog = new ProgressDialog(ShowPlacesOnMapActivity.this);
        pDialog.setMessage(Html.fromHtml("<b>Search</b><br/>Loading Places..."));
        pDialog.setIndeterminate(false);
        pDialog.setCancelable(false);
        pDialog.show();
    }

    /**
     * getting Places JSON
     */
    @Override
    protected Boolean doInBackground(String... args) {
        // creating Places class object
        googlePlaces = new GooglePlaces();

        try {

            String types = "cafe|restaurant"; // Listing places only cafes,
            double radius = 1600; // 1 mile (in meters)

            // get nearest places
            List<Place> listPlace = googlePlaces.search(gpsTracker.getLatitude(),
                    gpsTracker.getLongitude(), radius, types);

            if (listPlace != null && listPlace.size() > 0) {

                for (Place p : listPlace) {
                    Place place = googlePlaces.getPlaceDetails(p.getRef());
                    ContentValues cv = new ContentValues();
                    cv.put(DatabaseHelper.UNIQUEID, place.getId());
                    cv.put(DatabaseHelper.REFERENCE, place.getRef());
                    cv.put(DatabaseHelper.NAME, place.getName());
                    cv.put(DatabaseHelper.ADDRESS, place.getFormattedAddress());
                    cv.put(DatabaseHelper.PHONE, place.getPhone());
                    cv.put(DatabaseHelper.LAT, place.getLatitude());
                    cv.put(DatabaseHelper.LNG, place.getLongitude());

                    mDBHelper.getWritableDatabase().replace(DatabaseHelper.TABLE, null, cv);
                }
            }
            return true;

        } catch (Exception e) {
            Log.d(TAG, e.toString());
        }
        return false;
    }

    @Override
    protected void onPostExecute(Boolean success) {
        pDialog.dismiss();
        if (success) {
            List<Place> placeList = fetchFromDB();
            if (placeList.size() <= 0) {
                finish();
            }

            Double lastLat = 0.0;
            Double lastLng = 0.0;
            for (Place place : placeList) {
                lastLat = place.getLatitude();
                lastLng = place.getLongitude();
                if (lastLat + lastLng != 0) {
                    MarkerOptions marker = new MarkerOptions()
                    .position(new LatLng(lastLat, lastLng)).title(place.getName())
                    .snippet(place.getFormattedAddress());

                    marker.icon(BitmapDescriptorFactory
                            .defaultMarker(BitmapDescriptorFactory.HUE_BLUE));
                    googleMap.addMarker(marker);
                }
            }

            CameraPosition cameraPosition = new CameraPosition.Builder()
            .target(new LatLng(lastLat, lastLng)).zoom(15).build();

            googleMap.animateCamera(CameraUpdateFactory.newCameraPosition(cameraPosition));

        } else {
            Toast.makeText(ShowPlacesOnMapActivity.this, "Sorry error occured.",
                    Toast.LENGTH_SHORT).show();
        }
    }
}
}

GooglePlaces.java

public class GooglePlaces {

// Google API Key
//private static final String API_KEY = "MY GOOGLE KEY GOES HERE";  // Android
private static final String API_KEY = "MY GOOGLE KEY GOES HERE";    // Browser
private static final String TAG = GooglePlaces.class.getSimpleName();

// Google Places serach url's
private static final String PLACES_SEARCH_URL = "https://maps.googleapis.com/maps/api/place/search/json?";
private static final String PLACES_DETAILS_URL = "https://maps.googleapis.com/maps/api/place/details/json?";

@SuppressWarnings("unused")
private double _latitude;
@SuppressWarnings("unused")
private double _longitude;
private double _radius;

/**
 * Searching places
 * @param latitude - latitude of place
 * @params longitude - longitude of place
 * @param radius - radius of searchable area
 * @param types - type of place to search
 * @return list of places
 * */
public List<Place> search(double latitude, double longitude, double radius, String types)
throws Exception {

    this._latitude = latitude;
    this._longitude = longitude;
    this._radius = radius;

    StringBuilder urlString = new StringBuilder(
            PLACES_SEARCH_URL);

    urlString.append("&location=");
    urlString.append(Double.toString(latitude));
    urlString.append(",");
    urlString.append(Double.toString(longitude));
    urlString.append("&radius=" + _radius);
    urlString.append("&types=" + types);
    urlString.append("&sensor=false&key=" + API_KEY);

    Log.d(TAG, urlString.toString());

    try {
        String json = getJSON(urlString.toString());
        Log.d(TAG, json);
        JSONObject object = new JSONObject(json);
        JSONArray array = object.getJSONArray("results");
        List<Place> arrayList = new ArrayList<Place>();
        for (int i = 0; i < array.length(); i++) {
            try {
                Place place = Place
                .jsonToPontoReferencia((JSONObject) array.get(i),false);
                Log.d("Places Services ", "" + place);
                arrayList.add(place);
            } catch (Exception e) {
                Log.d(TAG, e.toString());
            }
        }
        return arrayList;

    } catch (JSONException ex) {
        Logger.getLogger(GooglePlaces.class.getName()).log(Level.SEVERE,
                null, ex);
    }
    return null;
}

/**
 * Searching single place full details
 * @param refrence - reference id of place
 *                 - which you will get in search api request
 * */
public Place getPlaceDetails(String reference) throws Exception {

    StringBuilder urlString = new StringBuilder(
            PLACES_DETAILS_URL);
    urlString.append("&reference=" + reference);
    urlString.append("&sensor=false&key=" + API_KEY);

    Log.d(TAG, urlString.toString());

    Place place = new Place();
    try {
        String json = getJSON(urlString.toString());
        JSONObject object = new JSONObject(json);
        JSONObject result=object.getJSONObject("result");

        place = Place.jsonToPontoReferencia(result, true);
        return place;

    } catch (JSONException ex) {
        Logger.getLogger(GooglePlaces.class.getName()).log(Level.SEVERE,
                null, ex);
    }
    return null;
}


protected String getJSON(String url) {
    return getUrlContents(url);
}

private String getUrlContents(String theUrl) {
    StringBuilder content = new StringBuilder();

    try {
        URL url = new URL(theUrl);
        URLConnection urlConnection = url.openConnection();
        BufferedReader bufferedReader = new BufferedReader(
                new InputStreamReader(urlConnection.getInputStream()), 8);
        String line;
        while ((line = bufferedReader.readLine()) != null) {
            content.append(line + "\n");
        }
        bufferedReader.close();
    }catch (Exception e) {
        e.printStackTrace();
    }
    return content.toString();
}
}

The other classes are for checking data connectivity and gps.

\$\endgroup\$
  • \$\begingroup\$ I don't know much about API key, but you do know that they are now public, since they are in the code (just want to make if those key are suppose to be private, they should be remove) \$\endgroup\$ – Marc-Andre Apr 2 '14 at 19:19
  • \$\begingroup\$ Thanks Marc-Andre, I have commented out my google api keys. \$\endgroup\$ – redbridge Apr 2 '14 at 19:21
  • 1
    \$\begingroup\$ @user1957341 Do know that they are still visible in the history of the post, you should revoke those keys and get new one for your private use. \$\endgroup\$ – Marc-Andre Apr 2 '14 at 20:07
  • \$\begingroup\$ Hi guys, so could someone let me know if I have done something really wrong in the above code? Specifically something that Google says never to do on Android? \$\endgroup\$ – redbridge Apr 3 '14 at 15:13
  • 1
    \$\begingroup\$ I certainly agree with you that they should have - at least - given you a bit more feedback upon your request... OTOH it's Google - my personal experience with them shows you shouldn't expect them to bother with anything if it's not in their interest. A shame actually, but who am I to judge their company policies? ;) \$\endgroup\$ – e-sushi Apr 5 '14 at 13:41
8
\$\begingroup\$
  1. GooglePlaces.search could return empty list instead of null. It would save you a few null checks in clients. (Effective Java, Second Edition, Item 43: Return empty arrays or collections, not nulls)

  2. Be careful about Double.toString:

    urlString.append("&location=");
    urlString.append(Double.toString(latitude));
    urlString.append(",");
    urlString.append(Double.toString(longitude));
    

    Double.toString(0.000974) returns 9.74E-4 which might not be what you want. (Google Maps might or might not handle it.)

RestaurantListFragment

  1. list could be renamed to result to express its purpose.

  2. list could have a smaller scope. (Effective Java, Second Edition, Item 45: Minimize the scope of local variables It has a good overview on the topic.)

  3. You could use only a while loop and cursor.moveToNext() to iterate over the cursor.

  4. Guard clauses makes the code flatten.

  5. You could remove some duplication with a helper method:

    private String getString(Cursor cursor, String attributeName) {
        return cursor.getString(cursor.getColumnIndex(attributeName));
    }
    

    Usage:

    place.setId(getString(cursor, DatabaseHelper.UNIQUEID));
    place.setRef(getString(cursor, DatabaseHelper.REFERENCE));
    ...
    
  6. Here is another one:

    private double getDoubleWithDefault(Cursor cursor, String attributeName, 
            double defaultValue) {
        if (cursor.isNull(cursor.getColumnIndex(attributeName))) {
            return defaultValue;
        }
        return cursor.getDouble(cursor.getColumnIndex(attributeName));
    }
    

    Usage:

    place.setLatitude(getDoubleWithDefault(cursor, DatabaseHelper.LAT, 0));
    place.setLongitude(getDoubleWithDefault(cursor, DatabaseHelper.LNG, 0));
    

Here is an improved version of fetchFromDB:

private List<Place> fetchFromDB() {
    Cursor cursor = null;
    try {
        DatabaseHelper dbHelper = new DatabaseHelper(getActivity());
        cursor = dbHelper.getReadableDatabase().rawQuery("SELECT * FROM  " 
            + DatabaseHelper.TABLE, null);
        if (cursor == null) {
            return Collections.emptyList();
        }
        List<Place> list = new ArrayList<Place>();
        while (cursor.moveToNext()) {
            Place place = new Place();
            place.setId(getString(cursor, DatabaseHelper.UNIQUEID));
            place.setRef(getString(cursor, DatabaseHelper.REFERENCE));
            place.setName(getString(cursor, DatabaseHelper.NAME));
            place.setFormattedAddress(getString(cursor, DatabaseHelper.ADDRESS));
            place.setPhone(cursor.getString(cursor.getColumnIndex(DatabaseHelper.PHONE)));
            place.setLatitude(getDoubleWithDefault(cursor, DatabaseHelper.LAT, 0));
            place.setLongitude(getDoubleWithDefault(cursor, DatabaseHelper.LNG, 0));
            list.add(place);
        }
        return list;
    } catch (Exception e) {
        Log.d(TAG, e.toString());
        return Collections.emptyList();
    } finally {
        if (cursor != null && !cursor.isClosed()) {
            cursor.close();
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Thank you very much for reviewing my in depth, and I shall make a note of all your points and try to implement them in future. \$\endgroup\$ – redbridge Apr 5 '14 at 13:32
  • \$\begingroup\$ Do you agree with the comments that I have not followed the android fundamentals and broken the design patters that one has to follow? I am sure that the task could be achieved more efficiently and cleaner, I will happily accept my errors if they are clearly pointed out, and If I have broken the minimum fundamental rules and design patterns then it should be obvious and glaring in the face. \$\endgroup\$ – redbridge Apr 5 '14 at 13:38
  • 1
    \$\begingroup\$ @redbridge: I can't say too much, I'm not familiar with Android development. You might get more reviews later. If not, there are at least two things you could do. 1. Place a bounty on the question. Unfortunately you don't have enough reputation for that yet. 2. Fix the issues mentioned above and post a follow-up question (or mabye more later) (meta.codereview.stackexchange.com/q/1065/7076). Try to post less code, currently it looks too much. It's easier to review shorter questions and they usually get more reviews. \$\endgroup\$ – palacsint Apr 5 '14 at 14:23
7
\$\begingroup\$

The worst thing you are doing that I am able to see by skimming through your code is this: you read from the database on the main thread instead of using the Loader API. In general, this is discouraged, because it may render the application UI unresponsive, depending on the amount of work the underlying database must perform to satisfy your query. You indeed perform database writing on a different thread, precisely an AsyncTask, but given that reads are on the main thread I would maintain that the writing-on-another-thread just happened by chance, not by design.

Besides, some nitpicking: you call a field left_frag, which not only is a meaningless label, but also violates the naming conventions in the Java language. This would have made me suspicious of your general experience with the language. I'm also curious as to why you are using TAG_LEFT_FRAG instead of declaring RestaurantListFragment.TAG public and using it.

As far as the iOS look is concerned, this is really impossible to judge unless you provide a couple of screenshots.

\$\endgroup\$
  • \$\begingroup\$ Thanks @Giulio, Cursor Loader makes a lot of sense. This was a quick demo that I had to write but still your points are very valid, I shall bear that in mind, Thanks for your time. \$\endgroup\$ – redbridge Apr 20 '14 at 15:04

protected by Jamal Sep 15 '16 at 5:24

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.