6
\$\begingroup\$

I have made my first app in Android Studio. I made a compass using a sensor-listener.

I would greatly appreciate if someone could review my code (it is very short), and suggest improvements.

My XML:

    <RelativeLayout xmlns:android="http://schemas.android.com/apk/res/android"
    android:layout_width="match_parent"
    android:layout_height="match_parent"
    android:background="#ff8688ff" >

    <TextView
        android:id="@+id/tvHeading"
        android:layout_width="wrap_content"
        android:layout_height="wrap_content"
        android:layout_centerHorizontal="true"
        android:layout_marginBottom="40dp"
        android:layout_marginTop="20dp"
        android:text="Current degrees: 0.0 " />

    <ImageView
        android:id="@+id/imageViewCompass"
        android:layout_width="wrap_content"
        android:layout_height="wrap_content"
        android:src="@drawable/mapCompass"
        android:layout_centerVertical="true"
        android:layout_centerHorizontal="true" />


    <Button android:id="@+id/makeCall"
        android:layout_width="fill_parent"
        android:layout_height="wrap_content"
        android:text="CALL EMERGENCY"
        android:layout_alignParentBottom="true"
        android:layout_alignParentLeft="true"
        android:layout_alignParentStart="true" />
</RelativeLayout>

My main activity:

    public class MainActivity extends Activity implements SensorEventListener {

    private ImageView compassImage;


    public float initialDegree = 0f;
    private SensorManager manager;

    TextView infoTextAboutDegrees;

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_main);

        Button startBtn = (Button) findViewById(R.id.makeCall);
        startBtn.setOnClickListener(new View.OnClickListener() {
            public void onClick(View view) {
                emergencyCall();
            }
        });


        compassImage = (ImageView) findViewById(R.id.imageViewCompass);

        infoTextAboutDegrees = (TextView) findViewById(R.id.tvHeading);

        manager = (SensorManager) getSystemService(SENSOR_SERVICE);


    }

    @Override
    protected void onResume() {
        super.onResume();

        manager.registerListener(this, manager.getDefaultSensor(Sensor.TYPE_ORIENTATION),
                SensorManager.SENSOR_DELAY_GAME);
    }

    @Override
    protected void onPause() {
        super.onPause();

        manager.unregisterListener(this);
    }

    public void emergencyCall() {

        Intent phoneIntent = new Intent(Intent.ACTION_CALL);
        phoneIntent.setData(Uri.parse("tel:911"));

        try {
            startActivity(phoneIntent);
            finish();
            Log.i("Call success...", "");
        } catch (android.content.ActivityNotFoundException ex) {
            Toast.makeText(MainActivity.this,
                    "Call fail", Toast.LENGTH_SHORT).show();
        }
    }

    @Override
    public void onSensorChanged(SensorEvent event) {


        float currentDegree = Math.round(event.values[0]);

        infoTextAboutDegrees.setText("Current degrees: " + Float.toString(currentDegree));

        RotateAnimation rotateAnimation = new RotateAnimation(
                initialDegree,
                -currentDegree,
                Animation.RELATIVE_TO_SELF, 0.5f,
                Animation.RELATIVE_TO_SELF,
                0.5f);

        rotateAnimation.setDuration(210);

        rotateAnimation.setFillAfter(true);

        compassImage.startAnimation(rotateAnimation);
        initialDegree = -currentDegree;

    }

    @Override
    public void onAccuracyChanged(Sensor sensor, int accuracy) {

    }
}

My AndroidManifest:

    <?xml version="1.0" encoding="utf-8"?>
<manifest xmlns:android="http://schemas.android.com/apk/res/android"
    package="com.example.abdiqani.compass" >
    <uses-permission android:name="android.permission.CALL_PHONE"></uses-permission>

    <application
        android:allowBackup="true"
        android:icon="@mipmap/ic_launcher"
        android:label="@string/app_name"
        android:theme="@style/AppTheme" >
        <activity
            android:name=".MainActivity"
            android:label="@string/app_name" >
            <intent-filter>
                <action android:name="android.intent.action.MAIN" />

                <category android:name="android.intent.category.LAUNCHER" />
            </intent-filter>
        </activity>
    </application>

</manifest>
\$\endgroup\$
4
\$\begingroup\$

We'll tackle this one file at a time.

Your XML

Not many things wrong here. Your indentation is wrong on the first line (should have no whitespace) but that doesn't do anything to the code at all and hardly improves readability so don't fret. One issue of concern, although, is your use of

android:text="Current degrees: 0.0 "

and

android:text="CALL EMERGENCY"

Realistically, you should utilise your strings.xml file. This is a good habit to get into - imagine writing a big project, using the same phrase in a lot of different places (say, fubar) and then realising that you spelt it wrong - oh no, it's actually spelt foobar! In your case, it might take quite a bit of effort (if you're using multiple activities like any good android developer) to fix, but if you get into the habit of using @string/fubar you only need to change one thing for full effect - the definition of the string fubar in your strings.xml file.

In fact, if you define a string in strings.xml, then discover you spelt the string name wrong, or that you need to change the name of the string, in Android Studio you can refactor and rename - this changes all instances of a string and renames it as something else, including the definition in strings.xml.

Another thing I'm really not liking is you aren't defining the size of your text in that text view of yours, nor in the button. To do this, use

android:textSize="20sp"

Note that all android developers had to read this, or something like it, at some point, to get a good idea of what dimensions do what and when to use each. Another thing that you need to make into a habit is using android:gravity, which sets the alignment of the content inside the view as explained here. This isn't a problem when

android:layout_width="wrap_content"
android:layout_height="wrap_content"

But when you start getting into views with defined heights, this android:gravity property will come in handy. Better get used to it now.

Your Java

I apologise in advance for not being very good at java. At all.

Alright. You're probably sick of me saying it but you need to get into the habit of doing certain stuff. And then there's things you need to get out of the habit of doing. Your third line of code (ignoring whitespace) is one of them.

public float initialDegree = 0f;

Does this really need to be public? Not unless there's some sneaky stuff you aren't showing us. Is it bad if it's public? Not yet. Will it screw you up big time when you're writing more complex stuff? Probably. There's really nothing shady here - if it doesn't need to be public, don't make it public - a good rule to go by.

Not entirely sure about how you're doing your compass rotation, but I suppose it looks more aesthetically pleasing if you use an animation rather than just rotate it accordingly.

Your onAccuracyChanged function is empty. That doesn't look terribly helpful.

But other than that, I can't see anything wrong with your java activity. Looks clean, quite easy to follow, nice crisp variable names (but make sure you declare as many variables as possible, usually all of them, after your imports and class definition).

Your AM

As a side note, AM isn't actually a known abbreviation of AndroidManifest but I'm hoping it will catch on.

Your AM is looking pretty succinct; you get all the information you need quite easily. There's really nothing that needs to be said about this AM, as it's all already working as best as I could make it. Kudos.

To conclude, I think you've gained a lot of useful experience from this little project of yours, regardless of how short you say it is. The only problems are things that aren't even problems yet; just concerns, and food for thought. Good luck in your next project, and good work in this one.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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