【コードレビュー】知り合いが書いたプログラムをレビューしてみた

2018年8月29日
BG
コードレビュー

とある知り合いがBGで「Fizz Buzz」を書いたのでレビューしてみた。

レビュー

下記は知り合いが書いたプログラム。よく見ると「ん？」ってなるコードが多数ある。

function amari( memory[ auto ] name waru, memory[ auto ] name count )
    memory[ auto ] name syou = 0
    memory[ auto ] name amari = 0
    memory[ auto ] name is_flag = false
    
    syou = count / waru        
    amari = count - waru * syou
    
    if amari == 0
        is_flag = true
    
    return is_flag
        
function main()
    memory[ auto ] name count = 1
    memory[ auto ] name amari = 0
    
    memory[ auto ] name is_fizzflag = false
    memory[ auto ] name is_buzzflag = false
    
    while count <= 30
        is_fizzflag = amari( 3, count )
        is_buzzflag = amari( 5, count )
        
        //FizzだけならFizz
        //BuzzだけならBuzz
        //両方ならFizzBuzz
        //両方立ってないなら数字
        if is_fizzflag == true
            if is_buzzflag == true
                show( "FizzBuzz" )
            if is_buzzflag == false
                show( "Fizz" )
                
        if is_buzzflag == true
            if is_fizzflag == false
                show( "Buzz" )
                
        if is_fizzflag == false
            if is_buzzflag == false
                show( count )
                
        count = count + 1
        is_fizzflag = false
        is_buzzflag = false

気になる点

該当行理由修正例
2と6
3と7		変数の作成と計算した値の代入を分ける必要がない。変数の作成時に計算した値を代入する。
memory[ auto ] name syou = count / waru
4と9と10分岐する必要がない。分岐を削除し、変数の作成時に比較した結果を代入する。
memory[ auto ] name is_flag = amari == 0
4意味ある変数名を付けてない。意味のある変数名に変更する。
memory[ auto ] name is_divisible = 0
12amariという名の関数が何故か論理値を返す。
( 関数名と戻り値がかみ合ってない )		余りを返却する。
return amari
16amariという名の変数は一度も使用してない。変数を削除する。
18と19変数名にflagを付けなくても意味が通る。flagを部分を削除する。
memory[ auto ] name is_fizz = false
29～41分岐が複数使われており、複雑。論理値を格納する変数を増やし、
その変数を使用して分岐を簡易にする。
memory[ auto ] name is_fizz_buzz = false
memory[ auto ] name is_count = false
22と44
23と45		毎ループで必ずamari関数の戻り値を代入するため、
falseを代入する必要がない。		falseを代入している文を削除する。

修正した場合

上記の気になる点を全て修正すると、下記のようなプログラムとなる。

function amari( memory[ auto ] name waru, memory[ auto ] name count )
    memory[ auto ] name syou  = count / waru
    memory[ auto ] name amari = count - waru * syou
    
    return amari
        
function main()
    memory[ auto ] name count        = 1
    memory[ auto ] name is_fizz      = false
    memory[ auto ] name is_buzz      = false
    memory[ auto ] name is_fizz_buzz = false
    memory[ auto ] name is_count     = false
    
    while count <= 30
        is_fizz      = amari( 3, count ) == 0
        is_buzz      = amari( 5, count ) == 0
        is_fizz_buzz = is_fizz and is_buzz
        is_count     = is_fizz == false and is_buzz == false
        
        if is_fizz_buzz
            show( "FizzBuzz" )
            
            is_fizz = false
            is_buzz = false
        
        if is_fizz
            show( "Fizz" )
        if is_buzz
            show( "Buzz" )
        if is_count
            show( count )
        
        count = count + 1

修正を入れることで不要なコードが減り、分岐がシンプルになったため、より良いプログラムとなったのではないか。

理咲のプログラム

ちなみに自分がFizz Buzzを書くと下記のプログラムになる。

function main()
    memory[ auto ] name max_number = 40
    memory[ auto ] name number     = 1
    
    while number <= max_number 
        show_fizz_buzz( number )
        number = number + 1
    
function show_fizz_buzz( memory[ auto ] name number )
    memory[ auto ] name is_fizz = modulo( number, 3 ) == 0
    memory[ auto ] name is_buzz = modulo( number, 5 ) == 0
    
    if is_fizz and is_buzz 
        show( "Fizz Buzz" )
        return
    
    if is_fizz
        show( "Fizz" )
        return
    
    if is_buzz
        show( "Buzz" )
        return
    
    show( number )

function modulo( memory[ auto ] name dividend, memory[ auto ] name divisor )
    memory[ auto ] name quotient = dividend / divisor
    return dividend - quotient * divisor

知り合いと主な違いは、Fizz Buzzの出力部を関数化して、return文で分岐を対処してるところ。簡単なコードでも割と違いが出て面白い。