HiveBrain v1.2.0
Get Started
← Back to all entries
patternModerate

Batch-File FizzBuzz

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
filebatchfizzbuzz

Problem

I've never done a FizzBuzz before, so I decided I would. Please let me know what I could have done better. This is ran via a .bat file in Windows.

@echo off
  set loopStart=1
  set loopEnd=100
  set /a i=%loopStart% - 1  
:LoopStart

  if %i% == %loopEnd% goto :LoopEnd

  set /a i=%i% + 1
  set /a fizzbuzz=%i%%%15
  set /a buzz=%i%%%5
  set /a fizz=%i%%%3

  if %fizzbuzz% == 0 (
    echo FizzBuzz
  ) else if %buzz% == 0 (
    echo Buzz
  ) else if %fizz% == 0 (
    echo Fizz
  ) else (
    echo %i%
  )

  goto :LoopStart

:LoopEnd
  set loopStart=
  set loopEnd=
  set i=
  set fizzbuzz=
  set buzz=
  set fizz=

Solution

Use FOR /L to iterate a range of numbers - much faster than SET /A with GOTO loop.

Use SETLOCAL to localize variables so no need to undefine them. The EnableDelayedExpansion option does just that so that you can use !var! within a FOR loop.

A minor optimization - it is faster to combine multiple math computations into one SET /A.

@echo off
setlocal enableDelayedExpansion
for /l %%N in (1 1 100) do (
set /a "fizzbuzz=%%N%%15, buzz=%%N%%5, fizz=%%N%%3"
if !fizzbuzz! == 0 (
echo FizzBuzz
) else if !buzz! == 0 (
echo Buzz
) else if !fizz! == 0 (
echo Fizz
) else echo %%N
)


You might consider parameterizing the start and end conditions of the loop.

@echo off
REM %1=start %2=end
setlocal enableDelayedExpansion
for /l %%N in (%1 1 %2) do (
set /a "fizzbuzz=%%N%%15, buzz=%%N%%5, fizz=%%N%%3"
if !fizzbuzz! == 0 (
echo FizzBuzz
) else if !buzz! == 0 (
echo Buzz
) else if !fizz! == 0 (
echo Fizz
) else echo %%N
)


You could even parameterize the two divisors, but then a different algorithm is needed. Here is a fully parameterized solution that is efficient, though perhaps a bit obfuscated. I still use a modulo operation to test if divisible, but instead of an IF statement, I intentionally divide by zero to raise an error and trigger the conditional execution of the SET statement. Of course I redirect error messages to NUL to avoid unwanted error messages..

@echo off
REM %1=start %2=end %3=divisor1 %4=divisor2
setlocal enableDelayedExpansion
for /l %%N in (%1 1 %2) do (
set "val="
set /a "1/(%%N %% %3)" 2>nul || set "val=Fizz"
set /a "1/(%%N %% %4)" 2>nul || set "val=!val!Buzz"
if defined val (echo !val!) else echo %%N
)

Context

StackExchange Code Review Q#36735, answer score: 10

Revisions (0)

No revisions yet.