Note that there are some explanatory texts on larger screens.

plurals
  1. PO
    primarykey
    data
    text
    <p>Since it already is tomorrow:<br> <strong>Happy Birthday!</strong></p> <p>Now, you do have a couple more issues than <strong>one</strong> leak.</p> <h3>The first one</h3> <p>is in your <code>initWithNibName:bundle:</code> as you aren't doing anything useful there: get rid of it, entirely! (Besides: don't release arguments, that are passed to your methods! That gang rapes little kittens. Luckily, you've placed those releases in lines that are unreachable, i.e. after the return statement...)</p> <h3>Next method, next problems</h3> <ol> <li>Why are you sending <code>release</code> to a class object? Don't! That's wrong on <em>many</em> levels.</li> <li>You have decided to create properties for your timers. That's nothing bad per se. But why then, are you using the ivars directly here? I'd strongly encourage you to implement <code>setTheTimer:</code> and <code>setBackgroundTimer:</code> to handle the invalidation and release properly and simply do <code>self.theTimer = nil; self.backgroundTimer = nil;</code> here. That would fix the asymmetry in handling those things, as well. (By the way: theTimer isn't <em>such</em> a great name for an ivar...especially when there is <strong>another</strong> ivar that is a timer!)</li> </ol> <p><code>textInstructions:</code> looks unsuspicious but...</p> <h3><code>viewDidLoad</code> has some more issues</h3> <ol> <li>It leaks an MPMoviePlayerController:<br> The <code>@property</code> will retain it, so you need to balance the <code>alloc</code> here.</li> <li><code>backgroundTimer</code> has a corresponding <code>@property</code> that is declared to be retaining: You are violating this API contract here, since you only assign the timer to the ivar. Use <code>self.backgroundTimer = ...</code> instead.</li> <li>From all the code you posted, it seems to me that passing <code>theTimer</code> as the last argument in your call to <code>-[NSNotificationCenter addObserver:selector:name:object:]</code> is a fancy way of passing in <code>nil</code> as that parameter. Which is kind of good, because usually <code>NSTimer</code> doesn't post too many <code>MPMovieDurationAvailableNotification</code>s. In fact, as I can't see <code>theTimer</code> being used except in <code>close:</code>: Could it be that this is just a useless remnant from before you introduced the <code>backgroundTimer</code> ivar/@property? (Well there is another occurrence of a variable of that name, but it should be accompanied by a big fat compiler warning...)</li> <li>Do you, in any way, implement <code>viewDidUnload</code>? If so, does it: <ol> <li><code>self.player = nil;</code>?</li> <li><code>[shoutOutTexts release], shoutOutTexts = nil;</code>?</li> <li><code>[shoutOutTimes release], shoutOutTimes = nil;</code>?</li> <li><code>self.keyframeTimes = nil;</code>?</li> <li><code>[[NSNotificationCenter defaultCenter] removeObserver:self name: MPMovieDurationAvailableNotification object:nil];</code>?</li> <li><code>self.backgroundTimer = nil;</code>? (Assuming, <code>setBackgroundTimer:</code> releases <strong>and invalidates</strong> the old value)</li> </ol></li> <li><strong>Update</strong> I've missed this one on the first go: You are leaking 15 <code>NSNumber</code>s here. Use <code>[NSNumber numberWithInt:]</code> instead of alloc/init in the setup of <code>shoutOutTimes</code>.</li> </ol> <p>A minor remark on <code>movieURL</code>, which you can turn into the following one-liner:</p> <pre><code>-(NSURL*)movieURL { return [[NSBundle mainBundle] URLForResource:@"sirloin" withExtension:@"m4v"]; } </code></pre> <h3>And then this</h3> <p><code>NSTimeInterval lastCheckAt = 0.0;</code> within the global scope. From your usage of it: ivar PLZ?!?one?</p> <p>More issues later. I gotta get myself something to eat first.</p> <hr> <h2>Part Two</h2> <h3>Now let's get into <code>timerAction:</code></h3> <p>The first issue is not too grave, — especially in this particular context — but you should be aware that <code>-[NSArray count]</code> returns an <code>NSUInteger</code> and that the <em>U</em> is not a typo, but a designation that this value is unsigned. You certainly won't run into problems with the signedness in this App and rarely do on other occasions, but <em>when</em> you do, they make up for really funky bugs and you should be aware of the implications...<br> The real issue with this method is, however, that you are <strong>leaking one <code>CommentView</code> per iteration</strong> while — at the same time — <strong>overreleasing one <code>NSString</code>.</strong> The fact, that you were using string literals (which will never be dealloced) in the first place, (i.e. when you were initializing shoutOutTimes) totally saves your butt, here.</p> <h3>Next up: <code>removeObserver:forKeyPath:</code></h3> <p>You <strong>really</strong> should get rid of that really bad habit of releasing parameters, that are passed to your methods!</p> <p>That being said, get rid of the entirety of this method!</p> <p>First and foremost <code>removeObserver:forKeyPath:</code> is a method from the <code>NSKeyValueObserving</code> informal protocol and plays a totally different role than what you are (ab-)using it to accomplish here. Secondly, it is one of those methods where it is essential to call through to <code>super</code> if — by any means — you <em>really</em> need to override it. (Well, except, when you were overriding <code>addObserver:forKeyPath:options:context:</code> as well and-it-should-go-without-saying-that-you-shouldn't-do-that-unless-you-really-know-what-you-are-doing-if-you-ever-planned-on-using-KVO.)</p> <h3><code>movieDurationAvailable:</code></h3> <p>Like Evan said, you're leaking <code>times</code> here. Go for his suggestion or — instead — make it <code>NSMutableArray *times = [NSMutableArray array];</code> and you are done here.</p> <h3><code>playerThumbnailImageRequestDidFinish:</code></h3> <p>You don't own <code>image</code>, so don't release it!<br> Personally, I would finish setting up the view (i.e. add the recognizer and do stuff like that) before adding it into the view-hierarchy, but that's completely a matter of taste...</p> <h3><code>makeThumbnailImageViewFromImage:andTimeCode:</code></h3> <p>...leaks an <code>NSNumber</code> (use <code>[NSNumber numberWithFloat:(pos * timeslice)]</code> instead of the <code>alloc/initWithFloat:</code>-dance) and prevents you from crashing due to overreleasing <code>myScrollView</code> by the unconditional return statement that directly precedes it (phew!). While we're at it: rename this method to either <code>newThumbnailImageView...</code> so that when you'll revisit this code in a year or so, you instantly know that <code>[imageView release];</code> at the bottom of <code>playerThumbnailImageRequestDidFinish:</code> really is necessary, without having to look at the implementation of this method.<br> Alternatively, you could rename it to <code>thumbnailImageView...</code> and change the return statement to <code>return [imageView autorelease];</code>. Bonus: one fewer line in <code>playerThumbnailImageRequestDidFinish:</code> as the <code>[imageView release];</code> there becomes obsolete then.</p> <h3><code>dealloc</code></h3> <p>Add <code>[[NSNotificationCenter defaultCenter] removeObserver:self];</code> at the very top.</p> <p>The rest of it looks okay. (Although I find it odd, that the ivar <code>landscapeView</code> is never/nowhere mentioned apart from its declaration.)</p> <h3>Summary</h3> <p>Read the sections <em><a href="http://developer.apple.com/library/ios/documentation/Cocoa/Conceptual/MemoryMgmt/Articles/mmRules.html#//apple_ref/doc/uid/20000994-BAJHFBGH" rel="noreferrer">Memory Management Rules</a></em> and <em><a href="http://developer.apple.com/library/ios/documentation/Cocoa/Conceptual/MemoryMgmt/Articles/mmObjectOwnership.html#//apple_ref/doc/uid/20000043-SW5" rel="noreferrer">Autorelease</a></em> from Apple's "Memory Management Programming Guide" once again. They're pure gold!</p>
    singulars
    1. This table or related slice is empty.
    plurals
    1. This table or related slice is empty.
    1. This table or related slice is empty.
    1. This table or related slice is empty.
    1. VO
      singulars
      1. This table or related slice is empty.
    2. VO
      singulars
      1. This table or related slice is empty.
    3. VO
      singulars
      1. This table or related slice is empty.
 

Querying!

 
Guidance

SQuiL has stopped working due to an internal error.

If you are curious you may find further information in the browser console, which is accessible through the devtools (F12).

Reload