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(SomeStrin开发者_StackOverflowg)? (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)"];


}

• 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)

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;
}

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)"];

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).

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.