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

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

ただいまの
回答率

87.34%

クラス内に定義したメソッドについて悩んでいます

解決済

回答 3

投稿 編集

  • 評価
  • クリップ 0
  • VIEW 773

score 74

やっていること

Areaクラスにセレクトボックスを出力するgetSelect()メソッドを定義しました。
view側ではArea::getSelect()で出力できて、シンプルなのでとてもいいなと思っていました。
ところが別のviewで以下のように「すべて」の項目があるセレクトボックスを出力する必要が出て来ました。

<select>
  <option value="">すべて</option>
  <option value="0"></option>
  <option value="1">西</option>
</select>

困っていること

1、新たに「すべて」を含んだセレクトボックスを生成するメソッドを定義する
2、getSelect()に新たに引数を与えて関数内で対応する

二つの方法を思いつきました。
どちらでも対応できそうですが、この調子でまたちょっと違うセレクトボックスを出力したくなった時に1の方法ですと同じようなメソッドがどんどん増えていくような気がしますし、2の方法ですと与える引数がどんどん増えていくことが想像出来る為どちらもあまり良い方法ではないのではないかと考えています。

セレクトボックスをまるまる生成するメソッドは諦めてoptionのみ返すようにした方がいいのでしょうか?
そもそもこのようなメソッドをクラス内に定義するのは間違いですか?
良い方法があるのでしょうか?アドバイス、参考になるページやオススメの書籍などもあれば教えて頂けませんでしょうか。よろしくお願いします。

コード

class Area
{
  protected static $title = 'エリア';
    protected static $form_name = 'area';

    protected static $areas = array(
        '0' => '東',
        '1' => '西'
    );

    public static function getData()
    {
        return self::$areas;
    }

    public static function getName($id)
    {
        $areas = self::getData();
        return $areas[$id];
    }

    public static function getSelect($id = null)
    {
        $format = '
        <div class="form-group">
          <label for="%s" class="control-label">%s</label>
          <select id="%s" class="form-control" name="%s">
            %s
          </select>
        </div>';

        $data = self::getData();

        foreach ($data as $key => $val) {
            $options .= sprintf(
                '<option value="%s" %s >%s</option>',
                $key,
                self::isSelected(strval($key), $id),
                $val
            );
        }

        $select = sprintf(
            $format,
            self::$form_name,
            self::$title,
            self::$form_name,
            self::$form_name,
            $options);

        return $select;
    }

    public function isSelected($key, $id)
    {
        if ($key === $i) {
            return 'selected';
        }
    }
}
  • 気になる質問をクリップする

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

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

    クリップを取り消します

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

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

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

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

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

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

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

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

    質問の評価を下げる

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

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

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

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

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

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

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

    詳細な説明はこちら

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

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

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

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

  • papinianus

    2018/12/17 13:27 編集

    「すべて」のHTMLでのvalueまたはphp上での配列のキーは何ですか?それはプルダウンが妥当なデータですか?以前に拝見した質問の例を借りると地方のときは複数の選び方のパターンが多くありませんか?チェックボックスが適切ではないですか?

    キャンセル

  • bws

    2018/12/17 13:31

    検索絞り込み用のセレクトボックスを想定していましてvalueは空('')にしようと思っていました。よろしくお願いします。

    キャンセル

  • bws

    2018/12/17 13:39

    盲点でした。
    そもそもセレクトボックスであれば「すべて」の項目が必要なくなりそうですね…

    キャンセル

  • m.ts10806

    2018/12/17 13:49

    そこは仕様なので決めてもらって良いかと。「何も選択されない」=「すべて」とすることもできますし、「すべて」を選択させることもできますが、それはあくまで仕様の話なので。

    キャンセル

回答 3

+3

getSelect()にtrue/falseだけを渡す引数を追加して
trueならその「全て」を含みfalseなら今までと同じような作りで良いと思います。
もし「全てだけじゃなく汎用的にしたい」のであれば、key=>valueの配列渡してその配列が定義されているかどうかで決めても良いですし。
拡張性をどの程度持たせるかどうかで決めてください。

投稿

  • 回答の評価を上げる

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

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

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

  • 回答の評価を下げる

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

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

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

  • 2018/12/17 14:09

    引数を配列で渡すという考えがなぜかありませんでした。ありがとうございます!

    キャンセル

  • 2018/12/17 14:17

    配列をうまく使えば呼び出し元のコードを触る必要がなくなるので メンテナンス性が上がります

    キャンセル

checkベストアンサー

+2

いろいろとやり方はあると思いますが、定番なのは $args 的なハッシュを渡す方式だと思います。
これはクラス設計というよりは一般的な関数/サブルーチンの設計的な考えですね。
クラス設計的にはオプション的な要素をメソッドで指定したりすることも考えられますが、個人的には無駄に複雑になるだけかな、と感じます。
よくある API のコンストラクタだと両方可能となっているものが多いですね。

投稿

  • 回答の評価を上げる

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

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

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

  • 回答の評価を下げる

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

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

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

  • 2018/12/17 14:11

    引数を配列で渡すという考えがありませんでした。$argsという言葉自体も知らなかったのでキーワードを頂けて大変助かりました。

    キャンセル

+2

回答ってほどでもないんですが、オレオレフレームワークのメンテナとしての経験から。

前回の質問でもあった質問者様の違和感は多分正しくて、つまり教科書的なオブジェクト指向からすると変なんだと思います。その理由として大きいのはこのクラスがデータモデルとビューの責務分けができてないからだと思います。

質問欄で伺ったのは、「すべて」に対応して格納すべきデータがあるか、という意味です。なので、これが盲点なんだとするとちょっと困るかと思います(これは設計に左右される問題でもないと思います)。データモデルとしては、データがあれば保存するし(つまり$areasに"全て"を持つ理由がある)、今回のように対応するデータがなければ$areasには保存しないと思います。

そして、getSelectはselect/optionを作るUI用パーツとして、このクラスの外に、ビューを管理するところで、用意される関数だと思います(業務系の画面だと、必須項目で初期値を空欄にするためにvalueが""であるプルダウンと、""のvalueをもたないプルダウンとを局面で出しわけたり、データにはあるが選択不可能なvalueがあったりして、getSelectに該当する関数はかなり色々なリクエストに応えられるように作っておく気がします。なので、このようにhtmlをクラス内で生成して返すのは設計的に危険に思えます)

投稿

  • 回答の評価を上げる

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

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

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

  • 回答の評価を下げる

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

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

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

  • 2018/12/18 11:12

    今回$areasに「すべて」を持たせる必要はなかったので、getSelect側で出し分けるのが自然ですね。
    またgetSelectを抽象化してViewを管理する側で管理できるようにがんばってみたいと思います。貴重なアドバイスありがとうございます!勉強させていただきました。

    キャンセル

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

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

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