3
\$\begingroup\$

I'm just beginning in Powershell - This will be my first full project, start to finish, and it's quite small. I have already had experience in C#, which helps in this .Net based language.

This program asks the user for the input folder (usually a media device), and colects the data (to sort). It also asks for the destination folder (usually named av), and sorts the pictures into folders named like so:

L:\av\2015\03\Picture01.jpg

I've tested this with 200-300 photos, and it takes around a minute to complete. This seems like too much time to simply copy a few photos, although it is over the network (L is a mapped drive). I tried comparing the same copy using my Powershell script and Windows Explorer, and as a matter of fact, they are almost the same. This leads me to believe that the speed can't really be improved (without changing computer equipment).

$pathToMedia = Read-Host 'Type the full path to your media device'
Write-Host("Great!")
$pathToDest = Read-Host 'Type in the path to the av (destination) folder (without trailing slash (\) )'
Write-Host("Awesome!")
Write-Host

$pictures = Get-ChildItem $pathToMedia | Where-Object {$_.Extension -eq ".jpg" -or $_.Extension -eq ".nef"} #get all the files with those extentions in this directory
$pictureCount = $pictures.Count
$array = new-object ‘object[,]’ $pictureCount,3   #use as follows:  The first column will be an int; id.  The sub-1 will be path, sub-2 will be date-year, and sub-3 will be date-month.
                                                    #e.g: 0,0 = /device/whatever/dc01.jpg
                                                    #0,1 = 2014
                                                    #0,2 = 09

#get
for($i=0; $i -lt $pictureCount; $i++) #note to self: $i++ increases the value after the loop runs
{
    $picture = $pictures[$i]
    $array[$i,0] = $picture.FullName #full file address - use for copying
    $array[$i,1] = $picture.CreationTime.Year.ToString() #creation time of picture - use for sorting
    if($picture.CreationTime.Month -lt 10)
    {
        $array[$i,2] = "0" + $picture.CreationTime.Month.ToString() #creation time of picture - use for sorting
    }
    else
    {
        $array[$i,2] = $picture.CreationTime.Month.ToString() #creation time of picture - use for sorting
    }
}


#copy, sort
Write-Host "Copying files..."
for($i=0; $i -lt $pictureCount; $i++)
{
    $dest = $pathToDest+"\"+$array[$i,1]+"\"+$array[$i,2]+"\"

    if (!(Test-Path -path $dest)) 
    {
        New-Item $dest -Type Directory
        Write-Host "Created directory."
        Write-Host
    }

    Copy-Item $array[$i,0] $dest -ErrorAction Stop
}
Write-Host "Done!"
Write-Host

#delete source (option)
$deleteSource = Read-Host "Delete source files? (t/f)"
if ($deleteSource -eq "t")
{
    Write-Host "Deleting source files."
    Write-Host
}
elseif($deleteSource -eq "f")
{
    Write-Host "Fine then. Not deleting source files."
    Write-Host
}
else
{
    Write-Host "Congrats!  You found the magic chars that automatically delete the hard drive.  System will automattically restart in 5 minutes."
    Write-Host
}


Write-Host "Done!"

exit

Being new to Powershell, I wouldn't know if I'm missing out on something, or if my code does things wrong somehow. That is (now) one of my main objectives here. Thank you guys for helping!



EDIT

Here's my final code. I added many of the changes, and made parameters rather than the read-host stuff.

[CmdletBinding()]
param(
    [Parameter(
        Mandatory=$true,
        HelpMessage='Type the full path to your media device'
    )]
    [ValidateScript({Test-Path $_ -PathType 'Container'})]
    [String]
    $pathToMedia,

    [Parameter(
        Mandatory=$true,
        HelpMessage='Type the full path to the destination for your photos'
    )]
    [ValidateScript({Test-Path $_ -PathType 'Container'})]
    [String]
    $pathToDest ,

    [Parameter(
         Mandatory=$false
    )]
    [switch]
    $deleteSource = $false
)
Write-Host
# -or (throw "'pathToMedia' is not an absolute path, or the path does not exist.") 

$exts = @('.jpg' , '.nef')
$pictures = Get-ChildItem $pathToMedia | Where-Object {$exts -icontains $_.Extension} #get all the files with those extentions in this directory
$pictureCount = $pictures.Count
$array = new-object ‘object[,]’ $pictureCount,3   #use as follows:  The first column will be an int; id.  The sub-1 will be path, sub-2 will be date-year, and sub-3 will be date-month.
                                                    #e.g: 0,0 = /device/whatever/dc01.jpg
                                                    #0,1 = 2014
                                                    #0,2 = 09

#get
for($i=0; $i -lt $pictureCount; $i++) #note to self: $i++ increases the value after the loop runs
{
    $picture = $pictures[$i]
    $array[$i,0] = $picture.FullName #full file address - use for copying
    $array[$i,1] = $picture.CreationTime.Year.ToString() #creation time of picture - use for sorting
    if($picture.CreationTime.Month -lt 10)
    {
        $array[$i,2] = "0" + $picture.CreationTime.Month.ToString() #creation time of picture - use for sorting
    }
    else
    {
        $array[$i,2] = $picture.CreationTime.Month.ToString() #creation time of picture - use for sorting
    }
}


#copy, sort
if($deleteSource)
{
    Write-Host "Moving files..."
    for($i=0; $i -lt $pictureCount; $i++)
    {
        $dest = $pathToDest | Join-Path -ChildPath $array[$i,1] | Join-Path -ChildPath $array[$i,2]    
        if (!(Test-Path -path $dest)) 
        {
            New-Item $dest -Type Directory
            Write-Host "Created directory."
            Write-Host
        }

        Move-Item $array[$i,0] $dest -ErrorAction Stop
        Write-Host -NoNewline "."
    }
}
else
{
    Write-Host "Copying files..."
    for($i=0; $i -lt $pictureCount; $i++)
    {
        $dest = $pathToDest | Join-Path -ChildPath $array[$i,1] | Join-Path -ChildPath $array[$i,2]    
        if (!(Test-Path -path $dest)) 
        {
            New-Item $dest -Type Directory
            Write-Host "Created directory."
            Write-Host
        }

        Copy-Item $array[$i,0] $dest -ErrorAction Stop
    }
}
Write-Host
Write-Host "Done!"
Write-Host

exit
\$\endgroup\$
  • 1
    \$\begingroup\$ It would be worth seeing how long it takes to copy the 300 files in Windows Explorer. That would give you a baseline to compare against. \$\endgroup\$ – Dangph Apr 23 '15 at 0:42
  • \$\begingroup\$ That's true. I just did that, and here are the results: Windows Explorer: 46 Seconds. Powershell Script: 50 Seconds. That's not as bad as I was expecting - makes me feel a lot better! \$\endgroup\$ – Cullub Apr 23 '15 at 1:23
  • \$\begingroup\$ Thanks for coming back and adding the code you ended up writing. \$\endgroup\$ – briantist May 5 '15 at 23:20
3
\$\begingroup\$
Write-Host("Great!")

You should omit the parentheses here and just do:

Write-Host "Great!"

Using parentheses (and subsequent commas) for arguments to cmdlet calls is a very common occurrence for people starting with Powershell. The problem is that is doesn't generally throw an exception because it ends up passing all the values as an array to the first parameter.

Cmdlets should be called with spaces after each argument. Adding to the confusion is that when you call a method on an object, like $a.DoThing(1,2,3) then this is the correct way to call it.

You can help avoid this on cmdlets by using Strict Mode as it will warn you when you make those calls.

I realize that in your case, with a single argument, it probably caused you no issues, but it's a habit that's worth breaking.


Where-Object {$_.Extension -eq ".jpg" -or $_.Extension -eq ".nef"}

There's nothing wrong with this per se but I want to point a few things out.

-eq is usually (maybe always?) case insensitive, but there are explicit case sensitive (-ceq) and case insensitive (-ieq) versions of this and many other operators.

With 2 extensions, using -or is fine, but if the number grows, it can get unwieldy. Consider an array of extensions and the -contains operator:

$exts = @('.jpg' , '.nef')
Where-Object {$exts -contains $_.Extension}

Note that contains can also be explicitly case [in]sensitive (-ccontains or icontains).


$pathToMedia = Read-Host 'Type the full path to your media device'
Write-Host("Great!")

Consider validating these; this is user input after all.

if( ($pathToMedia | Split-Path -IsAbsolute) -and ($pathToMedia | Test-Path) ) {
    Write-Host "Great!"
} else {
    throw "This is not an absolute path or the path does not exist."
}

If you were to create these as parameters to the script instead, you could do this validation as part of the parameter declaration, and by declaring the parameter required, Powershell would prompt the user for you if the parameter is not supplied (this goes at the top of the script):

[CmdletBinding()]
param(
    [Parameter(
        Mandatory=$true,
        HelpMessage='Type the full path to your media device'
    )]
    [ValidateScript( { (($pathToMedia | Split-Path -IsAbsolute) -and ($pathToMedia | Test-Path)) -or (throw "This is not an absolute path or the path does not exist.") } )]
    [String]
    $pathToMedia
)

At this point you can get rid of the Read-Host line for this. You would do something similar for $pathToDest.

However regarding this:

$pathToDest = Read-Host 'Type in the path to the av (destination) folder (without trailing slash (\) )'

Don't rely on the user to avoid the trailing slash. You can remove it yourself with:

$pathToDest = $pathToDest.TrimEnd('\\')

(works even when it's not supplied). But even better than that is to not care about it at all:

$dest = $pathToDest+"\"+$array[$i,1]+"\"+$array[$i,2]+"\"

Instead of this, use the Join-Path cmdlet:

$dest = $pathToDest | Join-Path -ChildPath $array[$i,1] | Join-Path -ChildPath $array[$i,2]

Or with the .Net method you might already be familiar with:

$dest = [System.IO.Path]::Combine($pathToDest, $array[$i,1], $array[$i,2])

Both of those handle trailing slashes for you.


$deleteSource = Read-Host "Delete source files? (t/f)"

Do consider making this a [Switch] parameter (the name probably depends on whether you want to default to deleting or not).

By changing your prompts into parameters, you allow the script to be run automated, or run repeatedly without typing the same information in again.


In response to your comment, to add a second parameter, you add a comma after the first parameter, and then repeat. This all happens inside the param():

[CmdletBinding()]
param(
    [Parameter(
        Mandatory=$true,
        HelpMessage='Type the full path to your media device'
    )]
    [ValidateScript( { (($pathToMedia | Split-Path -IsAbsolute) -and ($pathToMedia | Test-Path)) -or (throw "This is not an absolute path or the path does not exist.") } )]
    [String]
    $pathToMedia ,

    [Parameter(Mandatory=$false)] [ValidateNotNullOrEmpty()] [String] $param2 ,

    [int]
    $param3,

    $param4
)

The attributes in [] and the other pieces of a parameter declaration don't need to be on separate lines like they are in the first parameter, but I prefer them that way for readability. They also aren't all required. As you can see in $param4 you only need a name, not even a data type. And in fact the entire param() block could be on one line as well.

\$\endgroup\$
  • \$\begingroup\$ Thank you! This is valuable for a newbie in powershell (me). 1 question: where would I put the second paremeter, and what would it look like? Simply copying it twice doesn't work... Thanks again! \$\endgroup\$ – Cullub Apr 23 '15 at 22:47
  • \$\begingroup\$ @cullub, I edited the answer to show how to add additional parameters. There aren't a lot of powershell questions here in CR, but I enjoyed this. If you post any new ones shoot me a message and I'd be glad to have a look. \$\endgroup\$ – briantist Apr 23 '15 at 23:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.