Skip to content

Instantly share code, notes, and snippets.

@DavidBruant
Forked from julienw/timerj-review.md
Created November 28, 2012 15:01
Show Gist options
  • Save DavidBruant/4161820 to your computer and use it in GitHub Desktop.
Save DavidBruant/4161820 to your computer and use it in GitHub Desktop.
code review de timerjs

j'ai juste regardé l'API pour l'instant, j'ai quelques remarques déjà là-dessus, je regarderai le code plus tard.

  • l'argument à "play" est pas clair. Je pense que tu peux faire une configuration fluide du style :

timer.speed(2).play(); ou même timer.play().speed(2); ou timer.speed(2); timer.play();

Suffit que toutes les méthodes retournent timer.

  • je préférerais que "stop()" fasse la même chose que ton play(0) (et mon speed(0) donc ;) ) Et un "resume" ferait la même chose qu'un play(oldSpeed) (ou speed(oldSpeed) avec ma proposition d'API)

  • pour revenir à 0, que dirais-tu d'une méthode "reset()" ? ton "stop" actuel, ce serait alors "timer.stop().reset()".

  • toujours sur la conf fluide, on peut gérer duration/etc comme ça aussi :

var timer = new Timer().duration(5000).delay(1000).

Et pour récupérer une valeur, au lieu d'une propriété, ce serait la même méthode :

timer.duration() timer.delay()

  • tiens, à la place de delay, ce pourrait être "steps" parce que "delay" ça donne l'impression que c'est le temps avant que ça commence, et non pas les durées entre chaque saut.

  • je me dis aussi qu'on n'a pas besoin de ce "new". On devrait pouvoir créer une nouvelle instance juste avec Timer(): var timer = Timer().duration(xxx); C'est assez facile à faire si tu veux conserver la possibilité de faire le new (jette un oeil au code de jQuery), et encore plus facile si tu veux pas garder cette possibilité.

J'ai regardé un tout ptit peu le code:

  • ça m'embête qu'il y ait des effets de bord; genre quand on fait "get" sur certaines propriétés (is et position), en fait, ça fait plein de choses. Mais c'est peut-être voulu car tu voulais pas de setTimeout, c'est ça ?

  • j'enlèverais complètement ton "this.set" pour accéder aux variables closed. Par contre j'aime bien la manière que t'as eu de définir ces variables, ça encapsule bien leur fonctionnement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment