Go to Qiita Advent Calendar 2024 Top
LoginSignup
2
0

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?

More than 5 years have passed since last update.

@zephyria

[リファクタリング]リファクタのリファクタ。もしくはある日のきのこたけのこ大戦争～('ω`)ｷｭｩ...～

Posted at

### *[アドベントカレンダー 2019年]* #### *13th December 2019*

## これ、完全に身内用やねんな。
## 外野さんは細かい突っ込みは無しで頼むんやで。
## そのレベル？的な内容から書いてるでな。

前回やらかしたけど気を取り直していくぞーって言う。
そして見慣れてきたであろうこの表↓

  1. 内容の純粋な整理(不要な変数消したり、各種名前を整えたり。)
  2. 計算回数を減らす -> 効率性がUP!
  3. 見え方、ぱっと見の分かり易さを向上する -> 保守性がUP!
  4. 新しい記法のトライアルで -> 結論が分かっている機能だからナレッジに転化しやすい

今回は "3." について書いていきましょう。

例のリファクタリングコード

まずはいつもの再掲ですが、今回の主題となっているコードが以下になります。
16人の研修生にユニークな座席番号を割り振るって機能ですね。

selecter.sh
#!/bin/sh
readonly NUMBER_OF_TRINEE=16
selected_seats=()
NUM=0
for i in `seq $NUMBER_OF_TRINEE`
do

  echo ENTERを押してくださいね。:
  read

  while true
  do
    NUMWK=`expr $RANDOM % $NUMBER_OF_TRINEE`

    for i in ${selected_seats[@]}
    do
      if [ $i = $NUMWK ]; then
        continue 2
      fi
    done

    NUM=$NUMWK
    break
  done

  selected_seats=("${selected_seats[@]}" $NUM)
  #結果を表示
  echo $NUM

done
echo 全員セットできました。
exit 0

で、これをどうリファクタするかーってのが今回の主題です。
"2." の際に改修した内容は置いて、元の状態から "3." の観点で直してみましょう。

現状分析

しかし、この観点は結構戦争を起こしがちなんですよね。
最もきのこたけのこになり易いと言いましょうか。

先に断っておくと、僕(もしくは弊社)は基本的にjavaやphpやpythonが主言語なので、
shellでやるなよって言うオブジェクト指向寄りの内容で以下記載をします。
shellや手続き型を使ってきた諸兄からの反発は大きいと思いますが、
あくまで題材としてshellを使わせていただいたということで悪しからず。

まずはこの機能ですが、大きく処理機能に名前を付けようかと思います。
恐らくこの時点で既に火種が大きくなり始めそうな気もしますが、
気にせず行きましょう。

a. 16人と言う分母に対してコントロールを行う機能
b. 入力(Enter押下)を促し、UI的な制御を行う機能
c. ランダムな数字を取得する機能
d. cで取得した数字が既出のものか判定する機能
e. 結果を画面表示する機能

大きく分けるとこんな感じでしょうか。
これは今のコードの現状を読み取っただけなので、
おそらく人によって分析の仕方、名前の付け方にも差異があると思います。

"3." の観点ではこの機能を1年後に見てもすぐに概要が思い出せるようにリファクタして見ましょう。

各系統分けと分析の仕方

さて、この5要素を大きく分けて "a,b" の挙動制御系、 "c,d" の実処理系、 "e" の出力系に分けてみましょう。

挙動制御系.sh
#!/bin/sh
readonly NUMBER_OF_TRINEE=16
selected_seats=()
NUM=0
for i in `seq $NUMBER_OF_TRINEE`
do

  echo ENTERを押してくださいね。:
  read

　～中略～

done
実処理系.sh
#!/bin/sh
  while true
  do
    NUMWK=`expr $RANDOM % $NUMBER_OF_TRINEE`

    for i in ${selected_seats[@]}
    do
      if [ $i = $NUMWK ]; then
        continue 2
      fi
    done

    NUM=$NUMWK
    break
  done

  selected_seats=("${selected_seats[@]}" $NUM)
出力系.sh
#!/bin/sh
～色々略～
  #結果を表示
  echo $NUM

～色々略～
echo 全員セットできました。
exit 0

挙動制御系は分母全体に対する制御を行っています。
実処理系は分母の数だけ繰り返し実行され、分子に対して毎回等価な動きをします。
出力系は結果を人間が観測するために付け加えられた機能ですね。

リファクタの際に私が重要視しているのは、序で行った仕様把握とこの系統分けになります。

4.にも関連するのですが、本質的にそのプログラムが何を行っているのか把握することで、
他の表現への言い換え＝リファクタリングが容易になります。
この辺りは日本語を英語に翻訳する感覚にも近いでしょうか。

本題に戻って、今回の挙動制御系は 「分母の数だけ同じ処理を実行すること / それが使用者のEnterキー押下によって制御されること」 が本質です。
実処理系であれば 「分母の中で常にユニークな数字を取得すること」、出力系なら 「実処理系で取得した値をコンソールに出力すること」 と言えるでしょう。

僕はよく入社研修で基本的なプログラミングを教える際には、
「forで出来ることはwhileでも出来る、逆もまた然り」と言う話をします。

a. 16人と言う分母に対してコントロールを行う機能

それは、でこの "a." のレベルくらいまでは抽象的に考えるとどう言ったループ構文でも書き得る、
と言う意味で話しています。
このアドベントカレンダーを通して、そう言った伝え切れていない部分が補完出来たら嬉しいですね。

実際どこを直すのか

と言うことで、実際今はforで制御していますが、whileでもuntilでも書き直そうと思えば容易そうなことはお分かりいただけるかと思います。
しかし、書き直す必要があるかと言えば、個人的には挙動制御系は変更しなくてもいいかなぁ、と感じています。
僕は分母がはっきりしている際はforが見やすいので、この部分は触りません。
先に出力系についても、同様にシンプル化されているので別にこのままで良いでしょう。

問題は実処理系です。
仕様上やむを得ないのですが、無限ループもあり一見では意図が読み取り辛いコードになっています。
ここを少し見易くしたいですね。

実処理系.sh
#!/bin/sh
  while true
  do
    NUMWK=`expr $RANDOM % $NUMBER_OF_TRINEE`

    for i in ${selected_seats[@]}
    do
      if [ $i = $NUMWK ]; then
        continue 2
      fi
    done

    NUM=$NUMWK
    break
  done

ここをこのまま改良するのは難しいくらい整ってはいるのですが、
先輩の威厳のために 後からみて分かり易くする為にもう一工夫してみましょう。
少し処理順は変わってしまいますし、計算回数は微増しますけどね。

「分母の中で常にユニークな数字を取得すること」がやりたいことでした。
ので、ここは思い切ってそういう関数を作ってしまいましょう↓

サンプル.sh
#!/bin/sh
readonly NUMBER_OF_TRINEE=16
selected_seats=()
NUM=0
for i in `seq $NUMBER_OF_TRINEE`
do

  echo ENTERを押してくださいね。:
  read

  # 重複しない座席番号を取得
  NUM=getNumOnNotUsedYet ${NUMBER_OF_TRINEE} ${selected_seats}

  selected_seats=("${selected_seats[@]}" $NUM)
  #結果を表示
  echo $NUM

done
echo 全員セットできました。
exit 0

# name : getNumOnNotUsedYet
# args : 1-Range(for Random), 2-Used number's list
# return : Not used number.
function getNumOnNotUsedYet() {
  # エラーハンドリングは省略します。
  used_seats=$2
  while true
  do
    NUMWK=`expr $RANDOM % $1`

　　# 何もなかったらこけるかも、、初回だけエラーハンドリング必要かもです。
    for i in ${used_seats[@]}
    do
      if [ $i = $NUMWK ]; then
        continue 2
      fi
    done

    NUM=$NUMWK
    break
  done

  return $NUM
}

無限ループを抜き出しただけですが、本体側はだいぶやりたいことが整理されて見えるのではないでしょうか。
お互いに内容が独立したので、それぞれに必要な部分だけ読めばいいですし、
コードの量は増えましたが仕様の切り分けは容易になったと思います。

まとめ

関数化してリファクタと言うのはよくありますが、
意図を持ってやらないと迷走する原因になったり、より見辛くなることも多々あります。

コメントなども活用しながら、1年後に自分が見て「意図を考えなくても読めば伝わる」レベルを目指すと良いかと思います。

これはあくまで一例です。
きっと違う方法や書き方がいっぱいあるでしょう。
自身の思う理想を突き詰めて、他の人に批評してもらって、少しずつでも見やすいコードを書いていきたいものです。

実際にこの関数も "2." と合わせ技で計算回数を減らしたり出来ますし、
"4." の内容で新しい書き方・関数などを組み合わせることも出来ます。

少しまとまりにかけますが、今回はここまでにして次回に総集編を書いて全て完了にしようかと思います。

ここまで読んでくださってありがとうございましたーヽ( ･∀･)ﾉ

2
0
0

Register as a new user and use Qiita more conveniently

  1. You get articles that match your needs
  2. You can efficiently read back useful information
  3. You can use dark theme
What you can do with signing up
Sign upLogin
2
0

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?