Java言語で学ぶリファクタリングの練習問題について

Question

現在「Java言語で学ぶリファクタリング(結城 浩著)」を進めています。
本著の第6章練習問題6-2について質問させていただきます。

この問題と模範解答は実際のコードが記載しておらずクラス図しか記載してありません。
そのため以下に示すコードは私がクラス図から起こしたものです。（コードに起こす段階から勘違いしている可能性もありえます。）

問題は以下のとおりです。

Playerクラスは、音楽やビデオを再生するクラスです。setMediaメソッドで音楽かビデオを選択し、play,loop,stopの各メソッドで、それぞれ再生、ループ、停止の処理をおこないます。音楽とビデオの再生方法は異なるので、音楽に大してplayメソッドを実行する場合には内部的にplayMusicメソッドを呼び出し、ビデオの場合には内部的にplayVideoメソッドを呼びだします(loop,stopについても同様)。

このクラスの問題点を指摘し、改善案を示してください。

//問題があるクラス

public class Player {

	public static final int MUSIC = 1;
	public static final int VIDEO = 2;

	private int currentMedia;
	private String musicData;
	private String videoData;

	public Player(String musicData, String videoData){
		this.musicData= musicData;
		this.videoData= videoData;
	}

	public void setMedia(int currentMedia){
		this.currentMedia = currentMedia;
	}

	public void play(){
		if(currentMedia == MUSIC){
			playMusic();
		}else if(currentMedia == VIDEO) {
			playVideo();
		}
	}

	public void loop(){
		if(currentMedia == MUSIC){
			loopMusic();
		}else if(currentMedia == VIDEO) {
			loopVideo();
		}
	}

	public void stop(){
		if(currentMedia == MUSIC){
			stopMusic();
		}else if(currentMedia == VIDEO) {
			stopVideo();
		}
	}

	private void stopVideo() {
		System.out.println("stopVideo:" + videoData);
	}

	private void stopMusic() {
		System.out.println("stopMusic:" + musicData);
	}

	private void loopVideo() {
		System.out.println("loopVideo:" + videoData);
	}

	private void loopMusic() {
		System.out.println("loopMusic:" + musicData);
	}

	private void playVideo() {
		System.out.println("playVideo:" + videoData);
	}

	private void playMusic() {
		System.out.println("playMusic:" + musicData);
	}
}

この問題に対する私の解答は以下のとおりです。

//改善後のクラス(私の解答)
public interface Player {

	public void play();

	public void loop();

	public void stop();
}

class Media {
	String data;

	public Media(String data){
		this.data = data;
	}

	public String toString(){
		return data;
	}
}

class MusicPlayer implements Player {
	Media data;
	public MusicPlayer(Media data){
		this.data = data;
	}

	@Override
	public void play(){
		System.out.println("MusicPlayer:play:" + this.data);
	}

	@Override
	public void loop(){
		System.out.println("MusicPlayer:loop:" + this.data);
	}

	@Override
	public void stop(){
		System.out.println("MusicPlayer:stop:" + this.data);
	}
}

class VideoPlayer implements Player {
	Media data;
	public VideoPlayer(Media data){
		this.data = data;
	}

	@Override
	public void play(){
		System.out.println("VideoPlayer:play:" + this.data);
	}

	@Override
	public void loop(){
		System.out.println("VideoPlayer:loop:" + this.data);
	}

	@Override
	public void stop(){
		System.out.println("VideoPlayer:stop:" + this.data);
	}
}

play,stop,loopといったPlayerクラスの動作を表すメソッドに着目し、VideoPlayerクラスとMusicPlayerクラスの2種類が必要と考えました。
Mediaクラスはあくまでデータの塊であり、動作を表すメソッドは存在しません。

一方で、本著の模範解答は以下の通りでした。

//改善後のクラス(模範解答)
public class Player {
	Media media;

	public void play(){
		System.out.println("Player:play");
		media.play();
	}

	public void loop(){
		System.out.println("Player:loop");
		media.loop();
	}

	public void stop(){
		System.out.println("Player:stop");
		media.stop();
	}

	public void setMedia(Media media){
		this.media = media;
	}
}

class Media {
	String data;

	public Media(String data){
		this.data = data;
	}

	public void play(){
		System.out.println("Media:play");
	}

	public void loop(){
		System.out.println("Media:loop");
	}

	public void stop(){
		System.out.println("Media:stop");
	}
}

class Music extends Media{

	public Music(String data){
		super(data);
	}

	@Override
	public void play(){
		System.out.println("Music:play:" + this.data);
	}

	@Override
	public void loop(){
		System.out.println("Music:loop:" + this.data);
	}

	@Override
	public void stop(){
		System.out.println("Music:stop:" + this.data);
	}
}

class Video extends Media{

	public Video(String data){
		super(data);
	}

	@Override
	public void play(){
		System.out.println("Video:play:" + this.data);
	}

	@Override
	public void loop(){
		System.out.println("Video:loop:" + this.data);
	}

	@Override
	public void stop(){
		System.out.println("Video:stop:" + this.data);
	}
}

以上です。

どうも模範解答に納得できません。
MusicやVideoというデータを表すクラスがplay,stop,loopといった動作を表すメソッドをもっていることに違和感を感じてしまいます。

しかし、私の解答ですと、Mediaクラスのインスタンスと、どちらかのPlayerクラスのインスタンスの2つをつくらなければなりませんし、setMediaメソッドであとからPlayerの動作を変化させることはできないなど使い勝手が悪い気もします。

長くなりましたが、お聞きしたいことは以下です。
・私の解答に問題はないか？
・模範解答のように、MusicクラスとVideoクラスに動作を表すメソッドが存在してもよい理由はなにか？

よろしくお願いします。

Comments

[eripong]

http://www.hyuki.com/ref/#downloadからダウンロードできるzipファイルの中のsrc/ReplaceTypeCodeWithStateStrategy/AnsSmell/Player.javaに回答らしきコードがあるので、模範解答はそれに差し替えた方が良いかもしれません。

[nakanohitobot]

ご指摘ありがとうございます。
ご指摘のコードは、次の章の練習問題の解答で、本問題の解答ではないようです。
ただそれとは別に、「問題があるクラス」で一部間違いがあることに気付きました。自分で色々といじっている内に修正後のコードと混じってしまったのだと思います。
該当部分を修正させていただきます。

Answer 1

ここでのリファクタリングの目的は、次のような変化に対処しやすくすることだと思います。

1. video と music とは別の種類の mdeia を追加しやすくする。
2. 再生、ループ、停止とは別の種類の操作を追加しやすくする。

リファクタリング前のコードでは、media の種類を追加する場合は、次の事が問題です。
play(), loop(), stop() 中の if 分岐に それぞれ 条件分岐を追加していくことが必要です。
これは間違いや追加漏れが発生しやすいです。
操作を追加する場合も、その中に media 種類ごとの条件文を書くことが必要で、↑の場合と同様に間違いが発生しやすいです。

・質問文の解答に問題はないか？
==> Media の種類を追加して、アプリケーション側で play() を行う場合、
Media の種類ごとの条件分岐を増やす必要がある。

リファクタリングの目的は達成できているかもしれません。
でも使う側にとっては、リファクタリング前のコードの方が使いやすいです。

・模範解答のように、MusicクラスとVideoクラスに動作を表すメソッドが存在してもよい理由はなにか？
==> オブジェクト指向の基本は、オブジェクトにメッセージを投げることです。
Music というオブジェクトに play() のようなメッセージを投げることは問題ではありません。

 シューティングゲームを作る場合、自機を表すオブジェクトを作ると思います。
 そのオブジェクトに 位置を移送する move() や 弾を撃つ hit() といった動作を表すメソッドを追加することは、
 極めて自然な事と思います。
 

わたしなら、次のように書きます。
模範解答との差は extends でなく interface をつかっている点です。
(extends にすると、java では多重継承ができないので将来 拡張がしにくくなる可能性がある)

コメントとして、pause() 操作を追加する場合のことと、video クラスを追加する場合のことを書いてあります。

public class SamplePlayer {
    public static void main(String[] args) throws Exception {
        Player player = new Player();
        Media music = new Music("ABBA");
        player.setMedia(music);
        player.play();
        player.stop();
    }
}

class Player {
    Media media = null;

    public void setMedia(Media media) {
        this.media = media;
    }

    public void play() throws Exception {
        System.out.println("Player:play");
        this.media.play();
    }

    public void loop() throws Exception {
        System.out.println("Player:loop");
        this.media.loop();
    }

    public void stop() throws Exception {
        System.out.println("Player:stop");
        this.media.stop();
    }

    // public void pause(Media media) {
    // System.out.println("Player:pause");
    // this.media.pause();
    // }
}

interface Media {
    public void play() throws Exception;

    public void loop() throws Exception;

    public void stop() throws Exception;
    // public void pause() throws Exception;
}

class Music implements Media {
    private final String data;

    public Music(String data) {
        this.data = data;
    }

    @Override
    public void play() {
        System.out.println("Music:play:" + this.data);
    }

    @Override
    public void loop() {
        System.out.println("Music:loop:" + this.data);
    }

    @Override
    public void stop() {
        System.out.println("Music:stop:" + this.data);
    }

    // @Override
    // public void pause() {
    // System.out.println("Music:pause:" + this.data);
    // }
}

// class Video implements Media {
// private final String data;
// ...
// }

実行したときの出力

Player:play
Music:play:ABBA
Player:stop
Music:stop:ABBA

Comments

[nakanohitobot]

ご回答ありがとうございます。
わざわざコードまで示していただいてとても助かります。
シューティングゲームの例えは納得できました。確かに動作を表すメソッドを追加するのは自然ですね。
オブジェクトにメッセージを投げて、処理を指示するのがオブジェクト指向なのですね。
まだピンと来ないのが正直なところですが、今後慣れていこうと思います。

重ねて申し訳ありませんが、示していただいたコードについて２点ほど質問させてください。
・全てのメソッドに「throws Exception」がついているのはなぜでしょうか？
・Musicクラスのフィールド変数にfinalを付けているのはなぜでしょうか？(private final String data)

以上です。よろしくお願いします。

[katoy]

> 全てのメソッドに「throws Exception」がついているのはなぜでしょうか？
> Musicクラスのフィールド変数にfinalを付けているのはなぜでしょうか？
どちらも、質問に対する回答としては本質的なことではありません。

throws Exception をつけたのは、エラー処理を意識してほしかったからです。
(アプリケーションとしてこれを発展させて行く場合、エラーをどのように扱うかを初期段階から意識しておくべきです。
Excepiton を発生させるのは誰か、どんな Exception の種類にするか、Exception を catch してエラー処理をするのは誰か)

final をつけているのは、コンストラクタで設定した後は、そのフィールドに値は変化させたくないと思ったからでした。

[nakanohitobot]

ご回答有り難うございます。
解説していただければ納得できるのですが、なかなかそこまで気が回りませんね。
今後意識するよう心がけていきたいと思います。

Answer 2

こんにちは。

う～ん、そもそも元のプログラムと回答プログラムの外部からみた動きが異なるので、リファクタリングになっていないように思います。模範解答とnakanohitobotさんのプログラムはかなり近いですが、元のプログラムだけ大きく異なってます。書き起こし失敗されてますよね？

setMediaメソッドであとからPlayerの動作を変化させることはできないなど使い勝手が悪い気もします。

機能を変えてしまうと、そもそもリファクタリングではないと思います。

---

プログラムの構造としては、私はnakanohitobotさんの構造の方が好ましく思います。プログラムの構造は現実に近いモデルにしておいた方が、可読性／メンテナンス性にすぐれていると考えているからです。プレイヤーとメディアを分離するnakanohitobotさんの案の方がより現実世界に近いモデルになっています。

ただ、元のプログラムの外部I/Fが良く分からないので、制約条件が不明ですから、リファクタリングの演習問題の回答としての意見を述べることはできません。

Comments

[nakanohitobot]

ご回答有り難うございます。
確かにリファクタリングの大原則を考えれば不味いですよね私のコードは。。
クラスを修正することに夢中でそこまで意識していませんでした。
以後、肝に銘じようと思います。
プログラムの構造に関しては、人によって色々な考え方があるのですね。
私は最初、自分の考え方しかないと思っていましたが、他のかたの意見を聞くうちにそちらのほうが理にかなっているのではないかと思い始めました。
皆様の意見を聞かせていただいて少しでも精進して行ければと思います。

Answer 3

・質問1について
「interface Player」はplay,loop,stopできる物という意味を持ちます。一方で、「extend Media」はメディアの一つであるとなります、この違いはかなり大きいと思います。
質問者のコードでは、「人の動き」や「機械の動作」などもPlayerを実装する可能性があり、本来のコードの意味から離れてしまう可能性があります。
そのため、私はMediaを継承する方がいいと思います。

・質問2について
Javaでは、データだけを表すクラスは本来ありえないと思います。

クラスは「データ」と「データに対する操作」を定義した物です。

ここから、質問者のコードではMediaをデータだけを定義したクラスとなっていて、その使い方は他のクラスに任してしまう、何もしない仲介者となっています。

Comments

[Stripe]

誤）クラスは「データ」と「データに対する操作」を定義した物です。
正）オブジェクトは「データ」と「データに対する操作」を定義した物です。

データだけを表すクラスは普通にありえます。

[yona]

クラスで属性と振る舞いを定義し、オブジェクトで定義された内容を使うので、私の書き方は間違っていないと思います。

「データだけのクラスはあり得ない」は個人的な意見となります。
「クラスは自分が保持するデータとその使い方に責任を持つべき」と言う考えから来ています。

[nakanohitobot]

ご回答有り難うございます。
確かに、Mediaクラスをデータだけとすると、他の機能は全てPlayerクラスが担わなければならなくなりますね。機能追加のことまで今は考えられませんが、徐々にこういう視点も身に着けていきたいと思います。
「クラスは「データ」と「データに対する操作」を定義した物」これはクラスを習い始めたころに聞いた気がしますが、今になって腑に落ちました。
この原則に従えばMediaクラスがデータだけを持つのは明らかにおかしいですね。
「クラスの責任」というのは自分で所持しているデータは自分自身で使い方を決めるということなのですね。

Answer 4

それは、「Stateパターンを利用したタイプコードの置き換え」のリファクタリングの演習問題です。

PlayerはMediaの内容に応じて、振る舞いを変えることが要件となっています。
しかし、あなたの回答にはそれがありません。Mediaの内容がMusicなのかVideoなのかが、あらかじめ分かっていることが前提となってしまっています。

とは言え、Stateパターンは状態変化に伴う振る舞いの変化を実装するものなので、あまり出題の内容が良くない気もしますが。

Comments

[nakanohitobot]

ご回答有り難うございます。
デザインパターンはまだ勉強していないのでわからないですね。
逆を言えばデザインパターンを学んで身に付ければこういう考え方が自然にできるということなのでしょうか。

[Stripe]

あなたが読んだリファクタリングの本には、「タイプコードの置き換え」について、何も解説が書かれていないんですか？

[nakanohitobot]

目次をみたところ先の章で解説しているようです。
先に一通り目を通したほうが理解が深まりそうですね。

Answer 5

Mediaのplayという名前に違和感があるのならば、playではなくstartと考えれば自然でしょうか。
実際、Androidのメディア系のメソッドはstartなどがあります。
実際の機械を考えると、「再生」や「停止」の命令を入力するのは外部の機械のインタフェースで、
実際に動くのはメディアである点を考えれば、メディアが動作のメソッドを持っていても何ら不思議ではありません。

あなたの解答において、音楽と動画において「メディアを再生・停止」などの目的がほぼ同じなのにそれをインタフェース経由であえて実装を分ける必要性が余り感じられないように思います。
また、例えば再生時や停止時に画面に何か表示するという機能に変更が生じた際、2つのクラスを修正する必要が発生し、整合性の問題が生じます。

Comments

[nakanohitobot]

ご回答有り難うございます。
「実際に動くデータに対して使いかたを提供するのがメソッド」と考えればいいのですね。皆さんの回答で考え方がわかった気がします。
目の前の実装に必死で、修正のことまで考えるのはなかなか厳しいですが今後慣れていこうと思います。