Over zealous NOOB seeks code review for first script!

Okay, I understand that this is NOT a complicated script, but I am still very excited about it. I know that there are many ways to skin a cat, this cat being an AppleScript, but I want to know if there are any coding standards or best practices that I could implement to improve my scripting as I learn. Does this script look okay, or am I making some basic NOOB mistakes? Is there a sharper knife out there to skin THIS cat?

I know you don’t need an explanation but the code works as follows:

  1. Copies all of the files and folders from a specified drive (external keydrive, flash cards etc the focus) to a desktop folder
  2. asks if you want to delete the files and eject disk

All blunt criticism welcome!

set path1 to ("SCANDISK")
set path2 to (path to desktop folder as text) & "transfer from external"
tell application "Finder"
	if folder path2 exists then
		--well, do nothing. Wish there were a "if folder path2 does not exist then" command.
	else
		tell application "Finder" to make new folder at desktop with properties {name:"transfer from external", comment:"files transfered from an external drive or card"}
		--say "new folder on desktop created" using "Bells" --man, this takes too long. Have to wait for it to end?
	end if
	activate
	duplicate every file of disk path1 to folder path2 with replacing
	duplicate every folder of disk path1 to folder path2 with replacing
end tell
--say "transfer complete"
tell application "SystemUIServer"
	activate
	set button_pressed to button returned of (display dialog "Transfer Complete. 
Files in folder \"transfer from external\" on you desktop" buttons {"Close", "Eject Drive", "Delete Files & Eject Drive"} default button 3 with icon note)
end tell
if the button_pressed is "Eject Drive" then
	tell application "Finder" to eject path1
end if
if the button_pressed is "Delete Files & Eject Drive" then
	tell application "Finder"
		delete every file of disk path1
		delete every folder of disk path1
		delay 0.4
		empty
		delay 2.0 -- I don't trust this. If trash is full it may take more than 2 secs to empty, disk wont eject. Solution?
		eject path1
	end tell
end if

big thanks to Adam Bell, Kel,

iPhotoStuff:

  1. there is a way to check for non-existence >> if not exists (folder path2)…

  2. You can use items instead of files/folders. >> duplicate every item of disk path1 to folder path2 with replacing

  3. You can also nest your If/Thens…

if the button_pressed is “Eject Drive” then
tell application “Finder” to eject path1
else if the button_pressed is “Delete Files & Eject Drive” then
tell application “Finder”

It won’t speed things up but if you’ve already got your response to “Eject Drive” being true, there’s no sense in checking if button_pressed is anything else.

Just some thoughts.

Have fun,
Jim Neumann
BLUEFROG

Bluefrog,

I like those suggestions. The not(exists) and “item” are a huge help. The compiler automatically added parenths. Man, I wish I could just use regualr notation like ==, != etc.

The if else statement is returning an error for me:

if the button_pressed is "Eject Drive" then
	tell application "Finder" to eject path1
else if
	the button_pressed is "Delete Files & Eject Drive" then
	tell application "Finder"
		delete every item of disk path1
		delay 0.4
		empty
		delay 2.0 -- I don't trust this. If trash is full it may take more than 2 secs to empty, disk wont eject. Solution?
		eject path1
	end tell
end if

returns this error: Expected expression but found end of line.
am I missing something? I have an “end if”

iPhotoStuff,

The if goes on the next line with the statement. There’s also 2 ifs so another end if goes at the end.

About the deleting files, you might try:


if the button_pressed is "Eject Drive" then
	tell application "Finder" to eject path1
else
	if the button_pressed is "Delete Files & Eject Drive" then
		do shell script "/bin/rm -rf /Volumes/SCANDISK/*"
		tell application "Finder"
			eject path1
		end tell
	end if
end if

I’m pretty sure the next step won’t happen until that finishes.

iPhotoStuff:

the else…if should be one line.
else if the button_pressed is “Delete Files & Eject Drive” then…

gyuen’s suggestion with else on one line will work also but typically here’s how to think about this.

If/Else If…
check condition 1 of a requested operation >> validates false
else check condition 2 the the same requested operation >> validates false
else check condition 3, etc., etc.

if the ball is red then
throw the ball
else if the ball is blue then
kick the ball
else if the ball is green
keep the ball and run away.
end if

All these operations are talking only about the ball and what you want to do with it.

Where you would (typically) use gyuen’s way is if you are checking the conditions of more than one thing.

if the ball is red then
throw the ball
else
if the toy is a truck then
put the toy in your pocket
end if

Here you want to either take the toy or throw the ball, one or the other, but the choice is dependent on what you’ve got. (I have a green ball, so I’m not throwing it, but I do have a toy truck I’ll put in my pocket. In no case can I throw the ball AND keep the truck. Or I might not be able to do either thing - no red ball, no toy truck. Note: If I did want to do both operations, then I would separate them into individual if/then blocks.)

Hope this isn’t too confusing. Just my (maybe peculiar) way of looking at it.

Jim Neumann
BLUEFROG

gyuen,

love the shell script addition. It does EXACTLY what I want with just a single line of code. Beautiful. Looks like it deletes ONLY the items on the disk, somehow extraordinarily quickly at that. Something that Adam pointed out was a way to dump the entire drive but keep any item that had extension .app there. Is there any way to make exception using the same shell script? Combining shell scripts with AppleScript – now I’m getting nervous.

Jim,

I get the else if on one line now. Thanks.

updated code:

set path1 to ("SCANDISK")
set path2 to (path to desktop folder as text) & "transfer from external"
tell application "Finder"
	if not (exists (folder path2)) then
		tell application "Finder" to make new folder at desktop with properties {name:"transfer from external", comment:"files transfered from an external drive or card"}
		--say "new folder on desktop created" using "Bells" --man, this takes too long. Have to wait for it to end?
	end if
	activate
	duplicate every item of disk path1 to folder path2 with replacing
end tell
--say "transfer complete"
tell application "SystemUIServer"
	activate
	set button_pressed to button returned of (display dialog "Transfer Complete. 
Files in folder \"transfer from external\" on you desktop" buttons {"Close", "Eject Drive", "Delete Files & Eject Drive"} default button 3 with icon note)
end tell
if the button_pressed is "Eject Drive" then
	tell application "Finder" to eject path1
else if the button_pressed is "Delete Files & Eject Drive" then
	do shell script "/bin/rm -rf /Volumes/" & path1 & "/*"
	tell application "Finder"
		eject path1
	end tell
end if

Hello again,

I am having a bit of problem modifying the script below. The key is, once all of the files are transfered I I need to delete all of the files from a specific folder on my flash card while keeping the file structure intact. I have tried to use gyuen’s shell script below to accomplish this, but frankly, I have no idea what the shell script is doing, and now since I am pointing it to a specific folder it doesn’t seem to delete, only eject the disk.

Can anyone help me figure out nicely how to delete the files from the folder (erase them from the disk, not just put them in the trash) and then eject the disk? What am I doing wrong near the very bottom with that shell script? Thanks!

set trashTemp to (path to desktop as text) & "trashTemp:"
set diskName to ("LEXAR MEDIA")
set path1 to (diskName & ":DCIM:119NCD1X")
set path2 to "NEF_HD:NEF temp:"

tell application "SystemUIServer"
	activate
	display dialog "Start transfer process?" buttons {"Cancel", "Start Transfer"} default button 2 with icon note
end tell

tell application "Bridge" to activate
tell application "System Events" to keystroke "h" using {command down, option down}
tell application "Finder"
	activate
	with timeout of 3000 seconds
		duplicate every item of folder path1 to folder path2 with replacing
	end timeout
end tell
--say "transfer complete"
tell application "SystemUIServer"
	activate
	set button_pressed to button returned of (display dialog "Transfer Complete." & return & "Files in folder \"" & path2 & "\"" buttons {"Close", "Eject Drive", "Delete Files & Eject Drive"} default button 3 with icon note)
end tell
if the button_pressed is "Eject Drive" then
	tell application "Finder" to eject diskName
	tell application "Bridge" to activate
	tell application "SystemUIServer"
		activate
		display dialog "You may now remove your card" buttons {"ok"} default button 1 giving up after 3
	end tell
else if the button_pressed is "Delete Files & Eject Drive" then
	do shell script "/bin/rm -rf /Volumes/" & path1 & "/*"
	tell application "Finder"
		eject diskName
	end tell
	beep
	tell application "SystemUIServer"
		activate
		display dialog "You may now remove your card" buttons {"ok"} default button 1 giving up after 3
	end tell
	tell application "Bridge" to activate
end if

It looks like the problem is your trying to pass an AppleScript path to a shell script which expecting a POSIX path.

I would test this myself for you, but don’t have the time so try modifying your remove to something like this…


else if the button_pressed is "Delete Files & Eject Drive" then
   set path3 to posix path of path1
   do shell script "/bin/rm -rf /Volumes/" & path3 & "/*"

edit the other thing to consider is if you have spaces in the path name you may need to do it like this


else if the button_pressed is "Delete Files & Eject Drive" then
   set path3 to POSIX path of path1
   do shell script "/bin/rm -rf /Volumes/" & (quoted form of path3) & "/*"

Or you can escape the spaces, but that one I’ll leave to ya :wink:

Thanks for the quick response myndcraft, but neither of those changes work. I am not sure if it is a posix problem considering that previously I set the variable the exact same way for the disk name and used to erase and eject the entire name. So the variable type seemed to work before, but that is just a noobs thought process.

-i

Well I’m stuck at work at the moment, but will try to look at it a little later. So unless someone gets back sooner look for a response tonight/tomorrow morning.

For reference though… after the script runs what is the value of path3 set to?

Edit

Actually it looks like the posix path is the issue at least in the quick tests I did… the problem though is the POSIX path implies the “/Volumes” so your script parmater is becoming “/Volumes/Volumes/blah blah”

Try this change

else if the button_pressed is "Delete Files & Eject Drive" then
set path3 to POSIX path of path1
do shell script "/bin/rm -rf " & (quoted form of path3) & "/*"

STUCK AT WORK?! Hey, get out of there man. Run for the hills, the ocean, anything that’s close by!

Hey, that works! Thank you so much. You are the best! I really appreciate the help. I would have been stuck at my computer all day without the help. Thanks!

-i

Glad that worked for you! One of my servers blew up yesterday so I was there all night fighting the fires, headed back in there now as well.

Anyway the reason why it had stopped working was you introduced two things into your equation that was different than when you had it working. First is the introduction of spaces in the volume name and second was subfolders withhin your volume.

The addition of spaces is the reason we have to do quoted form of the path. Without doing that when the rm command encounters the space in the name it thinks its now looking at a second paramater and it breaks your path.

The introduction of subfolder within the volume caused you to end up with a half POSIX, half AS path "/Volumes/LEXAR MEDIA:DCIM:119NCD1X which also breaks the rm command and thus our need to convert the path to a complete POSIX path.

Anyway I’m rambling so Im going to go grab coffee and head into work.