Code review comment for lp:~samuel-buffet/entertainer/bug_380745

Revision history for this message
Samuel Buffet (samuel-buffet) wrote :

Matt hi,

> * Actors could have as many as 5 actors (according to
> entertainerlib.medialibrary.videos.Movie) so you'll need to slice the
> actor_list to actor_list[:5] instead of actor_list[:3] or else we may lose
> actors. It looks like you were just indenting this line of code, but this is
> just something I happened to notice.
> * Just like above, but with directors and in the opposite direction. There
> will only ever be two directors according to the Movie class so the slice
> should probably just be to :2.
> * Same story with writers. There will only ever be two, according to the
> Movie class.

Yep that's right. I've committed those changes.

> * Did you think about making a common method since this seems to be a
> repeated pattern here? Maybe it's not worth it with because of the number of
> size and position variables, but I was just curious if you considered it.

This is something I constantly try to do when it reduces code drastically or makes it more readable.

Thanks for the review.
Samuel-

« Back to merge proposal