Re: Does this caution need fixed? (newb)

FROM : Brian Stern
DATE : Wed Jul 09 21:39:21 2008

On Jul 9, 2008, at 10:44 AM, Chris Paveglio wrote:

> I'm trying to make sure that this part of my code is good and clean 
> and not leaking. I believe that I have it right (but still think I 
> have something wrong).
> This code makes a list of file paths, and then copies the files or 
> folders from their location to the destination the user selected. If 
> the item already exists in the destination, I delete it first (to 
> overwrite with new data, yes this is what I intend and the user is 
> aware of how the app works).
> I've read all your previous responses (Thanks) and it seems like 
> since I am just using "normal" strings that they will be 
> autoreleased on their own. There are now no compiler cautions and 
> the app seems to work OK.
> A side issue is that I want to add a 2 second delay before closing 
> the sheet that is displayed, is NSTimer the correct item to use?
> Chris


I have some stylistic comments on this code.

Your method is too large.  To make it smaller you should break it up 
into a number of smaller tasks.
As Jens mentioned a string like this

NSString *string1  = @"Library/Preferences/Adobe Photoshop CS3 
Settings";

is pretty much the same as a string like this: @"Library/Preferences/
Adobe Photoshop CS3 Settings"

I would build a method that does nothing but return the array or 
arrays that contain your manifest of files to save

-(NSArray)myPrefsArray
{
   // build array here
   return myPrefs;
}

I notice that your  myPrefsSaved array is derived from the myPrefs 
array but it just has the file name not the full path.  I'd use 
[astring lastPathComponent] to either build the second array or just 
call this method inside your loop.

I would break out the entire copy files loop into its own method.

Since you loop over the contents of the myPrefs array I wouldn't hard 
code the loop index to 8 or 9 I'd depend on the [myrefsArray length] 
value to index the loop.  You should also look at NSEnumerator and 
also fast enumeration as ways to drive this loop in a better way.

I notice that you call [myPrefs objectAtIndex:i] several times in your 
loop.  If you use NSEnumerator or fast enumeration you can avoid this 
otherwise just call it once at the top of the loop and save the value 
in a temp string variable.

Also, declaring the loop counter outside the loop is now old 
fashioned.  You can try this:

for (int i = 0; i < limit; i++)

You might need to set your C dialect setting to C99 or gnu99 if you're 
using an old project. I recommend it.

You don't check any errors from the file manager operations. What if 
the user chooses a folder on a locked volume or the disk gets full or 
the user chooses a network volume and the network drops out?  Your 
users will hate you if these things happen but your app fails 
silently.  You should both check for errors and report them to the user.

I don't see any problems in this code with leaking of objects.

I don't like the way that all the steps of this process are done one 
after another in a sequential fashion.  I would probably set up the 
tasks to be done in a separate thread or have each file copy be done 
inside a timer callback until all are done and show a progress window 
probably in place of a progress sheet (although it might work ok 
either way).

Regarding your 2 second delay you can use a timer or 
performSelector:withObject:afterDelay but both will require you to 
provide a callback method that will dismiss your sheet.

Good luck,

Brian