Code review comment for lp:~rockstar/ubuntuone-ios-music/replace-streaming-player

Revision history for this message
Mike McCracken (mikemc) wrote :

Needs-fixing because of the currentIndex issues in UOPlayer, otherwise please treat my naming comments as friendly suggestions, I won't complain if you decide you don't like them.

General comment: this really could have been split into a few smaller
branches to ease reviewing. For example, these all seem like
independent changes:

- adding the CGImage reflection
- updating duration to int64
- the secondsToClock change
- adding UOPlayer
- maybe also the XIB changes, if only to de-clutter the other branches

1. NSNumber+UbuntuOne:

Why is secondsToClock a category on NSNumber? NSNumber isn't
involved. Why not just make it a function?

Or if you really want a category, maybe a category on NSString
like "+clockStringFromSeconds:"...

2. UIImage+UbuntuOne: (~MINOR)

reflectedImage:(1) toImageView:(2) wasn't clear from the name what it
does. For one, both args are image views but the name makes it sound
like they're different. Also I think it needs a verb, like
"setImageView:(2) toReflectionOfImageView:(1)" or something.

Just my 2c, it's up to you.

3. UOPlayer.m:

In -next, I think that the two tests of (currentIndex +1 >
[... count]) might be off by one - if there are ten tracks and I'm on
track #10 (index 9), this will evaluate (10 > 10) = false and set
currentIndex to 10, which will be an invalid index into songs.

In prev, if currentIndex is 0, won't we get an invalid index there
too? I'm not 100% sure what unsigneds do when you do 0-1, but it's
probably not a good idea anyway. What should it do in that case?
Actually, prev looks unfinished, right? Should it always set the state
to stop?

addSongs:atIndex: was a confusing name, and it made it hard to understand
prepareForSegue in the view controller, because I was wondering what
happened to the indexing if there were already songs in the player
when you called it from prepareForSegue...
But - we're not *adding* songs, we're *replacing* the songs array
completely. I think the method would be much clearer if it was named
something like "setSongs:withStartingIndex:".

(MINOR): IMO, enums are more readable with underscores: e.g. REPEAT_OFF

review: Needs Fixing

« Back to merge proposal