Suggestions for improvements/discussions

[lipkau]

trafficstars

Hi.

Note: I really looked into the exisiting tickets before posting, but I might have overlooked something and included something you already discussed

I am really exited about this repo and already decided to change some stuff I do when coding. While thinking about how I code stuff I thought of these situations and am looking forward to your opinion on them:

• 1 line code blocks
  Altought an article arlready suggests having { in a dedicated line, I do get annoyed with block like:

if ($foo -match "^\d+$")
{
    $bar = $false
}

I write it like:

if ($foo -match "^\d+$")
    { $bar = $false }

• Documentation Comments relative to function
  The section DOC-01 Write comment-based help already ilustrates this, but I think a section to "where to put the comment" would be nice.

<#
    .DESCRIPTION
        Lorem
#>
function do-stuff
{
    param()
    process
    {
        "bar"
    }
}

vs

function do-stuff
{
    <#
        .DESCRIPTION
            Lorem
    #>
    param()
    process
    {
        "bar"
    }
}

vs

function do-stuff
{
    param()
    process
    {
        "bar"
    }
    <#
        .DESCRIPTION
            Lorem
    #>
}

• How to make param mandatory
  I don't like it, that Get-Help throw returns

...
USING THROW TO CREATE A MANDATORY PARAMETER

You can use the Throw keyword to make a function parameter mandatory.

This is an alternative to using the Mandatory parameter of the Parameter
keyword. When you use the Mandatory parameter, the system prompts the user
for the required parameter value. When you use the Throw keyword, the
command stops and displays the error record.

For example, the Throw keyword in the parameter subexpression makes the
Path parameter a required parameter in the function.

In this case, the Throw keyword throws a message string, but it is the
presence of the Throw keyword that generates the terminating error if
the Path parameter is not specified. The expression that follows Throw
is optional.

 function Get-XMLFiles  
 {  
     param ($path = $(throw "The Path parameter is required."))  
     dir -path $path\*.xml -recurse | sort lastwritetime | ft lastwritetime, attributes, name  -auto  
 }  

Maybe spend a # BAD example on this in When using advanced functions or scripts with CmdletBinding attribute avoid validating parameters in the body of the script when possible and use parameter validation attributes instead.?

• -verbose / -debug
  Best Practice on what kind of information should be return in Write-Verbose and Write-Debug ?
• [Alias()]
  Any best practices for [Alias()]? I don't like code where an alias is defined just to shorten a parameter name

# GOOD
param(
    [Alias("cn")]
    $computerName
)

# BAD
param (
    [Alias("comp")]   # as this would be usable without this line anyway
    $computerName
)

• Unit Tests
  Any recommendations on this? Shouldn't a .\Tests\ folder make sense in a Module?
• Module Folder Structure
  I see no use of making a single of .ps1 file for each function in a module with 2 - 3 functions. But where is the limit? Any recommendations in the structure?

   .\
      ModuleName.psd1
      ModuleName.psm1
      Functions\
                       Foo-Bar.ps1
                       Bar-Foo.ps1
      Dlls\
      [...]

• How to build a proper .psm1 file
  Any best practice on how to load .ps1 files in a module?

I currently use

Get-ChildItem -Path $PSScriptRoot -recurse | Unblock-File
Get-ChildItem -Path $PSScriptRoot\*.ps1 -recurse | Foreach-Object{ . $_.FullName }

[darkoperator]

"1 line code blocks” agree there are exceptions where it just does not make sense. "Documentation Comments relative to function” Not touching this one, everyone has their opinion, I would say all are valid and just leave it at that. "How to make param mandatory” Kind of agree on this one but feel if you need mandatory params just make it and advanced function and use the parameter properties to make it mandatory. "-verbose / -debug” agree and would also add -help to the list, see the -help a lot. “[Alias()]” Agree on this one, it shows lack of knowledge on how PS works with parameters when I see it "Unit Tests” No clue on this one, still learning about it. "Module Folder Structure” hard one outside of pester tests. I use Assemblies for DLLs I import on mine. the rest just put in folder to organize per general function. "How to build a proper .psm1 file” I would prefer to specify each one rather that listing and adding them dynamically. Starting to see backdoor profile files in capture the flag events and used by pentesters, this just adds an easier way for code to be added by accident.

[dlwyatt]

Personally, I don't think brace placement or comment-based help block placement matters, so long as it's consistent across the codebase. When you contribute to a project, if they've specified a style, then adhere to that style. (If it's already an inconsistent mess, then just use your best judgement, I suppose.)

My own personal preference is to put short conditionals all on one line. You'll often see this sort of thing in a finally block for cleanup, in my code:

finally
{
    if ($session) { Remove-CimSession $session }
}

[rkeithhill]

@dlwyatt +1. I even do the same in C#

if (arg1 == null) throw new ArgumentNullException(nameof(arg1));

[Jaykul]

1-line code blocks

I do this sometimes... particularly when pasting code into a script from a command-line history.

In C# I insist that if you want to leave braces off, it must go on one line, like what @rkeithhill wrote.

Despite that -- it's absolutely not a best practice. That is, I would be mildly opposed to having an automated check that stopped people from doing it occasionally, but I would also be opposed to even mentioning that format in a style guide, and I'd never intentionally put code like that in a lesson plan or presentation.

Long experience has shown that the way that leads to the least problems in the future (in every language) is to always put the closing brace on it's own line, and never put code inside the brace on the same line as the opening brace. It's just a lot harder to screw up when you come back to add something later.

Documentation Comments

I have tried over the years every place where PowerShell allows the doc comments: above (like I was used to in C#) or at the foot (just to get it out of the way), and inside the top ... my recommendation is what is in the guide (I think I wrote this paragraph, and I just added a headline to it so you can't miss it). Basically:

• In order to ensure that the documentation stays with the function, documentation comments should be placed INSIDE the function, rather than above.
• To make it harder to forget to update them when changing a function, you should keep them at the top of the function, rather than at the bottom.
• To make it harder to forget to keep the comments synchronized with changes to the parameters, the parameter documentation comments may be within the param block, directly above each parameter.

This is purely about the "pit of success" and is in no way meant to imply that putting them elsewhere is wrong -- I just believe that you'll make less mistakes in maintenance this way.

Mandatory parameters.

Your comment is dead on. Code that assigns a default value which throws is legacy code from before there was a [Parameter()] attribute in PowerShell. Feel free to provide that as a counter example (and mention that people may see it a lot, but only because there's a lot of code around the internet from PowerShell 1.0 still).

Misc.

Unless you want to have a go at writing those sections, I think it would be worth filing separate "issues" for each of these so that people can pick them up and write them. Your comments are dead on.

• What should go in Verbose output
• Is there a way to make Write-Debug useful? (yes, but not with the -Debug switch)
• Aliases (some discussion of this belongs in naming conventions, which is currently blank)
• Module Structure
  • Have a \Tests Folder!
  • Have a \lib or \assemblies (or \Dlls) folder if you need them
  • Don't check compiled assemblies into source control -- use packages.json and nuget for binaries, or git submodule for source code, and provide a setup and/or build script for people to generate packages.

We've been playing with some models for folder structure and build/test/publish scripts in our experimental development processes project (PoshCode\PSGit) -- I don't think we're done playing, but it has both binary and source dependencies, and an automated build with 100% code coverage, so it's worth looking at as a possible example of how to structure things ;-)

Organization

Personally ....

I am mildly opposed to having many script files for modules.

I strongly recommend you avoid using gci to import all the scripts in a subfolder ever. You're creating a code-injection vulnerability, and in this case, also breaking PowerShell's ability to guess what commands are in your module if you used ExportedFunctions = "*" (which you ought to avoid, but nevermind that).

If you must have nested files, make them psm1 files, and always list each of them explicitly. This avoids merging the variable scopes of all those scripts together, and protects you from silly bugs where someone editing one file is unaware of variables/functions in another file.

Keep functions which depend on each other in the same module, and only break out modules of functions which are self-contained (it's ok to depend on a sub module in your main module, but not OK for sub-modules to depend on each other).