7
\$\begingroup\$

I decided to help @Matt out a bit and ask a PowerShell question. This is a simple guess-the-number game learning how to use functions with and without parameters, and use the input and output functions. All comments are welcome.

Function GetNumber([int]$min, [int]$max) {
    Return Get-Random -minimum $min -maximum ($max + 1)
}

Function PlayGame() {
    Write-Host "This is a guess-the-number game.`r`nYou have 6 guesses to guess a number between 1 and 100."

    $secretNum = GetNumber 1 100

    [int]1..6 | ForEach {
        $guess = Read-Host
        If ($guess -eq $secretNum) {
            Write-Host "Correct!"
            Return
        }
        ElseIf ($guess -gt $secretNum) {
            Write-Host "Too high!"
        }
        Else {
            Write-Host "Too low!"
        }

        $remainingGuesses = 6 - $_
        Write-Host "You have $remainingGuesses guesses left"
    }

    Write-Host "The number was $secretNum"
}

PlayGame

# Borrowed from https://technet.microsoft.com/en-us/library/ff730938.aspx
Write-Host "Press any key to continue ..."
$host.UI.RawUI.ReadKey("NoEcho, IncludeKeyDown")
\$\endgroup\$
8
\$\begingroup\$

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 purpose there is no confusion here. Therefore I simply copied the parameter block from your other function!

Function Naming

PowerShell function naming recommendations are Verb-Noun(s). You see this is all stock cmdlets. GetNumber works fine as it is doing just that... Getting a number. Only thing that could improve that is specifying that you are getting a random number. Get-RandomNumber. PlayGame could use a hyphen but it is nit-picky as both functions describe

Watch out for your types

When using range operator

[int]1..6

Casting as an [int] here is redundant. From about_operators:

.. Range operator

Represents the sequential integers in an integer array, given an upper and lower boundary.

It cannot be anything but integers for that operation to work.

Left hand operators

Quill talks about this in his answer. You can also see people abbreviate this and call it LHS (Left Hand Side). To continue Read-Host is returning strings. You are doing string based comparisons in your if blocked. Not arithmetic comparisons as you may have thought.

Validate user input

Users cannot be trusted. So lets include a small function that will validate the input from the user. It will also repeatedly ask the user for a value until it is within bounds.

Function Get-ValidGuess([int]$min, [int]$max){
    # Loop until we get a number within the upper and lowerbounds ie. valid result.
    do{
        $value = (Read-Host -Prompt "Please enter a number $min and $max") -as [int]
    } until ($value -ge $min -and $value -le $max)
}

The -as operator will attempt to convert the LHS to an integer. If it cannot a $null value is returned.

Smaller points

Use of the return keyword

It can be misleading as times. Functions will return all data sent to the output stream up until the end of the function or return keyword. Return can still be used as an endpoint. I don't personally use it but it most cases there is no harm and notifies the user of the logical exit of the function. PlayGame does rely on this functionality so I propose no changes. Just be aware that extra output can come out from functions sometimes.

ReadKey in ISE

Use of the method ReadKey would generate an error if run in the ISE. You could have a check of the host to mitigate this possibility: (Get-Host).Name -match "ISE". If true you could use pause.

More Number Validation

For all we did with Read-Host there are several valid inputs to the function that could result in odd behavior. If you think people are not capable of making mistakes when calling your PlayGame function then you will be fine.


Function GetNumber([int]$min, [int]$max) {
    Return Get-Random -minimum $min -maximum ($max + 1)
}

Function Get-ValidGuess([int]$min, [int]$max){
    # Loop until we get a number within the upper and lowerbounds ie. valid result.
    do{
        $value = (Read-Host -Prompt "Please enter a number $min and $max") -as [int]
    } until ($value -ge $min -and $value -le $max)
    return $value
}

Function PlayGame([int]$min, [int]$max, [int]$NumberOfGuesses) {
    Write-Host "This is a guess-the-number game.
You have $NumberOfGuesses guesses to guess a number between $min and $max."

    $secretNum = GetNumber $min $max

    1..$NumberOfGuesses | ForEach {
        $guess = Get-ValidGuess $min $max
        If ($guess -eq $secretNum) {
            Write-Host "Correct!"
            Return
        }
        ElseIf ($guess -gt $secretNum) {
            Write-Host "Too high!"
        }
        Else {
            Write-Host "Too low!"
        }

        $remainingGuesses = $NumberOfGuesses - $_
        Write-Host "You have $remainingGuesses guesses left"
    }

    Write-Host "The number was $secretNum"
}

PlayGame -min 1 -max 100 -NumberOfGuesses 6

# Borrowed from https://technet.microsoft.com/en-us/library/ff730938.aspx
Write-Host "Press any key to continue ..."
$host.UI.RawUI.ReadKey("NoEcho, IncludeKeyDown")
\$\endgroup\$
8
\$\begingroup\$

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-Random -minimum 1 -maximum ($max + 1)
    #...
}
PlayGame 100

Multiline strings:

Instead of including new line characters in the string, use a multiline string, like so:

    Write-Host @"
This is a guess-the-number game. 
You have 6 guesses to guess a number between 1 and 100.
"@

Note that any indentation you add ends up in the resulting string.


Type comparison

In PowerShell, as written in this Stack Overflow post:

On comparison a right value is converted to the type of a left value. Thus, '2' -gt 9 becomes '2' -gt '9', that is False, and '2' -gt 10 becomes '2' -gt '10', that is True.

This is a potential bug. I would recommend a helper method that converts the input to an integer, and tests its boundaries. Obviously in a number guessing game, 100000000 isn't a valid guess for a limit of 100. Not to mention the above type coercion.

If you don't really care about people putting in massive amounts because they're idiots, than you can just swap the order of the operators, and the guess will be coerced to an integer.

\$\endgroup\$

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.