Directory listing model

Question

I need a class to read and store directory structure. This is my first step in Objective C so I ask to you to say what you think.

Header file:

#import <Foundation/Foundation.h>

@interface FileStructureRepresentation : NSObject

- (id)initWithPath:(NSString *)path;
-(NSMutableDictionary*) parse;

@end

Code:

#import "FileStructureRepresentation.h"

@implementation FileStructureRepresentation

NSFileManager *fileManager = nil;
NSString *startPath;

- (id)initWithPath:(NSString *)path
{
    self = [super init];
    if (self) {
        fileManager = [NSFileManager defaultManager];
        startPath = path;
    }
    return self;
}

-(NSMutableDictionary *) parse {
    return [self parseDirectory:startPath];
}

-(NSMutableDictionary *) parseDirectory:(NSString *)path {
    NSMutableDictionary *result = [[NSMutableDictionary alloc] init];
    NSMutableArray *collectionOfFilesAndDirectories = [[NSMutableArray alloc] init];
    NSError *error = nil;
    NSArray *listOfFiles = [fileManager contentsOfDirectoryAtPath:path error:&error];

    if(error.code != 0) {
        NSLog(@"Error code:%d, Description:%@", error.code, error.description);
    }

    for(NSMutableString *s in listOfFiles) {
        NSString *temporaryPath = [NSString stringWithFormat:@"%@/%@", path, s];
        NSDictionary *fileAttributes = [fileManager attributesOfItemAtPath:temporaryPath
                                                                     error:&error];
        NSString *fileType = [fileAttributes valueForKey:NSFileType];
        if([fileType isEqualToString:@"NSFileTypeRegular"])
            [collectionOfFilesAndDirectories addObject:s];
        else
            [collectionOfFilesAndDirectories addObject:[self parseDirectory:temporaryPath]];
    }

    [result setObject:collectionOfFilesAndDirectories forKey:path];

    return result;
}

@end

Example:

FileStructureRepresentation *fileStructure = [[FileStructureRepresentation alloc] initWithPath:@"/Users/demas/temporary/tst"];
NSMutableDictionary *structureModel = [fileStructure parse];

Answer 1

1. I usually think that a mutable array or dictionary as a return value is a bad thing, unless there is a good reason. By this I mean the promise of the API, not the actual value returned. I don't see a reason that the value returned must be mutable, so if you're not planing to modify the return value often I'd change the return type of parse to NSDictionary (you can still return the mutable dictionary in its implementation, that's OK; if you'd like to feel save do return [result copy];).

2. Since your class actually only has just one method, and that can be made generic, I'd either turn that method into a class method or turn it into a free function altogether (unless you plan to add more functionality, of course). Like:

   @interface FileStructureRepresentation : NSObject
   + (NSDictionary *)fileStructureRepresentationForPath:(NSString *)path;
   @end
   
   // or even
   
   NSDictionary *FileStructureRepresentationForPath(NSString *path);
   

3. -[NSFileManager contentsOfDirectoryAtPath:error:] returns an array of NSString objects, so it should be for(NSString *s in listOfFiles).

4. When you detect an error (if(error.code != 0)) you should return. I'd return nil. A better way to check for error here would be if(error != nil) since the error code 0 might be a valid value in certain error domains.

5. In your if([fileType isEqualToString:@"NSFileTypeRegular"]) block you are not using braces, even though you do use them for if(error.code != 0) (both have only one line bodies). I'd make sure they're consistent, but especially with respect to goto fail I recommend to always use braces unless you have the if and the body on the same line. It's a matter of taste, though.

6. Instead of [fileAttributes valueForKey:NSFileType] you really want [fileAttributes objectForKey:NSFileType] or even simply fileAttributes[NSFileType]. See Difference between objectForKey and valueForKey?.

7. This is minor: instead of doing [NSString stringWithFormat:@"%@/%@", path, s] it's more explicit and "correct" to do [path stringByAppendingPathComponent:s].

8. You're not evaluating and handling the error from your call to [fileManager attributesOfItemAtPath:temporaryPath error:&error];. If you don't plan to, just pass NULL instead of &error. Otherwise, the question is how to react to the error, that's up to you and your requirements: maybe just skip the entry: if (error != nil) { /* Clear error for next iteration. */ error = nil; continue }

Answer 2

First and foremost, I think the return type really should be an NSDictionary rather than a mutable one... but that aside, there are bigger issues:

-(NSMutableDictionary *) parseDirectory:(NSString *)path {
    // stuff
    NSError *error = nil;
    NSArray *listOfFiles = [fileManager contentsOfDirectoryAtPath:path error:&error];

    if(error.code != 0) {
        NSLog(@"Error code:%d, Description:%@", error.code, error.description);
    }

    // stuff
}

@end

We should handle errors differently and more similarly to how any other Objective-C method would handle errors (not by printing a log statement and continuing normal execution).

For starters, we need to take a pointer to an NSError pointer as an argument:

- (NSMutableDictionary *)directoryTreeAtPath:(NSString *)path error:(NSError **)error;

The reason we're doing this is because we want who ever is actually calling our method to be able to get all the details about the error.

From here, we have two options. If we don't want to hide any implementation details at all, we can pass the error pointer we were passed into listOfFiles. If we do want to hide some implementation details, we can pass our own error object. If we opt for the latter however, it's important that we set up the error object we were passed with useful information.

Either way, after we run the code that could potentially have an error, inside our method, we should simply do this:

if (error) {
    // if error is the variable passed into method, do nothing
    // otherwise, set the variable passed in with your custom error information then just:

    return nil;
}

Once there's an error, we no longer want to continue execution. We certainly don't want don't want to pass back an improperly constructed dictionary.

---

for(NSMutableString *s in listOfFiles)

It's been a while since I messed with contentsOfDirectoryAtPath:error:, but I don't think it provides an array of mutable strings. As such, iterating over its return as if they were mutable strings is a dangerous way of getting a "does not respond to selector" exception.

Moreover, even if they are mutable, if you're not going to use them as mutable strings in the loop, it's far better to treat them as immutable.

Also, we need a better variable name, so I suggest:

for(NSString *currentFile in listOfFiles)