Improvments

[Rapidhands]

Hi Mike, A long time ago, you wrote the module. Today, I've studied with attention, some of the internal functions. i.e. Test-AdServices. I've modified your code to

• Send only one mail if one or more services are stopped for one or more Domain Controller
• Adding some comments with the Write-Verbose cmdlet.

function Test-ADServices
{
    [cmdletBinding()]
    [OutputType([System.Object])]
    Param()

    begin
    {
        Import-Module ActiveDirectory
    }

    process
    {
        Write-Verbose -Message 'Restrieve Domain Controllers List from AD'
        $DClist = (Get-ADGroupMember 'Domain Controllers').name
        Write-Verbose -Message 'List of Services to monitor'
        $Collection = @('ADWS',
            'DHCPServer',
            'DNS',
            'DFS',
            'DFSR',
            'Eventlog',
            'EventSystem',
            'KDC',
            'LanManWorkstation',
            'LanManServer',
            'NetLogon',
            'NTDS',
            'RPCSS',
            'SAMSS',
            'W32Time')
        $Collection

        Write-Verbose -Message 'Retreive Info on each DC'
        forEach ($Server in $DClist)
        {
            Write-Verbose -Message "Retrieve Services Status for $Server"
            Write-Verbose -Message 'EmailBody and Subject initialization'
            $EmailBody = @'

'@
            $Subject = ''
            forEach ($Service in $Collection)
            {
                try
                {
                    $s = Get-Service -Name $Service -Computername $Server -ErrorAction Stop
                    $s
                }
                catch
                {
                    Out-Null
                }


                if ($s.status -eq 'Stopped')
                {
                    $Subject = 'Somme Windows Services for AD are stopped on Domain Controllers'
                    $EmailBody = @"
                                Service named <font color=Red><b>$($s.Displayname)</b></font> is stopped on $Server!
                                Time of Event: <font color=Red><b>"""$((Get-Date))"""</b></font><br/>
                                <br/>
"@
                    Write-Verbose -Message 'Adding EmailBody to previous (if existing)EmailBody'
                    $EmailBody += $EmailBody
                } #End If
            } #End Service Foreach
        } #End DCList Foreach
        If ($Null -ne $Subject)
        {
            <#
            By this way, There is only one single mail send if a service is stopped on one or more Domain Controller
            #>
            Write-Verbose -Message 'One or more Services are stopped. Send Mail'
            Write-Verbose 'Adding a final info into the EmailBody'
            $EmailBody += @'
                                <br/>
                                THIS EMAIL WAS AUTO-GENERATED. PLEASE DO NOT REPLY TO THIS EMAIL.
'@
            $MailParams = @{
                To         = $Configuration.MailTo
                From       = $Configuration.MailFrom
                SmtpServer = $Configuration.SmtpServer
                Subject    = $Subject
                Body       = $EmailBody
                BodyAsHtml = $true
            }
            Send-MailMessage @MailParams
        }
    } #End Process
} #End function

I've verified the code with the cmdlet Invole-ScriptAnalyzer (from PSScriptAnalyzer module), and now there are only 1 warning and 1 Information RuleName Severity ScriptName Line Message

---

PSUseSingularNouns Warning Test-ADSer 1 The cmdlet 'Test-ADServices' uses a plural noun. A singular vices.ps1 noun should be used instead. PSUseOutputTypeCorrectly Information Test-ADSer 32 The cmdlet 'Test-ADServices' returns an object of type
vices.ps1 'System.Object[]' but this type is not declared in the
OutputType attribute. For the second advice, it seems simple to resolve it. For the first one, this requires to change the name of the function.

Another generic potential issue : Often, I'm using an AD account from another domain. There is a Trust relationship between the 2 domain, and my account has admin rights on the domain i'm logged on. If i'm using Get-AdDomain, the cmdlet retreive the corresponding info from the domain where my account is, but not the info from the the domain im' currently logged on. The cmdlet has a parameter named -Current (possible values are LocalComputer or CurrentLoggedOnUser), it seems to me that you should add this parameter with value LocalComputer to avoid this issue, or add this parameter as a paramter for your function.

A last improvement will be to add a self help section in your function.

It's always a pleasure to read your posts on 4SysOp or other sites.

Regards P.S. : sorry if my english is not perfect, it's not my native tongue. I do my best :-)