HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

LinkedList (doubly) implementation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
implementationdoublylinkedlist

Problem

CKLinkedList is the implementation of a doubly linked list. I'd like to see comments on the memory management (I'm using ARC in the code below) and formatting + overall style.

CKList protocol file:

``
//
// CKList.h
// CollectionsKit
//
// Created by Igor Rastvorov on 11/30/14.
// Copyright (c) 2014 Igor Rastvorov. All rights reserved.
//

#import
#import "CKCollection.h"
#import "CKQueue.h"

/**
CKList is a formal protocol for all the adopters that represent a heterogenous list of objects.
*/
@protocol CKList

@property(nonatomic, assign, readonly) NSUInteger size;

/// --------------------------------
/// @name Adding objects to the list
/// --------------------------------

/**
Adds object to the head of the list.
@param object An object to add to head.
*/
-(void) addObjectToHead:(id) object;

/// -----------------------------------
/// @name Getting objects from the list
/// -----------------------------------

/**
Retrieves object at the tail of the list.
@throws
NSRangeException if the list is empty.
*/
-(id) objectAtTail;

/**
Retrieves object at the specified position in the list.
@param index Specifies the position to remove object from.
@throws
NSRangeException if the list is empty.
*/
-(id) objectAtIndex:(NSUInteger) index;

/// ------------------------------------
/// @name Removing objects from the list
/// ------------------------------------

/**
Removes object from the head.

@throws
NSRangeException if the list is empty.
*/
-(void) removeObjectFromHead;

/**
Removes object from the tail.

@throws
NSRangeException if the list is empty.
*/
-(void) removeObjectFromTail;

/**
Removes object from the specified position in the list.

@param index Specifies the position to remove object from.
@throws
NSRangeException` if the list is empty.
*/
-(void) removeObjectAtIndex:(NSUInteger) index;

/**
Constructs a new list within a range of the original list.

@param range Range of objects within the original list.

Solution

@bazola's answer hits on some points I would otherwise make, so be sure to stop by there.

Meanwhile, in this answer...

First of all, I don't particularly like objectAtTail or objectAtHead as method names. I think that as a general guideline, we can make our linked list work very similarly to NSArray. As for a start, we should replace objectAt... with firstObject and lastObject.

And more importantly, rather than throwing an exception, our method should just return nil when the list is empty. The documentation pointing out that an exception can be thrown is nice, but here I don't think throwing the exception is appropriate. And Apple wouldn't think so either, otherwise, they'd have firstObject and lastObject throw exceptions instead of returning nil.

Your size property is missing documentation. Is this the memory size the entire list takes up? Or is it the count of the objects in the list? If the latter, we should rename it to count, and the property shouldn't miss out on the detail documentation the rest of the class gets.

isEqual methods should basically always be optimized with the following:

-(BOOL) isEqual:(id)object {
    if (self == object) return true;
    // additional logic to determine if they are the same
}


What this does is starts with a check to see if self and object are pointers to the same object. If our pointers hold the same value, it's impossible for us to be unequal--we're the exact same objects.

-(NSString *) description {
    return [[self data] description];
}


This can lead to some really misleading debugging. We need to wrap our data's description in a string that indicates the object is in the list.

- (NSString *)description {
    return [NSString stringWithFormat:@"[: %@]", self, self.data];
}


Our toArray method is documented as returning an immutable array but actually returns a mutable array. Moreover, I think I'd like the method name better if it were arrayValue or arrayRepresentation.

But the return should look like this:

return [NSArray arrayWithArray:array];


Our clear method should be named more similarly to what we'd expect from other Objective-C collections. In this case, removeAllObjects.

You list several things as throwing exceptions, but I can't actually figure out where any exceptions are thrown.

It looks like eventually anything accessing an index calls back to this method:

-(CKListNode *) listNodeAtIndex:(NSUInteger) index {
    if (index >= [self size] ) {
        return nil;
    }

    // stuff...
}


All you do is return nil;, which doesn't throw an exception. Instead of returning nil, here is one of the few places we actually ought to throw an Objective-C exception:

NSString *reason = [NSString stringWithFormat:@"Index: %i is outside the range (0,%i)", index, (self.size - 1)];

@throw([NSException exceptionWithName:@"NSRangeException"
                               reason:reason
                             userInfo:nil]);


Our CKListNode object shouldn't have a data property or an initWithData: method. Data means an NSData object or subclass. It is a representation of raw data... bytes. This isn't what our nodes carry around. They carry objects. So, we can use Objective-C terminology and initWithObject: as well as naming the property object, or we can use what I've seen in other linked lists and use payload as the property name an initWithPayload: as the method name. I think I prefer object between the two.

Code Snippets

-(BOOL) isEqual:(id)object {
    if (self == object) return true;
    // additional logic to determine if they are the same
}
-(NSString *) description {
    return [[self data] description];
}
- (NSString *)description {
    return [NSString stringWithFormat:@"[<CKListNode:%p>: %@]", self, self.data];
}
return [NSArray arrayWithArray:array];
-(CKListNode *) listNodeAtIndex:(NSUInteger) index {
    if (index >= [self size] ) {
        return nil;
    }

    // stuff...
}

Context

StackExchange Code Review Q#74391, answer score: 6

Revisions (0)

No revisions yet.