Android Flashlight app

Question

This is my first project, a Flashlight application for Android-based devices:

public class main extends Activity
{
    private static final byte MENU_EXIT = 0, MENU_ABOUT = 1;
    private static boolean FlashlightState;
    private static long BackPressed;
    private static ImageView Lamp;
    private Flashlight FLASHLIGHT = null;

    @Override
    public void onCreate(Bundle savedInstanceState)
    {
        super.onCreate(savedInstanceState);
        super.setContentView(R.layout.main);
        super.getWindow().addFlags(WindowManager.LayoutParams.FLAG_FULLSCREEN);
        FlashlightState = false;
        Lamp = (ImageView) findViewById(R.id.imgVw_Lamp);
        FLASHLIGHT = new Flashlight(this.getApplicationContext());

        if (Flashlight.hasFlashlight) FLASHLIGHT.Open();

        Lamp.setOnClickListener(new OnClickListener()
        {
            @Override
            public void onClick(View arg0)
            {
                Lamp.setEnabled(false);

                if (getSystemService(VIBRATOR_SERVICE) != null)
                {
                    AudioManager VibratorState = (AudioManager) getApplicationContext().getSystemService(Context.AUDIO_SERVICE);
                    if (VibratorState.getRingerMode() != AudioManager.RINGER_MODE_SILENT)
                    {
                        Vibrator Switch = (Vibrator) getSystemService(Context.VIBRATOR_SERVICE);
                        Switch.vibrate(50);
                    }
                    VibratorState = null;
                }

                if (!FlashlightState) turnOnFlashlight();
                else turnOffFlashlight();

                Lamp.setEnabled(true);
                return;
            }
        });

        return;
    }

    @Override
    protected void onStart()
    {
        super.onStart();
        BackPressed = 0;

        if (Flashlight.hasFlashlight) FLASHLIGHT.Open();

        return;
    }

    @Override
    protected void onPause()
    {
        super.onPause();
        this.turnOffFlashlight();
        BackPressed = 0;

        if (Flashlight.hasFlashlight) FLASHLIGHT.Close();

        return;
    }

    @Override
    public void onBackPressed()
    {
        if ((BackPressed + 2000) > System.currentTimeMillis())
        {
            finish();
            this.onDestroy();
        }
        else
        {
            Toast.makeText(getApplicationContext(), getString(R.string.Exit_Message), Toast.LENGTH_SHORT).show();
            BackPressed = System.currentTimeMillis();
        }

        return;
    }

    @Override
    public boolean onCreateOptionsMenu(Menu menu)
    {
        menu.add(Menu.NONE, MENU_EXIT, Menu.NONE, getString(R.string.Exit)).setIcon(android.R.drawable.ic_menu_close_clear_cancel);
        menu.add(Menu.NONE, MENU_ABOUT, Menu.NONE, getString(R.string.About)).setIcon(android.R.drawable.ic_menu_info_details);
        return super.onCreateOptionsMenu(menu);
    }

    @Override
    public boolean onOptionsItemSelected(MenuItem item)
    {
        switch (item.getItemId())
        {
            case MENU_EXIT:
                finish();
                this.onDestroy();
            break;
            case MENU_ABOUT:
                this.showAboutBox();
            break;
        }

        return super.onOptionsItemSelected(item);
    }

    private final void showAboutBox()
    {
        AlertDialog.Builder AboutBox = new AlertDialog.Builder(this);
        AboutBox.setTitle(getString(R.string.About));
        AboutBox.setIcon(android.R.drawable.ic_dialog_info);
        AboutBox.setMessage("Name:\nVisionlight\n\nDescription:\nFlashlight application for Android based devices.\n\nVersion:\n1.0.0\n\nLanguages:\nEnglish/International, Persian, Arabic\n\nPlatform:\nAndroid +2.2.x(Froyo)\n\nPermissions:\nCamera, Flashlight, Vibrate, Wake Lock.\n\nUses-feature:\nCamera [NR], Flashlight [NR].\n\nProducer/Developer:\nYousha Aleayoub(Y.P.Y)\n\nLicense:\nBSD\n\nContact:\[email protected]\n\nLink:\nhttp://yousha.blog.ir/");

        AboutBox.setNeutralButton(getString(R.string.Ok), new DialogInterface.OnClickListener()
        {
            @Override
            public void onClick(DialogInterface dialog, int which)
            {
                dialog.dismiss();
            }
        });

        AboutBox.create().show();
        return;
    }

    @Override
    protected void onDestroy()
    {
        super.onDestroy();
        this.turnOffFlashlight();

        if (Flashlight.hasFlashlight) FLASHLIGHT.Close();

        FLASHLIGHT = null;
        android.os.Process.killProcess(android.os.Process.myPid());
        return;
    }

    private final void turnOnFlashlight()
    {
        FlashlightState = true;
        Lamp.setImageResource(R.drawable.lamp_on);

        if (Flashlight.hasFlashlight)
        {
            FLASHLIGHT.On();
        }
        else
        {
            WindowManager.LayoutParams Layout = getWindow().getAttributes();
            Layout.screenBrightness = 1F;
            getWindow().setAttributes(Layout);
            findViewById(R.id.lnrLyt_Main).setBackgroundColor(Color.WHITE);
        }

        return;
    }

    private final void turnOffFlashlight()
    {
        FlashlightState = false;
        Lamp.setImageResource(R.drawable.lamp_off);

        if (Flashlight.hasFlashlight) FLASHLIGHT.Off();
        else findViewById(R.id.lnrLyt_Main).setBackgroundColor(Color.BLACK);

        return;
    }
}

What do you think? Any suggestions for improvement?

Answer 1

Use camelcase for your variable names.

Instead of: public boolean MyBoolean write public boolean myBoolean

(This is to increase readability so you can tell the difference between class and object more clearly)

Use braces for if else statements

In your code you use:

        if (!FlashlightState) turnOnFlashlight();
        else turnOffFlashlight();

More appropriate would be to use:

if(!FlashlightState) {
  turnOnFlashlight();
} else {
  turnOffFlashlight();
}

If you absolutely want to omit braces, omit them for single-statement solely-if clauses only. Such as:

if(bool) doSomething(); //No else statements

Using killProcess is a bad habit. (Destroy all your activities instead)

If you want to quit an App, just make sure all activities in the app are finish();'ed. If you want to quit an activity, use finish();, this will call onDestroy() on your activity. You should let the OS handle the lifecycle of your activities as much as possible. Assuming that you have a single activity app, calling finish(); should destroy the only activity in your app, thus quitting it at the first opportunity.

Redundant return statements at the end of void functions.

Most of your functions have the statement return; in them at the end. This is completely redundant and serves no use.

Redundant final keyword in private method declarations

Your private methods are not going to be overridden or anything, they don't need the final keyword.

private: This method cannot be overridden by a subclass

final: This method cannot be overridden by a subclass

private final: This method cannot be overridden by a subclass and this method cannot be overridden by a subclass (Redundant)

---

Optional:

Refactor away your turnOn and turnOff functions.

You currently call two different functions that do a roughly similar thing depending on whether you toggle the flashlight on or off.

Instead of the following:

        if (!FlashlightState) turnOnFlashlight();
        else turnOffFlashlight();

You could do something like:

toggleFlashLight();

In toggle flashlight you could do:

private void toggleFlashLight() {

  FlashlightState = !FlashlightState;

  if(FlashlightState) {
    //Turn it on;
  } else {
    //Turn it off;
  }
}

There's a dozen other ways to design it. (Where do you want to do the actual toggling of your boolean, etc. But this is just one example)

Answer 2

This is on top of what @ZeroStatic already said.

---

These mutable static variables are deeply troubling:

private static boolean FlashlightState;
private static long BackPressed;
private static ImageView Lamp;

Why are these static? Do you intend to run multiple instances of this activity at the same time? What for? I suggest to make all of these non-static.

---

What is this about:

AudioManager VibratorState = (AudioManager) getApplicationContext().getSystemService(Context.AUDIO_SERVICE);
// ...
VibratorState = null;

First of all, you call an audio service VibratorState, which is completely misleading, and then why do you set it to null? Is there a documentation somewhere that recommends this practice? If not, it's reasonable to assume that the variable will be garbage collected normally.

---

This is pointless:

finish();
this.onDestroy();

It's enough to call finish(), the framework will call onDestroy.

---

It's not recommended to include texts like this inside the Java code of Android apps:

    AboutBox.setMessage("Name:\nVisionlight\n\nDescription:\nFlashlight application for Android based devices.\n\nVersion:\n1.0.0\n\nPlatform:\nAndroid +2.2.x(Froyo)\n\nPermissions:\nCamera, Flashlight, Vibrate, Wake Lock.\n\nUses-feature:\nCamera [NR], Flashlight [NR].\n\nProducer/Developer:\nYousha Aleayoub(Y.P.Y)\n\nLicense:\nBSD\n\nContact:\[email protected]\n\nLink:\nhttp://yousha.blog.ir/");

This belongs to strings.xml.

Also, in general, don't put such extremely long lines in code. As much as possible, it's to keep lines short enough to fit within a window. You could easily break this into 4 and it would have been easier to read without having to scroll so much to the right.

---

Use CamelCase for class names. But main has an even bigger problem that it doesn't tell anything about the class. The common convention is to name activities as SomethingActivity, so in your case FlashlightActivity is an obvious choice.

---

Use all capital letters for static constants only, so instead of:

private Flashlight FLASHLIGHT = null;

Should be:

private Flashlight flashlight = null;