11
\$\begingroup\$

After reading way too much into what was I though PowerShell could easily have its way with this.

Function Get-FizzBuzz{
    param(
        [Parameter(Position=0,ValueFromPipeline=$true)]
        [int]$Number
    )

    process{
       $result = switch($Number){
            {$_ % 3 -eq 0}{"Fizz"}
            {$_ % 5 -eq 0}{"Buzz"}
            default{$_}
       }
       -join $result

    }
}

What we have is a function that accepts pipeline input. For each number that is passed we run it through a switch. Be default it will check against both clauses. So when it hit 15 it will run for Fizz and Buzz. If neither are matched then the number just continues on without being converted. Since FizzBuzz must appear on the same line we capture the $result and use -join to concatenate the elements. This only has an effect on the $result when it has more than element.

Sample Output

PS C:\Windows\system32> Get-FizzBuzz -Number 15
FizzBuzz

PS C:\Windows\system32> 1..15 | Get-FizzBuzz
1
2
Fizz
4
Buzz
Fizz
7
8
Fizz
Buzz
11
Fizz
13
14
FizzBuzz

Not sure what could be done beyond this. I suppose you could argue that a filter as supposed to a function would be more terse. I leave that to a reviewer to chew on.

\$\endgroup\$
  • \$\begingroup\$ As far as I understand it, the usual output is FizzBuzz, not Fizz\nBuzz for multiples of both \$\endgroup\$ – Rob Jan 14 '16 at 4:11
  • \$\begingroup\$ @Rob Thanks for noticing. I made an appropriate update as a result. \$\endgroup\$ – Matt Jan 14 '16 at 4:24
  • \$\begingroup\$ I would expect this function to be named `ConvertTo-FizzBuzz', as is Converts, it doesn't Get. \$\endgroup\$ – oɔɯǝɹ Jan 4 '18 at 12:49
5
\$\begingroup\$

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 parameters are declared in the function.


The following comments are just my personal preferences. If this were a real code review for production code, I wouldn't insist on any of it. Feel free to ignore!


I think the switch statement there is elegant but a little confusing. In most languages, switch will only ever select a single branch. When I see "switch", I expect it to select one branch only. That's just a habit of thought. Your code took me by surprise. I had to stop and think about how it managed to get both "Fizz" and "Buzz" for the number 15.

I'm almost tempted to say that multiple selection in switch statements is a misfeature of PowerShell because it violates the principle of least surprise. I'm torn on that question however because it is a cool feature.

Personally I would have used two if statements. Even though that would have been clunklier, it would have been clearer and more explicit in my opinion, and therefore better.


Regarding this line:

-join $result

That is correct according to the help. -join can indeed be a unary operator. It looks weird though. I'd never seen it like that before, and it took me by surprise. Personally I would have used it as a binary operator in the usual fashion:

$result -join ""
\$\endgroup\$
  • 1
    \$\begingroup\$ "If this were a real code review for production code, I wouldn't insist on any of it." On the contrary, if this is what the company style guide dictates, you absolutely should! It helps maintain consistency and make it easier for everyone to read everyone's code. \$\endgroup\$ – Nic Hartley Jan 21 '16 at 0:20

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.