3
\$\begingroup\$

I wanted to create a very basic application for Android that plays an audio file based on a URL input. I would like it to be reviewed based on good programming practices. This is the first time that I have used the MediaPlayer class. It works as I would like it to, but I am sure my code can be optimized and improved.

Following is my code for the MediaPlayerActivity class that I have built:

public class MediaPlayerActivity extends AppCompatActivity {

    TextView tvArtistName, tvSongName;
    ImageView ivPlay, ivPrev, ivNext, ivPause;

    MediaPlayer mediaPlayer = new MediaPlayer();

    private double startTime = 0;
    private double finalTime = 0;

    private Handler myHandler = new Handler();
    ;
    private int forwardTime = 5000;
    private int backwardTime = 5000;
    private SeekBar seekbar;
    public static int oneTimeOnly = 0;

    ActionBar actionBar;

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_media_player);
        actionBar = getSupportActionBar();
        actionBar.hide();

        tvArtistName = (TextView) findViewById(R.id.tvArtistName);
        tvSongName = (TextView) findViewById(R.id.tvSongName);
        ivPlay = (ImageView) findViewById(R.id.iv_play);
        ivPause = (ImageView) findViewById(R.id.iv_pause);
        ivNext = (ImageView) findViewById(R.id.iv_next);
        ivPrev = (ImageView) findViewById(R.id.iv_prev);
        seekbar = (SeekBar) findViewById(R.id.seekBar);
        seekbar.setClickable(false);


        Intent intent = getIntent();
        if (intent.getExtras() != null) {
            final String url = intent.getStringExtra("url");
            String song = intent.getStringExtra("song");
            String artist = intent.getStringExtra("artist");

            tvArtistName.setText(artist);
            tvSongName.setText(song);

            mediaPlayer.setAudioStreamType(AudioManager.STREAM_MUSIC);
            mediaPlayer = MediaPlayer.create(getApplicationContext(), Uri.parse(url));


            ivPlay.setOnClickListener(new View.OnClickListener() {
                @Override
                public void onClick(View view) {
                    // Toast.makeText(getApplicationContext(), "Playing sound", Toast.LENGTH_SHORT).show();
                    if (mediaPlayer.isPlaying()) {
                        mediaPlayer.stop();
                        mediaPlayer.reset();
                    }
                    mediaPlayer.setOnCompletionListener(new MediaPlayer.OnCompletionListener() {
                        @Override
                        public void onCompletion(MediaPlayer mediaPlayer) {
                            mediaPlayer.stop();
                            mediaPlayer.reset();
                        }
                    });
                    mediaPlayer.start();
                    finalTime = mediaPlayer.getDuration();
                    startTime = mediaPlayer.getCurrentPosition();

                    if (oneTimeOnly == 0) {
                        seekbar.setMax((int) finalTime);
                        oneTimeOnly = 1;
                    }

                    seekbar.setProgress((int) startTime);
                    myHandler.postDelayed(UpdateSongTime, 100);
                    ivPlay.setVisibility(View.GONE);
                    ivPause.setVisibility(View.VISIBLE);

                }
            });

            ivPause.setOnClickListener(new View.OnClickListener() {
                @Override
                public void onClick(View view) {
                    Toast.makeText(getApplicationContext(), "Pausing sound", Toast.LENGTH_SHORT).show();
                    mediaPlayer.pause();
                    ivPause.setVisibility(View.GONE);
                    ivPlay.setVisibility(View.VISIBLE);
                }
            });

            ivNext.setOnClickListener(new View.OnClickListener() {
                @Override
                public void onClick(View view) {
                    int temp = (int) startTime;

                    if ((temp + forwardTime) <= finalTime) {
                        startTime = startTime + forwardTime;
                        mediaPlayer.seekTo((int) startTime);
                        //Toast.makeText(getApplicationContext(),"You have Jumped forward 5 seconds",Toast.LENGTH_SHORT).show();
                    } else {
                        //Toast.makeText(getApplicationContext(),"Cannot jump forward 5 seconds",Toast.LENGTH_SHORT).show();
                    }
                }
            });

            ivPrev.setOnClickListener(new View.OnClickListener() {
                @Override
                public void onClick(View view) {
                    int temp = (int) startTime;

                    if ((temp - backwardTime) > 0) {
                        startTime = startTime - backwardTime;
                        mediaPlayer.seekTo((int) startTime);
                        //Toast.makeText(getApplicationContext(),"You have Jumped backward 5 seconds",Toast.LENGTH_SHORT).show();
                    } else {
                        //Toast.makeText(getApplicationContext(),"Cannot jump backward 5 seconds",Toast.LENGTH_SHORT).show();
                    }

                }
            });
        }
    }

    @Override
    public void onBackPressed() {
        super.onBackPressed();
        mediaPlayer.release();
        mediaPlayer = null;
        seekbar.setProgress(0);
        startTime = 0;
        finalTime = 0;
    }

    private Runnable UpdateSongTime = new Runnable() {
        public void run() {
            if (mediaPlayer!=null)
            {
                startTime = mediaPlayer.getCurrentPosition();
                seekbar.setProgress((int) startTime);
                myHandler.postDelayed(this, 100);
            }
        }
    };
}
\$\endgroup\$
1
\$\begingroup\$

It looks good to me. Sensible identifiers, usual idioms.

After defining myHandler, delete the 2nd ; semicolon.

You probably want a boolean for oneTimeOnly.

Your "Pausing sound" toast is nice enough, but consider defining a TextView where you can flash messages, as that can give you greater thread scheduling control. A toast can block other threads from running for a moment, which will sometimes be important to an app.

If intent.getExtras() is null, it seems the caller did the Wrong Thing, so you might want to give up and signal fatal error at that point.

\$\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.