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

2018年9月25日
BG
コードレビュー

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

レビュー

下記は知り合いが書いたプログラム。突っ込みどころ満載だ。

memory[ auto ] name NOT_INPUT = 0
memory[ auto ] name ROCK = 1
memory[ auto ] name SCISSORS = 2
memory[ auto ] name PAPER = 3

function conversion_character( memory[ auto ] name hand )
    memory[ auto ] name hand_chara = ""
    
    if hand == ROCK
        hand_chara = "グー"
    
    if hand == SCISSORS
        hand_chara = "チョキ"
    
    if hand == PAPER
        hand_chara = "パー"
    
    return hand_chara
    
function zyanken_cpu()
    memory[ auto ] name seconds = get_seconds()
    memory[ auto ] name cpu_hand = 0   
    memory[ auto ] name syou = 0
    
    syou = seconds / 3
    cpu_hand = seconds - 3 * syou + 1   
    return cpu_hand
    
function judgment( memory[ auto ] name your_hand, memory[ auto ] name cpu_hand )
    memory[ auto ] name result = 0
    memory[ auto ] name is_judgment = true
    memory[ auto ] name syou = 0
    memory[ auto ] name temp = your_hand - cpu_hand + 3
    syou = temp / 3
    result = temp - 3 * syou
    
    if result == 0
        show( "【あいこ】" )
        is_judgment = false
    
    if result == 1
        show( "【負け】" )
    
    if result == 2
        show( "【勝ち】" )
    
    return is_judgment

function select_hand()
    memory[ auto ] name player_hand = NOT_INPUT
    
    //出来ればここで値を保持したいなぁ嫌だなぁ
    if is_pushing_left_key()
        player_hand = ROCK
        
    if is_pushing_up_key()
        player_hand = SCISSORS
        
    if is_pushing_right_key()
        player_hand = PAPER
        
    return player_hand
    
function main()
    memory[ auto ] name is_result = false
    memory[ auto ] name player_hand = 0
    memory[ auto ] name player_hand_char = ""
    memory[ auto ] name cpu_hand = 0
    memory[ auto ] name cpu_hand_char = ""
    
    show( "ジャン" )
    wait( 1000 )
    show( "ケン" )
    wait( 1000 )
    show( "自分の手を選んでね" )
    show( "←:グー　↑:チョキ　→:パー" )
    
    while is_result == false
        player_hand = NOT_INPUT
        while player_hand == NOT_INPUT
            player_hand = select_hand()
            
            if player_hand != NOT_INPUT
                cpu_hand = zyanken_cpu()
                
                player_hand_char = conversion_character( player_hand )
                cpu_hand_char = conversion_character( cpu_hand )
                player_hand_char = "自分の手:" . player_hand_char
                cpu_hand_char = "相手の手:" . cpu_hand_char
                show( "ポン！" )
                show( player_hand_char )
                show( cpu_hand_char )
                
                is_result = judgment( player_hand , cpu_hand )
                
            wait( 10 )
        
        if is_result == false
            wait( 3000 )
            clear()
            show( "アイ" )
            wait( 1000 )
            show( "コデ" )
            wait( 1000 )
            show( "自分の手を選んでね" )
            show( "←:グー　↑:チョキ　→:パー" )

気になる点

該当行理由修正例
6
29		関数名が名詞で名付けられており、不自然。名詞の部分を適切な動詞に変更する。
function convert_character( memory[ auto ] name hand )
function judge( memory[ auto ] name your_hand, memory[ auto ] name cpu_hand )
6関数名が抽象的で分かりづらい。具体的な関数名に変更する。
function to_hand_name( memory[ auto ] name hand )
20と49関数名に一貫性がない。関数名を一貫性がある名前に修正する。
function select_cpu_hand()
function select_player_hand()
25～26
34～35		余りを求める処理が重複している。関数化する。
function modulo( memory[ auto ] name dividend, memory[ auto ] name divisor )
21と25～26乱数を取得する処理であることが分かりにくい。関数化する。
function get_random_number( memory[ auto ] name max_number )
30～471つの関数に「結果を表示する処理」と
「勝敗がついたか判断する値を取得する処理」が混在している。		処理を分離し、関数化する。
function show_result( memory[ auto ] name your_hand, memory[ auto ] name cpu_hand )
function is_won_or_lost( memory[ auto ] name your_hand, memory[ auto ] name cpu_hand )
31
65		何を意味する変数なのか分かりづらい。分かりやすい変数名に変更する。
memory[ auto ] name is_won_or_lost = true
memory[ auto ] name is_won_or_lost = false
52不要なコメントがある。コメントを削除する。
22
66
68		マジックナンバーを使用している。定数( NOT_INPUT )を使用する。
memory[ auto ] name player_hand = NOT_INPUT
29と50と66同じ用途の変数なのに、変数名が統一されていない。
( your_hand, player_hand )		変数名を統一する。
memory[ auto ] name player_hand = NOT_INPUT
67と69と86～92自分・相手の手を表示するのに
変数を使用する必要性を感じない。		変数を削除して、処理を修正する。
show( "自分の手:" . conversion_character( player_hand ) )
show( "相手の手:" . conversion_character( cpu_hand ) )
83～94while player_hand == NOT_INPUT
の繰り返し内で処理する必要がない。		繰り返しの外に処理を出す。

修正した場合

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

memory[ auto ] name NOT_INPUT = 0
memory[ auto ] name ROCK      = 1
memory[ auto ] name SCISSORS  = 2
memory[ auto ] name PAPER     = 3

function to_hand_name( memory[ auto ] name hand )
    memory[ auto ] name name = ""
    
    if hand == ROCK
        name = "グー"
    if hand == SCISSORS
        name = "チョキ"
    if hand == PAPER
        name = "パー"
    
    return name

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

function get_random_number( memory[ auto ] name max_number )
    return modulo( get_seconds(), max_number + 1 )
    
function select_cpu_hand()
    return get_random_number( 2 ) + 1
    
function select_player_hand()
    memory[ auto ] name player_hand = NOT_INPUT
    
    if is_pushing_left_key()
        player_hand = ROCK
    if is_pushing_up_key()
        player_hand = SCISSORS
    if is_pushing_right_key()
        player_hand = PAPER
        
    return player_hand

function is_won_or_lost( memory[ auto ] name player_hand, memory[ auto ] name cpu_hand )
    return player_hand != cpu_hand 

function show_result( memory[ auto ] name player_hand, memory[ auto ] name cpu_hand )
    memory[ auto ] name result = modulo( player_hand - cpu_hand + 3, 3 )
    
    if result == 0
        show( "【あいこ】" )
    if result == 1
        show( "【負け】" )
    if result == 2
        show( "【勝ち】" )
        
function main()
    memory[ auto ] name is_won_or_lost = false
    memory[ auto ] name player_hand    = 0
    memory[ auto ] name cpu_hand       = 0
    
    show( "ジャン" )
    wait( 1000 )
    show( "ケン" )
    wait( 1000 )
    show( "自分の手を選んでね" )
    show( "←:グー　↑:チョキ　→:パー" )
    
    while is_won_or_lost == false
        player_hand = NOT_INPUT
        cpu_hand = select_cpu_hand()
        
        while player_hand == NOT_INPUT
            player_hand = select_player_hand()
            wait( 10 )
            
        show( "ポン！" )
        show( "自分の手:" . to_hand_name( player_hand ) )
        show( "相手の手:" . to_hand_name( cpu_hand ) )
        
        is_won_or_lost = is_won_or_lost( player_hand , cpu_hand )
        show_result( player_hand, cpu_hand )
        
        if is_won_or_lost == false
            wait( 3000 )
            clear()
            show( "アイ" )
            wait( 1000 )
            show( "コデ" )
            wait( 1000 )
            show( "自分の手を選んでね" )
            show( "←:グー　↑:チョキ　→:パー" )

修正を入れることで、全体的に意図の伝わりやすいコードになったと思う。

理咲のプログラム

自分がじゃんけんを書いた場合、下記のプログラムになる。

memory[ auto ] name TYPE_ROCK      = 0
memory[ auto ] name TYPE_SCISSCORS = 1
memory[ auto ] name TYPE_PAPER     = 2
memory[ auto ] name TYPE_NONE      = -1

memory[ auto ] name PREVIOUS_LEFT_KEY_INDEX  = 0
memory[ auto ] name PREVIOUS_UP_KEY_INDEX    = 1
memory[ auto ] name PREVIOUS_RIGHT_KEY_INDEX = 2

function main()
    initialize_key_flags()
    show_description()
    
    memory[ auto ] name player_hand_type = TYPE_NONE
    memory[ auto ] name npc_hand_type    = TYPE_NONE
    memory[ auto ] name is_won_or_lost   = false
    
    while is_won_or_lost == false
        player_hand_type = TYPE_NONE
        npc_hand_type    = get_npc_hand_type()
        
        while player_hand_type == TYPE_NONE
            player_hand_type = get_player_hand_type()
            update_key_flags()
            wait( 10 )
        
        show_each_hands( player_hand_type, npc_hand_type )
        
        is_won_or_lost = is_won_or_lost( player_hand_type, npc_hand_type )
        if is_won_or_lost == false
            show_draw()
    
    show_won_or_lost( player_hand_type, npc_hand_type )
    
function initialize_key_flags()
    memory[ PREVIOUS_LEFT_KEY_INDEX  ] = false
    memory[ PREVIOUS_UP_KEY_INDEX    ] = false
    memory[ PREVIOUS_RIGHT_KEY_INDEX ] = false

function show_description()
    show( "キーを入力してください。( 左キー: グー, 上キー: チョキ, 右キー: パー )" )

function get_npc_hand_type()
    return get_random_number( 2 )
    
function get_random_number( memory[ auto ] name max_number )
    return modulo( get_seconds(), max_number + 1 )

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

function get_player_hand_type()
    if is_pushed_left_key()
        return TYPE_ROCK
    if is_pushed_up_key()
        return TYPE_SCISSCORS
    if is_pushed_right_key()
        return TYPE_PAPER
    
    return TYPE_NONE

function is_pushed_left_key()
    memory[ auto ] name is_not_pushing_previous = memory[ PREVIOUS_LEFT_KEY_INDEX ] == false
    return is_pushing_left_key() and is_not_pushing_previous 

function is_pushed_up_key()
    memory[ auto ] name is_not_pushing_previous = memory[ PREVIOUS_UP_KEY_INDEX ] == false
    return is_pushing_up_key() and is_not_pushing_previous

function is_pushed_right_key()
    memory[ auto ] name is_not_pushing_previous = memory[ PREVIOUS_RIGHT_KEY_INDEX ] == false
    return is_pushing_right_key() and is_not_pushing_previous

function update_key_flags()
    memory[ PREVIOUS_LEFT_KEY_INDEX ]  = is_pushing_left_key()
    memory[ PREVIOUS_UP_KEY_INDEX ]    = is_pushing_up_key()
    memory[ PREVIOUS_RIGHT_KEY_INDEX ] = is_pushing_right_key()

function show_each_hands( memory[ auto ] name player_hand_type, memory[ auto ] name npc_hand_type )
    show( "自分: " . get_hand_name( player_hand_type ) )
    show( "相手: " . get_hand_name( npc_hand_type ) )

function get_hand_name( memory[ auto ] name hand_type )
    if hand_type == TYPE_ROCK
        return "グー"
    if hand_type == TYPE_SCISSCORS
        return "チョキ"
    if hand_type == TYPE_PAPER
        return "パー"
        
    return ""

function is_won_or_lost( memory[ auto ] name player_hand_type, memory[ auto ] name npc_hand_type )
    return player_hand_type != npc_hand_type
    
function show_draw()
    show( "あいこです。もう一度キーを入力してください。" )

function show_won_or_lost( memory[ auto ] name player_hand_type, memory[ auto ] name npc_hand_type )
    if is_won( player_hand_type, npc_hand_type )
        show( "勝ちました。" )
        return
    
    show( "負けました。" )    

function is_won( memory[ auto ] name player_hand_type, memory[ auto ] name npc_hand_type )
    if player_hand_type == TYPE_ROCK      and npc_hand_type == TYPE_SCISSCORS
        return true
    if player_hand_type == TYPE_SCISSCORS and npc_hand_type == TYPE_PAPER
        return true
    if player_hand_type == TYPE_PAPER     and npc_hand_type == TYPE_ROCK
        return true
    
    return false

知り合いとの主な違いは、キーの連続入力の防止方法。( 知り合いがwait関数で対処しているのに対し、自分はmemoryで対処している )

知り合いのwait関数で対処法は予想外だったため、見たときに少々驚いた。