6
\$\begingroup\$

I want to implement basic audio functions like play, stop and pause. I have stuffed all the code inside onCreate method. Is this best practise to follow?

public class MainActivity extends Activity {  
        Button start,pause,stop;  
        @Override  
        protected void onCreate(Bundle savedInstanceState) {  
            super.onCreate(savedInstanceState);  
            setContentView(R.layout.activity_main);  

            start=(Button)findViewById(R.id.button1);  
            pause=(Button)findViewById(R.id.button2);  
            stop=(Button)findViewById(R.id.button3);  
            //creating media player  
            final MediaPlayer mp=new MediaPlayer();  
            try{  
                    //you can change the path, here path is external directory(e.g. sdcard) /Music/maine.mp3  
            mp.setDataSource(Environment.getExternalStorageDirectory().getPath()+"/Music/maine.mp3");  

            mp.prepare();  
            }catch(Exception e){e.printStackTrace();}  

            start.setOnClickListener(new OnClickListener() {  
                @Override  
                public void onClick(View v) {  
                    mp.start();  
                }  
            });  
            pause.setOnClickListener(new OnClickListener() {  
                @Override  
                public void onClick(View v) {  
                    mp.pause();  
                }  
            });  
            stop.setOnClickListener(new OnClickListener() {  
                @Override  
                public void onClick(View v) {  
                    mp.stop();  
                }  
            });  
        }  
    } 
\$\endgroup\$
6
\$\begingroup\$

You could let your class implement OnClickListener

public class MainActivity extends Activity implements OnClickListener{
    ...
}

and then use the onClickfunction like this:

public void onClick(View v)
{
    switch (v.getId())
    {
        case R.id.button1:
            mp.start();  
            break;
        case R.id.button2:
            mp.pause();  
            break;
        case R.id.button3:
            mp.stop();  
            break;
        default:
            break;
    }
}

That way you have all actions that happen on a press of a button in the same place in your program.

You should also change the ID to something more meaningful, eg. play, pause, stop. This makes your code far more maintainable and easier to understand which Button does what just by skimming the code of the MainActivity(which you also might want to rename to something like MediaPlayerActivity incase you decide to add additional Activity classes, for example to select a new song.

Additionally I would advise you to change the visibility of the Buttons to private, rather than default.

\$\endgroup\$
  • \$\begingroup\$ via your solution if we change button to image then also no need for refactoring of code is required ! ! Thanks for the review, much appreciated \$\endgroup\$ – Ankit Garg Aug 22 '14 at 8:16
4
\$\begingroup\$

Not only can you have your class implement the interface, but you can also use an onClick property in your XML. See onClick in XML vs. onClickListener

In onCreate:

start = (Button) findViewById(R.id.startButton);

New method in your class:

public void startButtonPress(View view) {
    mp.start();
}

In your XML:

<button .... onClick="startButtonPress" />

Additionally, you should look over your indentation and spacing.

This part of your code:

        try{  
                //you can change the path, here path is external directory(e.g. sdcard) /Music/maine.mp3  
        mp.setDataSource(Environment.getExternalStorageDirectory().getPath()+"/Music/maine.mp3");  

        mp.prepare();  
        }catch(Exception e){e.printStackTrace();}  

Is more readable as:

        try {  
            // you can change the path, here path is external directory(e.g. sdcard) /Music/maine.mp3
            mp.setDataSource(Environment.getExternalStorageDirectory().getPath() + "/Music/maine.mp3");  
            mp.prepare();  
        } catch (Exception e) {
            e.printStackTrace();
        }

Additionally:

  • Think about the users of your app, do all of them have a /Music/maine.mp3 file? No. I don't know what your main goal with this app is though, but playing only one specific file that has to be at a certain place hopefully isn't it.
  • Again, think about the users, can they read the output from e.printStackTrace(); with adb logcat? No. I'd recommend showing a Toast or a Dialog there as well.
\$\endgroup\$
  • \$\begingroup\$ If we try to handle via xml only then I don't think there is any need for this "start = (Button) findViewById(R.id.startButton);" line of code. As i have read that performance wise both the approaches are same so what could be real advantages of each approach i.e handle click event via xml or through java code only ? \$\endgroup\$ – Ankit Garg Aug 23 '14 at 3:37
  • \$\begingroup\$ @AnkitGarg The only benefit is that you'll have less code in the constructor. \$\endgroup\$ – Simon Forsberg Aug 23 '14 at 15:07

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.