PDA

View Full Version : Would anyone mind......




mdeh
Feb 18, 2009, 07:12 AM
Looking at this code and offer some suggestions/criticism.

Note that GC is not yet covered. It's an answer to exercise 16.1 as follows.

Modify the copy program developed in Program 16.6 so that it can accept more than one source file to be copied into a directory, like the standard UNIX cp command. So, the command

$ copy copy1.m file1.m file2.m progs


should copy the three files copy1.m, file1.m, and file2.m into the directory progs. Be sure that when more than one source file is specified, the last argument is, in fact, an existing directory.


Assumes that the files and directory reside in the current working directory.

Thanks in advance.

#import <Foundation/NSObject.h>
#import <Foundation/NSFileManager.h>
#import <Foundation/NSString.h>
#import <Foundation/NSProcessInfo.h>
#import <Foundation/NSEnumerator.h>
#import <Foundation/NSAutoreleasePool.h>

#define ONEARGUMENT 1



int main (int argc, const char * argv[]) {
NSAutoreleasePool * pool = [[NSAutoreleasePool alloc] init];
NSFileManager *fm = [NSFileManager defaultManager];
NSProcessInfo *proc = [NSProcessInfo processInfo];
NSArray *args = [proc arguments];
BOOL isDirectory = NO, exists = NO;
NSString *working_directory, *destDir, *srcfile, *destfile;
NSInteger argCount;

NSLog(@"Current directory: %@", [fm currentDirectoryPath]);

/* check for correct usage */
if ( [[proc arguments] count] == ONEARGUMENT )
{
NSLog(@"Usage: file1, file2..., prog");
return 1;
}

/*check last argument is a directory */
working_directory = [ fm currentDirectoryPath];
destDir = [working_directory stringByAppendingPathComponent: [[proc arguments]lastObject ]];
exists = [ fm fileExistsAtPath: destDir isDirectory: &isDirectory];
if (!exists )
{
NSLog(@"Usage: Directory does not exist");
return 2;
}

if ( !isDirectory )
{
NSLog(@"Usage: Last file must be a Directory");
return 3;
}

/* iterate through[proc arguments] */

argCount = args.count;
int fileCount = 1;


while (--argCount > 1)
{
srcfile = [args objectAtIndex: fileCount++];

/*is source file readable */

if ( [ fm isReadableFileAtPath: srcfile] == NO)
{
NSLog(@"Error: Source file %@ is unreadable", srcfile);
return 4;
}

/*create destination path */

destfile = [ destDir stringByAppendingPathComponent: srcfile];
/* remove file if exists at destination */
[fm removeItemAtPath:destfile error:nil];
if ([fm copyPath:srcfile toPath:destfile handler:nil] == NO)
{
NSLog(@"Error: copy failed" );
return 5;
}

NSLog(@"Copy of %@ to %@ succeeded", srcfile, destfile);
NSLog(@"Contents of source:%@", [NSString stringWithContentsOfFile:srcfile encoding:NSUTF8StringEncoding error:nil]);
NSLog(@"Contents of destination:%@",[NSString stringWithContentsOfFile:destfile encoding:NSUTF8StringEncoding error:nil]);
}


[pool drain];
return 0;
}



HiRez
Feb 19, 2009, 03:06 PM
Don't have time to look it over in detail, but it looks really good initially. Appears you are checking for all the right things, in the right order, providing good feedback, and your code is well formatted and commented. So good job! I assume you've actually run it and it works? Also, maybe you want to prompt to overwrite existing files, or make that an option flag?

lee1210
Feb 19, 2009, 03:18 PM
My least favorite lines are these:

while (--argCount > 1)
{
srcfile = [args objectAtIndex: fileCount++];


It seems like this should work fine, but is a little difficult to follow. I don't know if the book has covered fast enumeration or not yet, but i would modify args by removing the first and last element and either store it back in args or assign it to a new NSArray called fileList or something like that. Then you could just run over fileList using fast enumeration or a simple for loop from 0 to [fileList count]-1, etc. The loop would be cleaner, and there would only be one thing you're "counting", which would be the current position (w/ Fast Enumeration, there'd be nothing to count at all).

The only other thing i am not so sure about is memory management. I don't know if all of the routines used return autoreleased objects. For a program this small it probably isn't a big deal, but i didn't see any retain/release/autorelease messages, so you might just want to be sure you're being tidy. Not for practical reasons for this program, but just to get in the habit.

-Lee

Edit: the fast enumeration documentation is here, if you're interested:
http://developer.apple.com/DOCUMENTATION/Cocoa/Conceptual/ObjectiveC/Articles/chapter_8_section_1.html

To get a new array without a few elements, you could use something like objectsAtIndexes:
http://developer.apple.com/DOCUMENTATION/Cocoa/Reference/Foundation/Classes/NSArray_Class/Reference/Reference.html#//apple_ref/occ/instm/NSArray/objectsAtIndexes:
or subarrayWithRange:
http://developer.apple.com/DOCUMENTATION/Cocoa/Reference/Foundation/Classes/NSArray_Class/Reference/Reference.html#//apple_ref/occ/instm/NSArray/subarrayWithRange:

Edit 2: This post was critical, but the intent was "Everything looks good. The only thing i'd change would be..."

mdeh
Feb 19, 2009, 08:42 PM
Don't have time to look it over in detail, but it looks really good initially. Appears you are checking for all the right things, in the right order, providing good feedback, and your code is well formatted and commented. So good job! I assume you've actually run it and it works? Also, maybe you want to prompt to overwrite existing files, or make that an option flag?

Thank you Hirez...and today I did go outside...and it was wonderful :)

mdeh
Feb 19, 2009, 08:45 PM
My least favorite lines are these:

while (--argCount > 1)
{
srcfile = [args objectAtIndex: fileCount++];


It seems like this should work fine, but is a little difficult to follow. I don't know if the book has covered fast enumeration or not yet, but i would modify args by removing the first and last element and either store it back in args or assign it to a new NSArray

Lee..yes..fast enumeration is covered. I **really** like your idea of a sub-array which is then enumerated. I always had problems making sure the x-- worked with the ptr++ :) ...even though when it does work, you feel like a real programmer!!!
Will implement your changes...very slick!
What criticism?
Thank you as always.

mdeh
Feb 19, 2009, 09:23 PM
My least favorite lines are these:

while (--argCount > 1)
{
srcfile = [args objectAtIndex: fileCount++];




What about this ( much thanks to you)

/* create subarray that will be enumerated */
subArrRnge.location = 1; /* first argument after program name */
subArrRnge.length = args.count - 2; /* 1 for program name and 1 for directory */

NSArray *subArr = [ args subarrayWithRange: subArrRnge];


/* iterate through subArr */

for ( srcfile in subArr)

{ ..................



where args = [proc arguments].

Lee...it is **so** much cleaner...I am beginning to like Obj-C.

thanks so much again.

lee1210
Feb 19, 2009, 09:30 PM
No problem. It is a little longer, but definitely easier read.

This is a *little* more convoluted, and the NSMakeRange could be moved to a new line, but it saves a little space. I don't really love mixing a function call in with a message pass... But anywho, here it is:
NSArray *subArr = [args subarrayWithRange: NSMakeRange(1,args.count - 2)];

-Lee

mdeh
Feb 19, 2009, 10:07 PM
No problem. It is a little longer, but definitely easier read.

This is a *little* more convoluted, and the NSMakeRange could be moved to a new line, but it saves a little space. I don't really love mixing a function call in with a message pass... But anywho, here it is:
NSArray *subArr = [args subarrayWithRange: NSMakeRange(1,args.count - 2)];

-Lee

Very nice...I will incorporate that too...appeals to the C side of me!!! Thank you again.