HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

AsyncTask method

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
asynctaskmethodstackoverflow

Problem

Even I know that this isn't a good way of writing code, but I need to improve this.

Here I am retrieving data from Server in Json format by Posting some variables in doInbackground. Getting all the products depends on Posting variable ie Latitude and Longitude of Current Location.All the Json data are stored in LocalDB and for accessing it I used Object Class. Depends on the users current location, the nearby restaurant will be delivered the food once they ordered.
Now the Dummy data has 101 products and 13 category.So the category is added ArrayList as a duplicates category id which will be repeated whenever the product is added. So I wrote Set code for removing the duplicates.Because these sorted category will be set in the Tabs as a TabTitleName of it.

```
private class TabNameSync extends AsyncTask {
String BASE_URL = Config.DATA_URL;
ProgressDialog nDialog;
HashMap hashMapPost = new HashMap<>();
CartRes item1 = new CartRes(); // Object Class initialization
Map.Entry me;

@Override
protected void onPreExecute() {
nDialog = new ProgressDialog(MainActivity.this);
nDialog.setMessage("Loading...");
nDialog.setIndeterminate(true);
nDialog.setCancelable(true);
nDialog.show();
}

@Override
protected String doInBackground(Void... params) {

HttpURLConnection con = null;
InputStream is = null;

StringBuffer buffer;
String bufferVariable = null;

hashMapPost.put("tag", "onload"); // values to get the response by Posting
hashMapPost.put("lat", "8.7xxxxxxxx");
hashMapPost.put("log", "77.7xxxxxxx");

try {
con = (HttpURLConnection) (new URL(BASE_URL)).openConnection();

Solution

OK, so I'm probably being quite picky here, but:

  • Your definition of hashMapPost should be changed to ... = new HashMap();. hashMapPost isn't a good name for the variable. You probably also shouldn't be using a HashMap - consider HttpPost.



  • item1 isn't a good name for the variable. This is never actually used, despite being initialised in a loop, delete it.



  • me isn't a good name for the variable. It's not actually used in the code provided. Delete it.



  • doInBackground() method body is far too long, refactor to re-usable methods and call these from doInBackground(). I'd suggest, at a minimum, something along the lines of prepareRequest(), createConnection(), sendData(conn, req) and parseResponse(resp).



  • Consider using try-with-resources instead of finally {...} blocks. try-with-resources will automatically close your resources when finished with them.



  • bufferVariable isn't a good name for the variable. Use something like responseData.



  • Consider doing something if the HTTP response code is not HTTP_OK. Maybe informing the user is appropriate?



  • As you're iterating over arrays, use the Iterable interface properly. I.e. for(Object o : arr) rather than using the length of the array directly.



  • commonUtil.dbUtil is bad practice. Consider using a more appropriate persistence method. Things like Hibernate and JPA may be overkill, but putting utilities in "common" classes/packages is generally discouraged.



  • Refactor the JSON parsing, build some Object from the JSON and pass this along for storage in the DB.



  • You have two editors, editor and editor1 - this should at least be editor1 and editor2 - but the names should be more descriptive about what they're editing.



  • onPostExecute() method body is too long. Refactor along similar comment for doInBackground()



  • When exceptions happen, display some appropriate text to the user. Printing stacktraces is only meaningful to developers.



  • There's no point initialising variables to null (that's the default).

Context

StackExchange Code Review Q#127297, answer score: 3

Revisions (0)

No revisions yet.