Serviceを作りました。コードが汚いと言われました。どこが問題でしょうか？

Question

このServiceで、やりたいことはできるのですが、先輩にこれじゃ、改善しないと使えないって言われました。コードが汚いってことだと思うんですが、どこが問題でしょうか？

import android.app.Service;
import android.content.Intent;
import android.location.Criteria;
import android.location.LocationManager;
import android.os.Binder;
import android.os.IBinder;
import android.location.LocationListener;
import android.content.pm.PackageManager;
import android.location.Location;
import android.support.v4.app.ActivityCompat;
import android.os.Bundle;
import android.util.Log;
import com.google.firebase.database.DatabaseReference;
import com.google.firebase.database.FirebaseDatabase;
import com.google.firebase.auth.FirebaseAuth;

public class GpsService extends Service implements LocationListener{

    private final String TAG ="GpsService";
    final FirebaseDatabase database = FirebaseDatabase.getInstance();


    public class GpsServiceBinder extends Binder {
        public GpsService getService() {
            return GpsService.this;
        }
    }

    @Override
    public void onCreate() {
        super.onCreate();
    }

    public GpsService() {
    }

    //removeupdatesは必要
    //    @Override
    //    protected void onPause(){
    //        super.onPause();
    //        locationManager.removeUpdates(this);
    // mLocationManager.removeUpdates(listener);
    //    }

    protected void initLocationService() {

        if (ActivityCompat.checkSelfPermission(this, android.Manifest.permission.ACCESS_FINE_LOCATION) != PackageManager.PERMISSION_GRANTED && ActivityCompat.checkSelfPermission(this, android.Manifest.permission.ACCESS_COARSE_LOCATION) != PackageManager.PERMISSION_GRANTED) {
            return;
        }
        if (ActivityCompat.checkSelfPermission(this, android.Manifest.permission.ACCESS_FINE_LOCATION) != PackageManager.PERMISSION_GRANTED &&
                ActivityCompat.checkSelfPermission(this, android.Manifest.permission.ACCESS_COARSE_LOCATION) != PackageManager.PERMISSION_GRANTED) {
            return;
        }

        Criteria criteria = new Criteria();
        criteria.setAccuracy(Criteria.ACCURACY_FINE); //setAccuracyは内部では、https://stackoverflow.com/a/17874592/1709287の用にHorizontalAccuracyの設定に変換されている
        criteria.setPowerRequirement(Criteria.POWER_HIGH);
        criteria.setAltitudeRequired(false);
        criteria.setSpeedRequired(true);
        criteria.setCostAllowed(true);
        criteria.setBearingRequired(false);
        criteria.setHorizontalAccuracy(Criteria.ACCURACY_HIGH);
        criteria.setVerticalAccuracy(Criteria.ACCURACY_HIGH);


        LocationManager locationManager = (LocationManager) getSystemService(LOCATION_SERVICE);
        String bestProvider = locationManager.getBestProvider(criteria, true);
        locationManager.requestLocationUpdates(bestProvider, 6000, 1, this);
    }

    @Override
    public void onLocationChanged(Location location) {
        Log.d(TAG,"onLocationChanged");
        refreshGeoLocation(location);
    }

    @Override
    public void onStatusChanged(String provider, int status, Bundle extras) {
    }

    @Override
    public void onProviderEnabled(String provider) {
    }

    @Override
    public void onProviderDisabled(String provider) {
    }

    Double forlatitude;
    Double forlongitude;
    boolean run=false;
    Double pchecklatitude = 0.0;
    Double mchecklatitude = 0.0;
    Double pchecklongitude = 0.0;
    Double mchecklongitude = 0.0;
    Integer count=0;

    protected void refreshGeoLocation(Location location) {
        Log.v(TAG,"refreshGeoLocation");
        String user = FirebaseAuth.getInstance().getCurrentUser().getUid();
        DatabaseReference ref = database.getReference().child("users").child(user);
        DatabaseReference latitudeRef = ref.child("latitude");
        DatabaseReference longitudeRef = ref.child("longitude");


        count++;
        if (count == 1) {
            forlatitude = location.getLatitude();
            forlongitude = location.getLongitude();
        }
        if (run == false) {
            pchecklatitude = forlatitude + 0.00025094944;
            mchecklatitude = forlatitude - 0.00025094944;
            pchecklongitude = forlongitude + 0.00025094944;
            mchecklongitude = forlongitude - 0.00025094944;

                Log.v(TAG,"LocationChange():"+count.toString());
                Log.v(TAG,"getLatitude():"+location.getLatitude()+"pchecklatitude"+pchecklatitude.toString());
                Log.v(TAG,"getLatitude():"+location.getLatitude()+"mchecklatitude"+mchecklatitude.toString());

            if (location.getLatitude() > pchecklatitude || location.getLatitude() < mchecklatitude &&
                    location.getLongitude() > pchecklongitude || location.getLongitude() < mchecklongitude) {

                latitudeRef.setValue(location.getLatitude());
                longitudeRef.setValue(location.getLongitude());
                run = true;

                Log.v(TAG,"移動検知:"+count.toString());
                Log.v(TAG,"getLatitude():"+location.getLatitude()+"pchecklatitude"+pchecklatitude.toString());
                Log.v(TAG,"getLatitude(:)"+location.getLatitude()+"mchecklatitude"+mchecklatitude.toString());

                //ここで保存用に追加
                forlatitude = location.getLatitude();
                forlongitude = location.getLongitude();
            }
        } else if (run == true) {
            latitudeRef.setValue(location.getLatitude());
            longitudeRef.setValue(location.getLongitude());
            Log.v(TAG,"追跡中:"+count.toString());
            Log.v(TAG,"getLatitude():"+location.getLatitude()+"pchecklatitude"+pchecklatitude.toString());
            Log.v(TAG,"getLatitude():"+location.getLatitude()+"mchecklatitude"+mchecklatitude.toString());
        }
    }

    @Override
    public IBinder onBind(Intent intent) {
        throw new UnsupportedOperationException("Not yet implemented");
    }

    private void startLocation() {
        initLocationService();
    }

    private void stopLocation() {
        //firebase.auth().signOut();これでログアウトはできるが、ログアウトはしない
        database.goOffline();
    }

    @Override
    public int onStartCommand(Intent intent, int flags, int startId) {

        String check =intent.getStringExtra("start/stop");
        if(check.equals("start")){
            startLocation();
        }else if(check.equals("stop")){
            stopLocation();
        }

        return super.onStartCommand(intent, flags, startId);
    }

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

}

Comments

[yoorwm]

「コードが汚いってことだと思うんですが」という部分は先輩に聞いたのでしょうか？推測に過ぎない事を、赤の他人に聞くのは好ましい事では無いと思いますが

[ygoooo]

「コードが汚い」も抽象的でわかりません。まず先輩に、なぜ使えないと思ったのかききましょう。

[atatatatata]

コードが汚いとも言っていました。何かアドバイスをもらいたいです

[atatatatata]

調べろって言われました。

[atatatatata]

見てわかる問題がないならそれでいいんですが、

Answer 1

「誰に質問すべきか」を考えていますか。

先輩が「改善すべき」と言ったのであれば、改善すべき点を知っているのは先輩で先輩に聞くべきです。
ネットで質問しても、得られた回答が先輩の意図したものである可能性は限りなく低いです。

また、仮に改善点を聞いて「自分で考えろ」と言われたのであれば、この質問をしている行為自体が先輩の期待を裏切っています。

また、先輩は「使い物になるように改善しろ」と言ったのであれば、「コードが汚い」という意味ではないと思います。コードは汚くても動きます。
先輩はこれでは「仕様を満たしていないから改善しろ」と言った可能性もあります、これもやはり先輩に確認すべきです。

Comments

[atatatatata]

わかりました。先輩に確認して、何を意図した発言であったのかを理解して考えます。
ありがとうございます。

Answer 2

その先輩が、具体的にどの部分が汚いと言ってきたのかがわかりませんが、
コーディングルールは会社などのグループや、個人によって違うので、
何がどう汚いのか？などを指示するのは難しいです。

ただ、私が思うに、失礼ながら質問者さんはまだjavaの基礎知識自体が不足しているようなので、
このあたりのHPか
Java セキュアコーディングスタンダード
Javaコーディング標準

このあたり書籍などを参考にしましょう。
Javaルールブック ～読みやすく効率的なコードの原則

Comments

[atatatatata]

ありがとうございます。勉強します！！

Answer 3

限られた時間内で成果を出さないといけないのでなかなか難しいとは思いますが、
逆に完璧、美しいと言われるようなコードがどんなコードを想像してみましょう。
それに近づけるためにすぐ出来ることをやってみてはいかがでしょうか。
汚いかどうかは別として、綺麗とは言えない というような意味合いで考えてみてはいかがでしょうか。

Comments

[atatatatata]

わかりました。ありがとうございます！！

Answer 4

基本的に、その組織のコーディングルールに従って書くべきで、何をもってきれいか汚いかというのは外部から指摘しにくいかと思います。
ただ、あえて言えば、定数は定義してそれを代入なり計算なりに使ったほうが良いかと思います。

Comments

[atatatatata]

わかりました。定数の定義を使用するようにします。
ありがとうございます。