# Tag Info

65

I assume you're just trying to avoid the error message in case it doesn't exist. What if you just ignore it: Remove-Item $folder -Recurse -ErrorAction Ignore If that's not what you want, I would recommend writing your own function: function Remove-ItemSafely { [CmdletBinding(SupportsShouldProcess=$true)] param( [Parameter( Mandatory=$true, ... 13 First, you don't need to call Get-Date for every file. Just call it once at the beginning:$t = (Get-Date).AddMinutes(-15) Get-ChildItem -Path $path -Recurse | Select Name, PSIsContainer, Directory, LastWriteTime, Length | where {($_.LastWriteTime -gt $t)} That's saves about 10% (as measured by Measure-Command). Secondly, you don't need to call ... 9 Your code and overall logic look good. In general the areas of improvement I see are There are some things you are inconsistent about, e.g., function parameters You are sending notification information down the pipeline. Some of the parameter declaration could be improved. I see that your are trying to make the code fit within a certain amount of ... 8 I know this question is a bit old, but I ran into it while looking for other Excel/PowerShell help and had recently found one potential fix. A two line change may actually speed up the entire process significantly. Change from: #loop through the worksheets in the current workbook for ($i = 1; $i -le$wb.Sheets.count; $i++) To: #loop through the ... 8 I have three points to make: GetNumber: The naming indicates it may be for integer conversion. While the truth is far from that, renaming might be a good idea. Instead of having the function outside the main loop, have the max passed into PlayGame and simply do the operation in the game loop: Function PlayGame([int]$max) { #... $secretNum = Get-... 8 Needs more variables I see plenty of integer constants. They define the upper and lower bounds of the game. Let turn those into variables and define them. Makes the game easier to make changes. Function PlayGame([int]$min, [int]$max, [int]$NumberOfGuesses) { # Hilarity ensues } Since these values get passed to the other function and serve the same ...

8

Once you've found the right password and moved the file, then there is no need to try the rest of the passwords on it. You can break out of the loop with a break command. (You have to change the pipeline % into a foreach ( in ) because otherwise the break would break out of the outer pipeline too. Thanks to Lieven in the comments for that.) foreach ($... 7 Reusable Commands One of the most important ideas in PowerShell is the Get/Update/Set pattern. A clear delineation between resources (nouns) and commands (verbs) helps to write code that is useful not only for scripting, but also in the interactive shell. We only need a couple of new commands. First we need to Find the working copies under some directory, ... 7 Better is subjective but I will try and improve the code your have here to make it more robust and scalable. I will try to cover areas of improvement or focus and bring it together in the end. A lot of this is error hardening and trying to help remove potentials flaws in the assumptions the code was making. Pitfalls with non validated paths passed ... 7 Want to start of with I am new to workflows as well. For the most part your code is solid. You should see a couple of suggestions that would actually improve performance especially if you are running this against 100's of computers. Grouping logic This is more of a compliment. I thought, from what I read, that -ThrottleLimit would be a simple answer to ... 6 To reduce the amount of log files I would keep things in memory:$Log = "C:\scripts\list.txt" $purged = "C:\scripts\purged.txt"$now = Get-Date $days = "7"$Lastwrite = $now.AddDays(-$days) $datetime = get-date -f MM-dd-yy-hhmmtt #Robocopy will automatically overwrite the log file unless you specifiy /log+: robocopy "c:\downloads" "c:\downloads1" /e /Purge ... 6 One thing that I notice, you can change this repetitive fragment if($($domain.DistinguishedName) -like "*DomainA*"){ Write-Output ((Get-Date).ToString('MM-dd-yyyy:hh:mm:ss') + ": Moved$($comp.DistinguishedName) to$DomainAOUDump") | Out-File $log -Append Move-ADObject -Identity$comp.DistinguishedName -Server $($Domain.ReplicaDirectoryServers[0]) -...

6

Parsing HTML I won't lay into you too badly about using RegEx on HTML since you clearly already know it's a bad idea and are trying to parse it as XML instead. Not a bad idea. I have to recommend HTML Agility Pack though. It is designed for HTML and it works well with imperfect HTML (unlike the XML parser which is very strict). It seems to be designed ...

6

Purposeful PowerShell Help I see that you have some comments in your script. Those for the most part would be hidden inside the script and the only way someone would see them is to open it. This may or not be what you intended but PowerShell has its own help based comment system that can contain this information as well as make it easy for running users to ...

6

To streamline the permission handling you may want to wrap ACE creation in a function and set the ACL just once at after all ACEs were added: function New-Ace { [CmdletBinding()] Param( [Parameter(Mandatory=$true, Position=0)] [Security.Principal.NTAccount]$Account, [Parameter(Mandatory=$false, Position=1)] [Security.AccessControl.... 6 Disclaimer: I never wrote code like that, I do not see a reason to do it and I discourage everyone to do it. This however may be seen as nothing else aside of a preference. But what are you trying to achieve? Prefectionism? I do not associate myself with perfectionism, and even if I did as I stated, I doubt I would include this on the things I must do to ... 6 Your code looks fine. Just some notes: Excel The main weakness of your design as I see it is its reliance on the Excel application. Since you say robustness is important, Excel would be the main weak point. Microsoft says don't use Office as a server. From my own bitter experience, I think they are right to say that. It's not so bad in your case because ... 5 Your code looks pretty good to me. You can replace this: [string]::Format('{0} = 0x{0:X8}',$val) with this: '{0} = 0x{0:X8}' -f $val Empty "catch all" catch blocks are not a good idea: try { # ... } catch { } They will hide all exception types indiscriminately, including ones that you might want to know about, which is not a good thing.... 5 Okay, I'll start by trying to answer your questions first. If there is a lot of files and you should look into using runspaces to run several copy-jobs in separate threads. Just be careful, you are still limited by the bandwidth of the network. We need to see some kind of error to be certain, but I would start by using PowerShell commands instead of old dos ... 5 I find it easier to use the Interop vs. COM approach to Office automation. If nothing else it makes finding values to the enumerated types easier, everything else is basically the same. Add-Type -ASSEMBLY "Microsoft.Office.Interop.Excel" You should also look at$WorkSheet.UsedRange This will give you a range (Row and Columns) that excel identifies as ...

5

It is better to use pipeline syntax. If there are no files, then nothing will happen: Get-ChildItem $folder -Recurse | Remove-Item 5 Your code looks good to me. I like how it takes pipelined input. That is pretty elegant. You don't need the Position=0 here: [Parameter(Position=0,ValueFromPipeline=$true)] This is what the help says about that: By default, all function parameters are positional. Windows PowerShell assigns position numbers to parameters in the order in which the ...

5

I don't know enough about handling credentials to know whether the overall concept is sound, so I will ignore that aspect. I do have the following suggestions regarding the details of your code. Use Splats Instead of Backticks $splat = @{ TextExtension = @("2.5.29.37={text}1.3.6.1.4.1.311.80.1") Provider = "Microsoft Enhanced RSA and AES ... 5 I'm no powershell expert but if spaces sprinkled around here and there would make the code more readable. So instead of this: for($inputIndex=1;$inputIndex -le$inputNumbers[0];$inputIndex++){ it would read like this: for (inputIndex = 1;$inputIndex -le $inputNumbers[0];$inputIndex++) { Less crammed and easier to read I'd say. Your outer for loop could ...

5

Aliases You're used to my reviews by now so you're probably tired of me saying this, but avoid aliases and abbreviations (% vs ForEach-Object, Select vs Select-Object, -expand vs -ExpandProperty, sort vs Sort-Object, etc.). Backticks ` as line continuation Try to avoid shortening lines with backtick where possible. The backticks are hard to see / easy ...

5

I am glad you asked your question here. I'm hoping to explain why what you are trying to do is a bad idea. I cannot speak much for improving your batch as that is not my strong point but that should not matter as the focus of the review is .... Security Through Obscurity If you are unfamiliar with that phrase as a concept Wikipedia has a good primer on it. ...

5

The first thing to observe is that the five pieces of code are identical except for two parameters that are varying. We can capture the common the code in a function (let's call it populateComboBox), and call that five times: populateComboBox $UI.Combobox1$ColumnsHashTable.Column1 populateComboBox $UI.Combobox2$ColumnsHashTable.Column2 populateComboBox $... 5 For a beginner script there are some There might be more to cover but there are a few things here that could improve the script. Variable and function naming In general functions and cmdlets should follow the Verb-Noun naming convention. You have the verb and noun part down but the hyphens are not there. Not a must but I would expect you to see those on ... 5 Get-ChildItem -include *.ext1, *.ext2 instead of Where Use pipelining via | (in direct fashion, that is without storing the results in intermediate variables) instead of foreach statement so that the processing starts immediately without waiting for the entire directory to be enumerated first (or use the advanced method of non-blocking IO.DirectoryInfo ... 5 split and spread to variables:$IP, $SubnetBits =$Subnet -split '/' your user-function invocation with parentheses is incorrect and works only for single parameter (basically it's interpreted as user-function (param1), the correct syntax is user-function param1 param2 param3 e.g. IPv4toBin $IP to get a power of 2 use a bitwise left shift:$imax = (1 -shl ...

Only top voted, non community-wiki answers of a minimum length are eligible