Script improvement suggestions

This topic contains 0 replies, has 1 voice, and was last updated by  Forums Archives 5 years, 7 months ago.

  • Author
    Posts
  • #5259

    by aviasddsa at 2012-11-04 02:54:26

    Hi , I'm n00b in powershell
    can anyone suggest an improvement on my first script ?
    thx

    < #
    Download sysinternals + nirsoft packages from the web
    #>

    #globals#
    $downloads= @{}
    $target = 'c:\utils'
    $DebugPreference = 2
    $VerbosePreference = 2
    $WarningPreference = 2
    $log = $env:TEMP + '\' + $MyInvocation.MyCommand.name + '.log'

    #function#

    function dl_and_extract($what) {
    $webClient = New-Object System.Net.WebClient
    $downloadFile = $env:TEMP + '\New.zip'

    write-output "downloading $what"
    $webClient.DownloadFile($what ,$downloadFile)

    write-output "Extracting $downloadFile"
    7za e -y $downloadfile -x!Nirlauncher* -x!autorun*

    Remove-Item -Force $downloadFile
    }

    ######################################################################################################

    #main
    write-output "new log started - $(Get-Date -format "yyyy/MM/d hh:mm")" | tee -Append $log

    #nirsoft
    #get the new dl url
    $what = wget -q http://launcher.nirsoft.net/download.html -O - | grep "http.*pack.*.zip" | sed -n "s/>.*//;s/.*http/http/;s/.$//;p;q"
    $downloads['nirsoft'] = $what

    #sysinternals
    $downloads['sysint'] = 'http://download.sysinternals.com/files/SysinternalsSuite.zip'

    #dl and extract
    cd $target
    write-output "Logfile: $log"
    $downloads.keys | %{ dl_and_extract($downloads[$_]) } | tee -Append $log

    write-output "finished running script - $(Get-Date -format "yyyy/MM/d hh:mm")" | tee -Append $log

    by nohandle at 2012-11-04 04:39:24

    Hi,
    just by quick look:
    1) the code gives me errors straight away.
    Bad numeric constant: 7.
    At C:\Users\nohandle\AppData\Local\Temp\d9a71423-a8d9-40b0-b6a6-932536361b7d.ps1:24 char:6
    + 7 < <<< za e -y $downloadfile -x!Nirlauncher* -x!autorun*
    + CategoryInfo : ParserError: (7:String) [], ParseException
    + FullyQualifiedErrorId : BadNumericConstant

    (I dont have 7zip on my station, but this is syntax not a runtime error. What version of powershell do you use?)

    The '< ' operator is reserved for future use.
    At C:\Users\nohandle\AppData\Local\Temp\d9a71423-a8d9-40b0-b6a6-932536361b7d.ps1:37 char:18
    + $what = wget -q < <<<< !-- m -->htt
    p://launcher.nirsoft.net/download.html
    -O - | grep "http.*pack.*.zip" | sed -n "s/>.*//;s/
    .*http/http/;s/.$//;p;q"
    + CategoryInfo : ParserError: (< :OperatorToken) [], ParseException
    + FullyQualifiedErrorId : RedirectionNotSupported

    Tee-Object : A parameter cannot be found that matches parameter name 'Append'.
    At C:\Users\nohandle\AppData\Local\Temp\d9a71423-a8d9-40b0-b6a6-932536361b7d.ps1:48 char:93
    + write-output "finished running script - $(Get-Date -format "yyyy/MM/d hh:mm")" | tee -Append < <<< $log
    + CategoryInfo : InvalidArgument: (:) [Tee-Object], ParameterBindingException
    + FullyQualifiedErrorId : NamedParameterNotFound,Microsoft.PowerShell.Commands.TeeObjectCommand

    2) I would make the download variable an array not a hashtable
    3) using join-path when creating paths less error prone than using + concatenation
    4) add error handling

    Hope you repair it and give it another turn .)

    by aviasddsa at 2012-11-04 06:08:10

    Thanks for the input david,
    I'm using powershell 3.0

    [quote]
    Bad numeric constant: 7.
    At C:\Users\nohandle\AppData\Local\Temp\d9a71423-a8d9-40b0-b6a6-932536361b7d.ps1:24 char:6
    + 7 < <<< za e -y $downloadfile -x!Nirlauncher* -x!autorun*
    + CategoryInfo : ParserError: (7:String) [], ParseException
    + FullyQualifiedErrorId : BadNumericConstant

    [/quote]
    I don't know why that error happens on your station, but i noticed some other parts were not pasted correctly to the post. I'll repaste

    this script requires wget , 7za (7zip archiver ) and sed -so i guess you'll have to comment those lines if you don't have these...
    Btw – any good alternative for sed in ps ?

    Anyway I've reworked the script , added join-path ,removed the download array added some error trapping and tiding. it's working over here so without issues ,give it another go.
    ( hope i did not over complicate – i use it to learn a bit PS ...)

    < #
    Download sysinternals + nirsoft packages from the web
    Requirements (probably will change sometime, so just for reference) :
    * 7za ( \http://downloads.sourceforge.net/sevenzip/7za920.zip )
    * sed ( \http://garr.dl.sourceforge.net/project/gnuwin32/sed/4.2.1/sed-4.2.1-bin.zip )
    * wget ( \http://users.ugent.be/~bpuype/cgi-bin/fetch.pl?dl=wget/wget-1.10.2.exe )

    #>

    #################################################globals############################################

    $my_err = @{
    ERR_DL = '-240'
    ERR_EXT = '-200'
    }

    $packages= @{
    nirsoft = @{
    url = 'http://launcher.nirsoft.net/download.html'
    sed = "/http.*pack.*zip/ {s/.*\(http[^\""]\+\)\"".*/\1/;p;q}"
    }
    sysint = @{
    url = 'http://download.sysinternals.com/files/SysinternalsSuite.zip'
    }
    }

    $targetDir = 'c:\utils'
    $scr = get-item $MyInvocation.MyCommand.Definition
    $log = join-path $env:TEMP -childpath "$($scr.BaseName).log"
    $downloadFile = join-path $env:TEMP -childpath "$($scr.basename).zip"

    #################################################functions############################################

    #init script
    function init_scr() {
    $DebugPreference = 2
    $VerbosePreference = 2
    $WarningPreference = 2

    Write-Verbose "Downloadfile: $downloadFile , log: $log "
    write-verbose "Logfile: $log"
    write-output "new log started - $(Get-Date -format "yyyy/MM/d hh:mm")" | tee -Append $log

    $reqs = "sed.exe","7za.exe","wget.exe"
    $reqs | %{
    if ( -not $(Get-Command($_) -ErrorAction SilentlyContinue) )
    {
    Write-Warning "$_ not found on your system , this script may not run correctly"
    }
    }
    }

    #download package and extract to target dir
    function dl_and_extract {
    param(
    [parameter(Mandatory=$true)]
    [string]
    $url,
    [parameter(Mandatory=$true)]
    [string]
    $downloadFile
    )

    process {
    $webClient = New-Object System.Net.WebClient

    write-output "downloading $url"
    try
    {
    $webClient.DownloadFile($url ,$downloadFile)
    }
    catch [System.Exception]
    {

    #Todo: Check why tee locks the file causing a crash
    #$(
    # write-output "Error downloading $url,cannot continue."
    # Write-output "ERROR: " + $_.Exception.ToString()
    # ) | tee -a $log

    write-output "Error downloading $url,cannot continue."
    Write-output "ERROR: " + $_.Exception.ToString()
    return $my_err['ERR_DL']
    }

    write-output "Extracting $downloadFile to $(get-location)"
    try
    {
    7za e -y $downloadfile #-x!Nirlauncher* -x!autorun*
    }
    catch [System.Exception]
    {
    $(
    write-output "Error extracting $downloadFile"
    Write-output "ERROR: " + $_.Exception.ToString()
    ) | tee -a $log
    return $my_err['ERR_EXT']
    }

    if (Test-Path $downloadFile )
    {
    Remove-Item -Force $downloadFile
    }
    }
    }

    #if retrieving the package requires additional proccessing , do it. add to hash
    function reparse_package_url() {
    $packages.keys | %{
    if ($packages[$_]['sed'])
    {
    $packages[$_]['dl_url'] = wget -q $packages[$_]['url'] -O - | sed -n $packages[$_]['sed']
    }
    else
    {
    $packages[$_]['dl_url'] = $packages[$_]['url']
    }
    }
    }

    ################################################main######################################################

    init_scr

    #detect if sedding is needed
    reparse_package_url

    #dl and extract
    cd $targetDir

    $packages.keys | %{
    dl_and_extract -url $packages[$_]['dl_url'] -downloadFile $downloadFile
    } | tee -Append $log

    write-output "finished running script - $(Get-Date -format "yyyy/MM/d hh:mm")" | tee -Append $log

    by nohandle at 2012-11-05 03:08:40

    To me it seems like you are overcomplicating it a bit. try to break the problem to smaller bits. Imagine how it should work, before you start writing first line of script you should already know how the basic parts of the app are going to be implemented and connected. start from the core functionality. test it properly, do exception handling. and then add the specialities. to be able to achieve the goal you should have at least basic understanding of how the language works so take your time, some powershell book and start with the simple examples and work your way up to the complicated ones.
    But remember the golden rule in programming – if it is too complicated you are probably doing it wrong :).
    And if you can get a copy of Powershell in action second edition book the first part explaining the language is gonna give you tons of useful information. Some general OOP book can also help.

    The "intialize" function (like every function) is setting the variables only in its scope if not explicitly told otherwise. meaning you get the debug visible only in the function not in the whole script.
    Your exception handling doesn't do more than the standard.

    Hope this does not demotivate you. it just takes time and practice and reading 🙂

    by aviasddsa at 2012-11-05 08:15:48

    I'm not demotiviated , Actually i appericate criticism 🙂 Maybe I am overcomplicating the problem (it's a problem with me)

    The core functionality is :

    1. Downloading zipped packages from the web.
    2. After download completed , extract them to target location

    1 special case – some package link are coming from dynamic pages and their links change periodically so to download them, I have to download the dynamic page ,parse the content and locate the package link from the page , only afterwords download the package. (in this script – nirsoft is the example)

    Could you give your take on how would you build this script ? not the whole thing, just an approach/outline ....

    I'll take a look on Powershell in action second edition so thanks for that.

    I believe i have some basic knowledge in oop , are you refering to something you saw in the script ? ( if it's the nested hash – that's the way I do small things in javascript and php though it could probably be an object too)

    I missed the $DebugPreference in function thing ( thought those were globals ). the exception handling is lacking i know , just wanted to test it out..

    BTW – I could make the whole thing an object with some methods to download but wouldn't that be an overcomplication for this task ?
    Thanks for the reply. critic helps me learn.

    by nohandle at 2012-11-06 00:43:13

    Ok. I am constructing a script that is gonna be used by me or my tech colleagues – so the exceptions can be "raw".
    I want the script to help us download the packages because we do it on regular basis. No production system relies on it, so the worst thing that can happen if the script fails is that the have to be downloaded manually.
    I think my colleagues are going to like it, so I anticipate there are going to be few more packages added to the job.I t should be quick to write because it saves us only about 20 minutes of work a week. So I should not waste a whole manday on it. Also the files downloaded are small and my network is fast so i dont worry too much about download the files one after another etc.

    OK,lets break it down to smaller pieces:
    As you said the core functionality is to download and to extract the file. two different problems so I am going to address them in (at least two) different functions.
    The basic parameters that a donwload function should accept is probably a link to the file and the destination path (defaulting to users download folder). If the operation is succesfull, file object is returned, if it fails nothing is returned in the output (mimicking behaviour of the standard cmdlets like get-item), errors are output to the error output. I am not familiar with the exact behavior of the web client so teke it with grain of salt .)

    I refer here for the info on exceptions http://msdn.microsoft.com/cs-cz/library/ez801hhe.aspx . Seems like there is not many exceptions we can do something about – If the address provided is wrong there is no way for this function to fix it.

    define script parameters [string[]]$url, destination, force

    validate everything you can (external commands etc) thaht would prevent the
    script from running. if something is wrong and you cannot fix it (or it would take
    too much effort) throw terminating error

    if (-not (7zip is available)) {"no 7zip on this station cannot continue"}
    etc.

    function downloadFile ([string[]]$url, [string]$Destination = \downloads)
    {
    check if the destination is existing folder,
    Foreach item in $url
    {
    download file
    void all unnecessary output
    if the file object is not returned by the web
    client function by default, get-item the resulting path
    to return the file object
    if the download ar saving the file
    fails return nothing to the standard, non terminating
    errors are output to error output
    }
    }

    function extractFile ([string]$Path, [string]$Destination=\downloads, [switch]$Force)
    {
    check if the destination path exists and if it is a container (folder)
    if force is specified and the path does not exist create
    the directory
    foreach item in path
    {
    check if the file exists. check if it has the right extension
    if force is specified rewrite the files in the destination directory

    function body
    return appropriate objects
    }

    $destination

    #this is not ideal approach, it makes the download run first and after
    #all files are downloaded the extarct is run
    #using jobs is better option especially for big files

    $downloadedFiles = DownloadFile @psBoundParameters
    extractFiles @psBoundParameters -path $downloadedFiles
    }

    hope this approach works 🙂

You must be logged in to reply to this topic.