patternMinor
LinkedList (doubly) implementation
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
And more importantly, rather than throwing an exception, our method should just return
Your
What this does is starts with a check to see if
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.
Our
But the return should look like this:
Our
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:
All you do is
Our
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.