Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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.

share|improve this question
1  
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. –  Marc-Andre Jul 16 at 13:48
    
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. –  Swann Polydor Jul 16 at 13:50
    
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!) –  Marc-Andre Jul 16 at 13:52
    
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. –  Tom Hart Jul 16 at 15:20

2 Answers 2

up vote 4 down vote accepted

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.

share|improve this answer
    
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() ? –  Swann Polydor Jul 16 at 14:17
    
@SwannPolydor Yes you can still use this.locationManager.removeUpdates(this.locationListener); even if locationManager and/or locationListener are final fields. –  Simon André Forsberg Jul 16 at 14:21
    
@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. –  Simon André Forsberg Jul 16 at 14:22
    
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. –  Swann Polydor Jul 16 at 14:23
    
@SwannPolydor Sorry, it was a typo. I meant final of course. –  Simon André Forsberg Jul 16 at 14:25

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.

share|improve this answer
    
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. –  Swann Polydor Jul 16 at 14:15

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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