CODEWITHSUNDEEP
objective-cnsstring
James Raitsev
James Raitsev

Reputation: 96391

Objective-C, Constructing a String, code review

This code works fine, but each time i look at it, i die a little bit inside. :(

Can you help me streamline it a bit?

Is there a more elegant way to .append(SomeString)? (To give you some perspective, code prints elements of the Linked List)

- (NSString *) description {      
    Node* tempNode = [self firstNode];

    if (tempNode == nil) {
        return @"List contains no elements";
    }

    NSString *desc= [[NSString alloc] initWithString:@"(null) -- "];
    desc = [desc stringByAppendingString:[firstNode nodeCharacter]];
    desc = [desc stringByAppendingString:@" -- "];

    while ([tempNode nextNode] != nil) {
        desc =  [desc stringByAppendingString:[[tempNode nextNode]nodeCharacter]];
        desc =  [desc stringByAppendingString:@" -- "];
        tempNode = [tempNode nextNode];
    }

    return [desc stringByAppendingString:@" (null)"];


}

Upvotes: 1

Views: 573

Answers (5)

AliSoftware
AliSoftware

Reputation: 32681

  • First of all, if you want to build a string step by step or modify it, stop using stringByAppendingString:, and use NSMutableString instead of NSString !!!

  • Then, for your matter, you can even use stringWithFormat: to build part of your string.

  • Finally, you forgot to manage your memory: you alloc/init your string but never release it (and as your reassign the desc variable the line after, you loose track of the allocated memory and have a leak.

So here is the revised code:

- (NSString *) description {      
  Node* tempNode = [self firstNode];

  if (tempNode == nil) {
    return @"List contains no elements";
  }

  NSMutableString *desc = [NSMutableString stringWithFormat:@"(null) -- %@ -- ",[firstNode nodeCharacter]];
  while ((tempNode = [tempNode nextNode]) != nil) {
    [desc appendFormat:@"%@ -- ",[tempNode nodeCharacter]];
  }
  [desc appendingString:@" (null)"];

  return desc;
}

(Note that you may also build an NSArray of the nodes of your list and use componentsJoinedByString then at the end… so you have multiple possibilities here anyway)

Upvotes: 3

kevboh
kevboh

Reputation: 5245

Have you looked at NSMutableString? It has -appendString: methods.

Edit: You could also use a recursive function on your node to traverse the list and build up the string. I would make some simple public method like - (NSString *)description to call the first method and then use a private method internally to do your dirty work, like so:

- (NSString *)recursiveDescriptionWithSubnode:(Node *)node {
    if(!node) {
        return [self nodeCharacter];
    }
    else {
        return [[self nodeCharacter] stringByAppendingString:[self recursiveDescriptionWithSubnode:[self nextNode]];
    }
}

Note that this isn't tail recursive and so for a long list this would build up a sizable autorelease pool and call stack. Making it tail recursive is left as an exercise for the reader (but you could use NSMutableString to do it).

Upvotes: 1

Mark Granoff
Mark Granoff

Reputation: 16938

Instead of building up a string, you could create an array of strings, that you ultimately return as a single, joined string separating each value with your " -- " string.

If you know how many elements you might have, you could create the array with that capacity, which might be slightly more efficient under the hood for NSMutableArray.

Upvotes: 0

FeifanZ
FeifanZ

Reputation: 16316

Rather than using the while loop, take a look at Cocoa's Fast Enumeration. It is supported by NSArrays already, and allow you to rapidly enumerate through array elements:

for (id object in myArray)
    // Do something with object

You can adopt fast enumeration (or use NSEnumerator) in your node elements, then loop through them:

// Use mutable strings
NSMutableString *desc = [[NSMutableString alloc] initWithString:@"(null) -- "];
for (Node *node in nodeList) {
    [desc appendString:[node nodeCharacter];
    [desc appendString:@" -- "];
}
return [desc stringByAppendingString:@" (null)"];

Upvotes: 1

Joe
Joe

Reputation: 57169

Yes use an NSMutableString which will involve much less memory allocations.

- (NSString *) description {      
    Node* tempNode = [self firstNode];
    if (tempNode == nil) {
        return @"List contains no elements";
    }

    //Autorelease string to prevent memory leak
    NSMutableString *desc= [NSMutableString stringWithString:@"(null) -- "];
    [desc appendString:[firstNode nodeCharacter]];
    [desc appendString:@" -- "];

    while ([tempNode nextNode] != nil) {
        [desc appendString:[[tempNode nextNode]nodeCharacter]];
        [desc appendString:@" -- "];
        tempNode = [tempNode nextNode];
    }

    [desc appendString:@" (null)"];
    return desc;
}

Upvotes: 1

Related Questions