Retrieving location coordinates from SQL database and plotting on a map android Gmaps

Question

I am new to Android development. I am currently working on an Android app that fetches changing location coordinates from a SQL database, and plots on a Map view at regular intervals. But I think this code can be made more efficient, as I am unable to fetch locations after a certain time and can't figure out how to solve it.

I am attaching the code of which contains 2 classes. The first is MapsActivity.java that displays the locations on maps, and the second is BGTask.java. It is inside MapsActivity.java and extended from AsyncTask for fetching data from the database. There is a thread also for regular fetching of data from database and plotting on maps.

Here is my MapsActivity.java:

package com.maps.saury.mybus_demo;

import android.Manifest;
import android.content.pm.PackageManager;
import android.os.AsyncTask;
import android.os.Bundle;
import android.os.Handler;
import android.os.Message;
import android.support.v4.app.ActivityCompat;
import android.support.v4.app.FragmentActivity;
import android.view.View;
import android.widget.Button;
import android.widget.Toast;

import com.google.android.gms.maps.CameraUpdateFactory;
import com.google.android.gms.maps.GoogleMap;
import com.google.android.gms.maps.OnMapReadyCallback;
import com.google.android.gms.maps.SupportMapFragment;
import com.google.android.gms.maps.model.BitmapDescriptorFactory;
import com.google.android.gms.maps.model.LatLng;
import com.google.android.gms.maps.model.Marker;
import com.google.android.gms.maps.model.MarkerOptions;

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStream;
import java.io.InputStreamReader;
import java.net.HttpURLConnection;
import java.net.MalformedURLException;
import java.net.URL;

public class MapsActivity extends FragmentActivity implements OnMapReadyCallback {

    private GoogleMap mMap;
    String DATA_STRING;
    String lat,lng;
    Double d_lat,d_lng;
    public static Handler handler;
    public static Thread thread1;
    public static Boolean status=false;
    public static Button button;
    public static LatLng latlng,latlng_currentloc;
    public static byte count=0;
    public static  Marker now;



    @Override
    protected void onCreate(Bundle savedInstanceState) {
         button=(Button)findViewById(R.id.button2);

        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_maps);
        // Obtain the SupportMapFragment and get notified when the map is ready to be used.
        SupportMapFragment mapFragment = (SupportMapFragment) getSupportFragmentManager()
                .findFragmentById(R.id.map);
        mapFragment.getMapAsync(this);
        startAct();

    }





    @Override
    public void onMapReady(GoogleMap googleMap) {
        mMap = googleMap;
        if (ActivityCompat.checkSelfPermission(this, Manifest.permission.ACCESS_FINE_LOCATION) != PackageManager.PERMISSION_GRANTED && ActivityCompat.checkSelfPermission(this, Manifest.permission.ACCESS_COARSE_LOCATION) != PackageManager.PERMISSION_GRANTED) {

            return;
        }

        mMap.setMyLocationEnabled(true);
    }


    public void showLoc(View v){
        mMap.moveCamera(CameraUpdateFactory.newLatLng(latlng));
        mMap.animateCamera(CameraUpdateFactory.zoomTo(16));
    }


     public void exitMap(View v){
         status=false;
         finish();
     }


    public void startAct(){
        count=0;
        status=true;
        thread1=new Thread(new Loopclass());
        thread1.start();
        handler=new Handler(){
            @Override
            public void handleMessage(Message msg) {
                Toast.makeText(getApplicationContext(),"count= "+count,Toast.LENGTH_SHORT).show();
                new BGTask().execute();
            }
        };
    }


    class BGTask extends AsyncTask<Void, Void, String> {
        @Override
        protected void onPreExecute() {}

        @Override
        protected String doInBackground(Void... params) {
            String locurl = "http://krazydomain.in/test.php";
            try {
                URL url = new URL(locurl);
                HttpURLConnection http = (HttpURLConnection) url.openConnection();
                InputStream is = http.getInputStream();
                BufferedReader br = new BufferedReader(new InputStreamReader(is));
                StringBuilder sb = new StringBuilder();
                while ((DATA_STRING = br.readLine()) != null) {
                    sb.append(DATA_STRING + "\n");
                }
                br.close();
                is.close();
                http.disconnect();
                return sb.toString().trim();
            } catch (MalformedURLException e) {
                e.printStackTrace();
            } catch (IOException e) {
                e.printStackTrace();
            }
            return null;
        }

        @Override
        protected void onPostExecute(String s) {
            int i=0;
            while(s.charAt(i)!=' '){i++;}
            lat=s.substring(0,i-1);
            lng=s.substring(i+1);
             d_lat= Double.parseDouble(lat);
             d_lng= Double.parseDouble(lng);
             latlng = new LatLng(d_lat,d_lng);
            if(now!=null)
                now.remove();
            if(count==0)
            {
                mMap.animateCamera(CameraUpdateFactory.newLatLng(latlng));
                count++;
            }
            else
            {

                    now=mMap.addMarker(new MarkerOptions().position(latlng).icon(BitmapDescriptorFactory.fromResource(R.drawable.bus)).title("My loc"));

            }

        }
    }

    class Loopclass implements Runnable{

        @Override
        public void run() {
     while(true){
         if(status==false)break;

                Message msg=Message.obtain();
                msg.arg1=8;
                handler.sendMessage(msg);
                try {
                    Thread.sleep(2000);
                } catch (InterruptedException e) {
                    e.printStackTrace();
                }}


        }
    }
}

Answer 1

(not complete, but I'd start with these)

1. Your variable convention is inscrutable. Why is mMap private? Why are the next three package-private? Why does DATA_STRING look like a constant but is neither static nor final? Why are some member level and the rest static level? Variables names like br, is, sb are not clear and should be reconsidered.

2. Your static variables are A) created whenever the class is first referenced, and B) may outlive the Activity. I don't see any reason in the existing code for these to be static.

3. You should probably coordinate your thread with your activity lifecycle. For example, check if the Activity is still running in your download while loop, and exit out if not. In your onPostExecute, see if you still have a UI to modify. You might want to cancel the AsyncTask during onPause or onStop.

4. Your code style is inconsistent. The whitespace convention changes throughout and is hard to follow. You have some single-line blocks without braces, and some with braces. You have some blocks with a brace on each line and some with 2 braces and a statement on one line.

5. You don't test for null in onPostExecute when examining the String returned from doInBackground - it's very likely that http connection could fail (server down, interrupted connection, etc), and you'll end up throwing an NPE if that happens, rather than managing it.

6. while(true) { if(status==false)break; } is probably better as while(status)

7. msg.arg1=8 is not clear. I also don't see it referenced anywhere.

8. Add javadocs.

After OP's comment:

1. Instead of sending a new network request every 2 seconds, I'd send it 2 seconds after the last one was complete. Put a method in onPostExecute to initiate a new request in 2000ms. You also don't need to run your time like that - Handler has methods like postDelayed and sendEmptyMessageDelayed that'll handle that for you.

2. I'd probably use a networking library like Volley or OkHttp instead of doing it manually, but honestly what you have is probably fine (although you should probably do more to anticipate and handle failure)