Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I just built an audio player in Angular 2 using a player component and a player service. It's all working fine, I just feel like there is a much better way to do this.

Should the audio object be in the service or the component? I'm skeptical because I'm using three different observables and I don't think that is the best way to do it.

player.component.ts:

export class PlayerComponent implements OnInit, OnDestroy {

  // General variables
  private song: Song;
  private currentTime: string;
  private fullTime: string;
  private isPlaying: boolean;
  // Subscription variables
  private songSubscription: any;
  private currentTimeSubscription: any;
  private fullTimeSubscription: any;

  constructor(private _playerService: PlayerService) {
  }

  ngOnInit() {
    this.songSubscription = this._playerService.song.subscribe(data => this.song = data);
    this.currentTimeSubscription = this._playerService.currentTime.subscribe(data => this.currentTime = data);
    this.fullTimeSubscription = this._playerService.fullTime.subscribe(data => this.fullTime = data);
    console.log("Player subscription initialized");
  }

  toggleAudio() {
    this.isPlaying = this._playerService.toggleAudio();
  }

  ngOnDestroy() {
    this.songSubscription.unsubscribe();
    this.currentTimeSubscription.unsubscribe();
    this.fullTimeSubscription.unsubscribe();
    console.log("Player subscription destroyed");
  }

}

player.service.ts:

export class PlayerService {

  private audio: any;
  public song: Subject<Song> = new Subject<Song>();
  public currentTime: Subject<string> = new Subject<string>();
  public fullTime: Subject<string> = new Subject<string>();


  constructor(private _utilityService: UtilityService) {
    this.audio = new Audio();
  }

  setPlayer(song: Song) {
    this.song.next(song);
    this.audio.src = song.audio;
    this.audio.oncanplaythrough = () => {
      this.audio.play();
      this.fullTime.next(
        this._utilityService.getFormatedTime(this.audio.duration)
      );
    };
    this.audio.ontimeupdate = () => {
      this.currentTime.next(
        this._utilityService.getFormatedTime(this.audio.currentTime)
      );
    };
  }

  toggleAudio() {
    if (this.audio.paused) {
      this.audio.play();
    } else {
      this.audio.pause();
    }

    return this.audio.paused;
  }

}

player.component.html:

<ul *ngIf="song" class="player">
  <li class="player-item">
    <a class="player-link" (click)="toggleAudio()">
      <i *ngIf="isPlaying"  class="fa fa-play" aria-hidden="true"></i>
      <i *ngIf="!isPlaying" class="fa fa-pause" aria-hidden="true"></i>
    </a>
  </li>
  <li class="player-item">
    <a class="player-link"><i class="fa fa-volume-up" aria-hidden="true"></i></a>
  </li>
  <li class="player-item">
    <a class="player-link">{{currentTime}}</a>
  </li>
  <li class="player-item">
    <a class="player-link"><i class="fa fa-fast-backward" aria-hidden="true"></i></a>
  </li>
  <li class="player-desc">
    <a class="player-link">{{song.title}} by {{song.artist}}</a>
  </li>
  <li class="player-item">
    <a class="player-link"><i class="fa fa-fast-forward" aria-hidden="true"></i></a>
  </li>
  <li class="player-item">
    <a class="player-link">{{fullTime}}</a>
  </li>
  <li class="player-item">
    <a class="player-link"><i class="fa fa-plus" aria-hidden="true"></i></a>
  </li>
  <li class="player-item">
    <a class="player-link"><i class="fa fa-share" aria-hidden="true"></i></a>
  </li>
</ul>

It's a pretty primitive player right now. I want to make sure I'm implementing it correctly before I add more features.

share|improve this question
    
I think it is correct. Im doing the same thing. But im using Event (event emitter) to send Player Service info to other components. – Paulo Coutinho Nov 11 '16 at 20:44

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Browse other questions tagged or ask your own question.