Class works not correct. How to make it better?

Question

I have written class and want show it to you ...

I think that this class written not correct and thats why I have leeks in my application. First of all the delloc never calls. What can I change in this class to make it better, please help.

Articles.h

#import <Foundation/Foundation.h>

@interface Article : NSObject {
    BOOL favorite;
    NSMutableString * title;  
    NSMutableString * summary;  
    NSMutableString * mainLink;  
    NSMutableString * pubDate;  
    NSMutableString * author;  
    NSMutableString * imageLink;  
}

@property (nonatomic, assign) BOOL favorite;  
@property (nonatomic, retain) NSMutableString * title;  
@property (nonatomic, retain) NSMutableString * summary;  
@property (nonatomic, retain) NSMutableString * mainLink;   
@property (nonatomic, retain) NSMutableString * pubDate;  
@property (nonatomic, retain) NSMutableString * author;  
@property (nonatomic, retain) NSMutableString * imageLink;  

- (id)initWithValues:(NSString *) inTitle mainLink:(NSString *) inMainLink summary:(NSString *) inSummary
             pubDate:(NSString *) inPubDate author:(NSString *) inAuthor imageLink:(NSString *) inImageLink;


//Setter methods
- (void)setTheTitle:(NSString *) inTitle;
- (void)setTheMainLink:(NSString *) inMainLink;
- (void)setTheSummary:(NSString *) inSummary;
- (void)setThePubDate:(NSString *) inPubDate;
- (void)setTheAuthor:(NSString *) inAuthor;
- (void)setTheImageLink:(NSString *)inImageLink;

@end

Articles.m

#import "Articles.h"

@implementation Article

@synthesize favorite;  
@synthesize title;  
@synthesize summary;  
@synthesize mainLink;
@synthesize pubDate;  
@synthesize author;  
@synthesize imageLink;  

- (void)dealloc {
    NSLog(@"article dealloc \n");
    [self.title release];
    [self.mainLink release];
    [self.summary release];
    [self.pubDate release];
    [self.author release];
    [self.imageLink release];

    [super dealloc];
}



- (id)init {

    self = [super init];
    if(self) {
      // set your properties...
      self.title     = [[[NSMutableString alloc] init] autorelease];
      self.mainLink  = [[[NSMutableString alloc] init] autorelease];
      self.summary   = [[[NSMutableString alloc] init] autorelease];
      self.pubDate   = [[[NSMutableString alloc] init] autorelease];
      self.author    = [[[NSMutableString alloc] init] autorelease];
      self.imageLink = [[[NSMutableString alloc] init] autorelease];
      self.favorite = NO;
    }
    return self;
}

- (id)initWithValues:(NSString *) inTitle mainLink:(NSString *) inMainLink summary:(NSString *) inSummary
            pubDate:(NSString *) inPubDate author:(NSString *) inAuthor imageLink:(NSString *) inImageLink
{
    self = [super init];
    if(self) {
        // set your properties ...
        if (inTitle != nil) {
            self.title = inTitle;
        }

        if (inMainLink != nil) {
            self.mainLink = inMainLink ;
        }

        if (inSummary != nil) {
            self.summary = inSummary;
        }

        if (inPubDate != nil) {
            self.pubDate = inPubDate;
        }

        if (inAuthor != nil) {
            self.author = inAuthor ;
        }

        if (inImageLink != nil) {
            self.imageLink = inImageLink ;
        }

        self.favorite = NO;
    }

    return self;
}


@end

ADDED:

Look I have NSXMLParser in my main class. In the main class .h file I write:

Article * currentArticle;

Now In .m file when parser didStartElement I alloc ant initialize Article in parser didEndElement I release it [self.currentArticle release]; but delloc not calles.

- (void) parser:(NSXMLParser *)parser didStartElement:(NSString *)elementName namespaceURI:(NSString *)namespaceURI 
                                        qualifiedName:(NSString *)qName attributes:(NSDictionary *)attributeDict {
    // Copy current Xml Element name.
    currentElement = [elementName copy];
    if ([elementName isEqualToString:@"item"]) {
        // Clear out our story item caches...
        self.currentArticle = [[Article alloc] init];
    }
    [currentElement release];

}

ADDED RELEASE FOR TEST

- (void) parser:(NSXMLParser *)parser didStartElement:(NSString *)elementName namespaceURI:(NSString *)namespaceURI 
                                        qualifiedName:(NSString *)qName attributes:(NSDictionary *)attributeDict {
    // Copy current Xml Element name.
    currentElement = [elementName copy];
    if ([elementName isEqualToString:@"item"]) {
        // Clear out our story item caches...
        self.currentArticle = [[Article alloc] init];
        [self.currentArticle release];
    }
    [currentElement release];

}

look I have added [self.currentArticle release]; right after initialization and put breakpoint here ... When at first time my application enters in this part of code it call init but not call release at second time it call release ? But why ? It's not right

WHY I DON'T USE AUTORELEASE !!!

 self.title     = [[[NSMutableString alloc] init] autorelease];
    self.mainLink  = [[[NSMutableString alloc] init] autorelease];
    self.summary   = [[[NSMutableString alloc] init] autorelease];
    self.pubDate   = [[[NSMutableString alloc] init] autorelease];
    self.author    = [[[NSMutableString alloc] init] autorelease];
    self.imageLink = [[[NSMutableString alloc] init] autorelease];

I do not use autorelease in this part of code because I have read that it brings to leeks, because when I write autorelease the objects releases in the end of application work ! Am I write ???

Thanks !!!

Answer 1

You can remove every setter like

- (void)setTheSummary:(NSString *) inSummary
{

    if (self.summary != nil) {
        [self.summary release];
    }

    self.summary = [[NSMutableString alloc] initWithString:inSummary];
}

You've declared every ivar as property already and synthesized getters and setters. So you can set the title for example with:

self.title = newTitle;

This will retatin the newTitle and assign it to title and release the previous(if present) value.

EDIT

If you set properties like

self.title = [[NSMutableString alloc] init];

the instande of the mutable string will get over retained, so there will be leaks.

Retain increases the retain count by 1, this happens through the property's declaration and it will go one up by calling init.

Change it to:

self.title = [[[NSMutableString alloc] init] autorelease];

EDIT 2

Change your initialization of these constructs:

if (inTitle == nil) {
    self.title = [[NSMutableString alloc] init];
}
else
{
    [self.title release];
    self.title = [[NSMutableString alloc] initWithString:inTitle];
}

To:

if (inTitle != nil) {
    self.title = inTitle;
}

Now add

self = [self init];

and remove

[super init];

at the beginning of your init method initWithValues; This will initialize the properties for you first, it reduces code duplication and makes your class smaller. The removal of [super init] is necessary to call the the initializer of NSObject only once, you are doing this by calling self = [self init];.

You've created with this pattern a so called designated initializer. You can read more on initializers here.

EDIT 3

To make your intializers perfect you should write them this way:

- (id)init 
{
    self = [super init];
    if(self) {
       // set your properties...
    }
    return self;
}

and

- (id)initWithValues:(NSString *) inTitle mainLink:(NSString *) inMainLink summary:(NSString *) inSummary
            pubDate:(NSString *) inPubDate author:(NSString *) inAuthor imageLink:(NSString *) inImageLink
{
    self = [self init];
    if(self) {
       // set properties with parameters ...
    }
    return self;
}

This pattern will allow you to react on errors which can occur in your designated initializer and/or in the initializers called up the inheritance hierarchy. This will ensure that a nil is returned if something goes wrong and you don't set properties to an erroneous instance.

Answer 2

While there are numerous things going wrong with your code, this is the part which causes memory leaks:

self.title = [[NSMutableString alloc] init];

Why? The title property is defined as:

@property (nonatomic, retain) NSMutableString * title;

Hence, the setter increases the retain count to 2.

In dealloc retain count for title is only decreased by one, so the object is still alive after deallocing the article.

Quick fix:

self.title = [[[NSMutableString alloc] init] autorelease];

Edit: this is why dealloc never gets called:

self.currentArticle = [[Article alloc] init];

Again, take a look at the retain count: alloc sets retain count to 1. Then - I assume the currentArticle is defined as (nonatomic, retain) - it is retained again, retain count is now 2. In the dealloc method - I assume again - you release currentArticle, so retain count is 1. An object won't dealloc unless retain count is 0!

Quick fix: same as above

self.currentArticle = [[[Article alloc] init] autorelease];

Answer 3

For starters I would replace every

   if (self.imageLink !=nil) {
        [self.imageLink release];
        self.imageLink = nil;
    }

in dealloc with

[imageLink release];

To help you fix the major issue, that is, why your instance never gets deallocated, we need to see the code where you create it.

Edit

Ok, here is a guess what might be happening. I hope you can use this guess to fix the issue (without seeing the actual code this is all I can do):

Somewhere in you code you have something like this:

Article *article = [[Article alloc] init];

You say you are calling release on the article so you will also have

[article release];

somewhere else in you code.

Now why is dealloc not called even though you call release? My guess is that you are doing something like this in between:

NSMutableArray *array = [NSMutableArray arrayWithCapacity: 0];
[array addObject: article];

This actually retains article and could be the reason why it never gets released. How to fix? Like this:

[array removeObject: article];

Hope this helps.