質問をすることでしか得られない、回答やアドバイスがある。

15分調べてもわからないことは、質問しよう!

ただいまの
回答率

90.36%

  • Java

    16754questions

    Javaは、1995年にサン・マイクロシステムズが開発したプログラミング言語です。表記法はC言語に似ていますが、既存のプログラミング言語の短所を踏まえていちから設計されており、最初からオブジェクト指向性を備えてデザインされています。セキュリティ面が強力であることや、ネットワーク環境での利用に向いていることが特徴です。Javaで作られたソフトウェアは基本的にいかなるプラットフォームでも作動します。

  • オブジェクト指向

    340questions

    オブジェクト指向プログラミング(Object-oriented programming;OOP)は「オブジェクト」を使用するプログラミングの概念です。オブジェクト指向プログラムは、カプセル化(情報隠蔽)とポリモーフィズム(多態性)で構成されています。

  • リファクタリング

    17questions

    リファクタリングとはコードの本体を再構築するための手法であり、外見を変更せずに内部構造を変更/改善させることを指します。

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

解決済

回答 5

投稿 編集

  • 評価
  • クリップ 4
  • VIEW 2,539

nakanohitobot

score 31

現在「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クラスに動作を表すメソッドが存在してもよい理由はなにか?

よろしくお願いします。

  • 気になる質問をクリップする

    クリップした質問は、後からいつでもマイページで確認できます。

    またクリップした質問に回答があった際、通知やメールを受け取ることができます。

    クリップを取り消します

  • 良い質問の評価を上げる

    以下のような質問は評価を上げましょう

    • 質問内容が明確
    • 自分も答えを知りたい
    • 質問者以外のユーザにも役立つ

    評価が高い質問は、TOPページの「注目」タブのフィードに表示されやすくなります。

    質問の評価を上げたことを取り消します

  • 評価を下げられる数の上限に達しました

    評価を下げることができません

    • 1日5回まで評価を下げられます
    • 1日に1ユーザに対して2回まで評価を下げられます

    質問の評価を下げる

    teratailでは下記のような質問を「具体的に困っていることがない質問」、「サイトポリシーに違反する質問」と定義し、推奨していません。

    • プログラミングに関係のない質問
    • やってほしいことだけを記載した丸投げの質問
    • 問題・課題が含まれていない質問
    • 意図的に内容が抹消された質問
    • 広告と受け取られるような投稿

    評価が下がると、TOPページの「アクティブ」「注目」タブのフィードに表示されにくくなります。

    質問の評価を下げたことを取り消します

    この機能は開放されていません

    評価を下げる条件を満たしてません

    評価を下げる理由を選択してください

    詳細な説明はこちら

    上記に当てはまらず、質問内容が明確になっていない質問には「情報の追加・修正依頼」機能からコメントをしてください。

    質問の評価を下げる機能の利用条件

    この機能を利用するためには、以下の事項を行う必要があります。

質問への追記・修正、ベストアンサー選択の依頼

  • eripong

    2015/12/30 11:53

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

    キャンセル

  • nakanohitobot

    2015/12/31 00:11

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

    キャンセル

回答 5

checkベストアンサー

+5

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

投稿

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

  • 2015/12/31 16:34

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

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

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

    キャンセル

  • 2015/12/31 17:43

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

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

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

    キャンセル

  • 2015/12/31 19:07

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

    キャンセル

+2

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

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

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

投稿

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

  • 2015/12/31 17:00

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

    キャンセル

  • 2015/12/31 17:31

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

    キャンセル

  • 2015/12/31 19:09

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

    キャンセル

+2

こんにちは。

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

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

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


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

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

投稿

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

  • 2015/12/31 16:41

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

    キャンセル

+1

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

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

投稿

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

  • 2015/12/31 17:05

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

    キャンセル

+1

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

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

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

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

投稿

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

  • 2015/12/30 02:49

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

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

    キャンセル

  • 2015/12/30 12:45

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

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

    キャンセル

  • 2015/12/31 16:57

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

    キャンセル

15分調べてもわからないことは、teratailで質問しよう!

  • ただいまの回答率 90.36%
  • 質問をまとめることで、思考を整理して素早く解決
  • テンプレート機能で、簡単に質問をまとめられる

同じタグがついた質問を見る

  • Java

    16754questions

    Javaは、1995年にサン・マイクロシステムズが開発したプログラミング言語です。表記法はC言語に似ていますが、既存のプログラミング言語の短所を踏まえていちから設計されており、最初からオブジェクト指向性を備えてデザインされています。セキュリティ面が強力であることや、ネットワーク環境での利用に向いていることが特徴です。Javaで作られたソフトウェアは基本的にいかなるプラットフォームでも作動します。

  • オブジェクト指向

    340questions

    オブジェクト指向プログラミング(Object-oriented programming;OOP)は「オブジェクト」を使用するプログラミングの概念です。オブジェクト指向プログラムは、カプセル化(情報隠蔽)とポリモーフィズム(多態性)で構成されています。

  • リファクタリング

    17questions

    リファクタリングとはコードの本体を再構築するための手法であり、外見を変更せずに内部構造を変更/改善させることを指します。