Locate user of an Android device

Question

I am new to oriented object paradigm and I work on Android using Java project for an internship.
I must be able to locate the user and some around buildings I read stuff about how to setup LocationListener and all, and decided that I better write a class that manage everything for me.

public class NetCampusLocation {
 private Activity activity;
 private LocationManager locationManager;
 private LocationListener locationListener;
 private Location freshLocation;
 public NetCampusLocation(Activity activity) {
 this.activity = activity;
 this.locationManager = (LocationManager) this.activity.getSystemService(Context.LOCATION_SERVICE);
 this.locationListener = new LocationListener() {
 @Override
 public void onLocationChanged(Location location) {
 }
 @Override
 public void onStatusChanged(String provider, int status, Bundle extras) {
 }
 @Override
 public void onProviderEnabled(String provider) {
 }
 @Override
 public void onProviderDisabled(String provider) {
 }
 };
 this.freshLocation = this.locationManager.getLastKnownLocation(LocationManager.GPS_PROVIDER);
 if (this.freshLocation == null) {
 this.freshLocation = this.locationManager.getLastKnownLocation(LocationManager.NETWORK_PROVIDER);
 }
 }
 public void setLocationUpdate(String provider, int minTimeInterval, int minDistance, LocationListener listener) {
 this.locationManager.requestLocationUpdates(provider, minTimeInterval, minDistance, listener);
 }
 public void stopLocationUpdate() {
 this.locationManager.removeUpdates(this.locationListener);
 }
}

That is the first time I am writing a class on my own (without any pedagogic goal) and I ever seen an example with an interface nested to a class, I am wondering if this is even logical or totally absurd.

I am also wondering if this a good practice to pass my activity to the constructor or the class or not.

All kind of advices would be really good.

• Marc-Andre: I change your title, I hope I express what the code do. (that's what we want in the title) It's Java and not JAVA.
• Swann: I am new to the code review community and my question was more about "Am I doing something horrendous regarding oriented object programming or not" but your title seem more appropriate.
• Marc-Andre: Well I will redo my comment : Welcome to Code Review :D! Your question seems good, but your title is not quite what your question would deserve. I've changed it to represent what your code do. You can add your question about what you want to be reviewed inside your text. I hope you will have good reviews! (I'm sorry if I may had sound rude or something that was not the goal at all!)
• TMH: One thing I'd suggest, which most (all?) of the Google/Android tutorials have, is class variables start with m, so mLocationManager, mLocationListener etc. I may be wrong on this, but I believe this to denote that the variable belongs to the class, so you can quickly type m and get a list of all the variables in your class.

Answer 1

It is good practice to mark as many fields as possible as final.

private final Activity activity;
private final LocationManager locationManager;
private final LocationListener locationListener;

I assume that freshLocation will be changed in your locationListener so that one should not be final.

All fields that only get initialized once can be marked final.

---

You seem to only be using your activity inside the constructor, so you don't need to keep that as a field at all.

Also, instead of passing an Activity, it's enough to pass a Context (Activity extends Context so you can still pass an activity). The only method you use on the activity is getSystemService which is part of the Context class. Contexts are often required to pass to various methods in Android, so that is perfectly fine. It is better to pass a Context than an Activity.

---

About this code:

this.locationListener = new LocationListener() {
 @Override
 public void onLocationChanged(Location location) {

It is totally fine to do it this way. This is an anonymous inner class. The alternative is to use an inner class (non-anonymous), this would reduce code from your constructor but that code would be added in other parts of the class instead so which way you go doesn't matter much. This is fine.

• Swann: I won't change directly LocationManager nor LocationListener but I will call method to modify it like this.locationManager.removeUpdates(this.locationListener); for example, is this still fine with the final field ? Also to pass a context to I need to call the constructor using getApplicationContext() ?
• Simon Forsberg: @SwannPolydor Yes you can still use this.locationManager.removeUpdates(this.locationListener); even if locationManager and/or locationListener are final fields.
• Simon Forsberg: @SwannPolydor You don't need to change anything when passing the context, because an activity is a context you can still pass the same activity as you did before.
• Swann: I understand about the context part now. But I was not asking about the fact that the field is private but that it is "final" I am not really aware of all this things, but I will check out by my own.
• Simon Forsberg: @SwannPolydor Sorry, it was a typo. I meant final of course.

Answer 2

This piece of code clutter almost all your method space :

 this.locationListener = new LocationListener() {
 @Override
 public void onLocationChanged(Location location) {
 }
 @Override
 public void onStatusChanged(String provider, int status, Bundle extras) {
 }
 @Override
 public void onProviderEnabled(String provider) {
 }
 @Override
 public void onProviderDisabled(String provider) {
 }
 };

If you really need an empty implementation of the LocationListener, you can simply omit white-space in method like so.

 this.locationListener = new LocationListener() {
 @Override
 public void onLocationChanged(Location location) {}
 @Override
 public void onStatusChanged(String provider, int status, Bundle extras) {}
 @Override
 public void onProviderEnabled(String provider) {}
 @Override
 public void onProviderDisabled(String provider) {}
 };

This will take less space in your method and we still understand that it's an empty implementation. I find it weird that there is no default implementation that you could use, but this is not that much of a problem.

• Swann: The code was more about "is this right to do something like that" (like the skeleton) more than my code's content itself, obviously nothing is really functional in this state, so I will fill some of the interface LocationListener's methods.