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

Revision history for this message
Matt Layman (mblayman) wrote :

Samuel,

Here are my comments:
 * 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.
 * 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.
 * I like the one lining of the quick constant labels like "Directed by." That was a nice touch to clean things up.

Thanks,
Matt

review: Approve

« Back to merge proposal