LoginSignup
0
0

More than 1 year has passed since last update.

@s17161278(コウキ)

もしAndroidオープンソースがクソコードだったら

Posted at

まえおき

  • 下品なタイトルで申し訳ありません。
  • あえてクソコード化させてクラス設計を考察する。という内容です。
  • というのは、直近でCodeCompleteを読んでいてクラスをどう設計するべきかについて学びました。
  • 抽象的な表現も多く記憶から消える前に実際のソースコードを通してクラス設計のアンチパターンをまとめてみようと思います。
  • しかし設計を１からするのは難しいため、逆の発想でAndroidのオープンソースコードであればJavaで美しく設計されていると信頼して、そのうちのSimpleAdapterクラスを部分的に変更してどんなリスクが生じるのかを考察します。Fragmentクラスなどはメンバが多すぎたため少しマイナーかもしれませんがアダプタを説明の都合上採用します。
  • CodeCompleteを読んだ人やクラス設計を学習している人、JavaでAndroid開発をしている人に楽しんでもらえたらうれしいです。

クラス定義のねらい

  • CodeCompleteを読んでいるといろんなことが書いてあるわけですが、そもそものクラス設計の本質は『複雑さの緩和』にあるように感じました。
  • コードを再利用、実装の詳細部分の隠蔽も得られますが、設計段階の意識としては『人間が理解するのに時間がかからない』『脳のリソースを無駄につかわなくて読みやすい』かどうかを念頭に置くべきだと思います。

SimpleAdapterクラス

public class SimpleAdapter extends BaseAdapter implements Filterable, ThemedSpinnerAdapter {
    // 10個の変数
    private final LayoutInflater mInflater;
    private int[] mTo;
    private String[] mFrom;
    private ViewBinder mViewBinder;
    @UnsupportedAppUsage(maxTargetSdk = Build.VERSION_CODES.P, trackingBug = 115609023)
    private List<? extends Map<String, ?>> mData;
    private int mResource;
    private int mDropDownResource;
    private LayoutInflater mDropDownInflater;
    private SimpleFilter mFilter;
    private ArrayList<Map<String, ?>> mUnfilteredData;
    
    // コンストラクタ
    public SimpleAdapter(Context context, List<? extends Map<String, ?>> data,
            @LayoutRes int resource, String[] from, @IdRes int[] to) {}
    
    // 16個のメソッド
    public int getCount() {}
    public Object getItem(int position) {}
    public long getItemId(int position) {}
    public View getView(int position, View convertView, ViewGroup parent) {}
    private View createViewFromResource(LayoutInflater inflater, int position, View convertView,
            ViewGroup parent, int resource) {}
    public void setDropDownViewResource(int resource) {}

    @Override
    public void setDropDownViewTheme(Resources.Theme theme) {}
    	
    @Override
    public Resources.Theme getDropDownViewTheme() {}

    @Override
    public View getDropDownView(int position, View convertView, ViewGroup parent) {}
    private void bindView(int position, View view) {}
    
    // 使わないので割愛
	.
    .
    .
	
    // インターフェース、値をビューにバインドするために使われる
    public static interface ViewBinder {
        ...
    }
	
    // インナークラス
    private class SimpleFilter extends Filter {
        ...
    }
}
  • SimpleAdapterクラスはXMLファイルで定義されたビューにArrayList(動的配列)などのデータをマッピングするクラスです。
  • BaseAdapterクラスを継承しており、Filterable, ThemedSpinnerAdapterを実装しています。
  • 29個のメンバがあり、そのうちの10個が変数で1つはコンストラクタ、16個のメソッド、１つのインナークラスとインターフェースが定義されています。

ADTを壊してみる

  • ADTとは、Abstract data typeの略で日本語では抽象データ型となります。

    抽象データ型（ちゅうしょうデータがた、英: abstract data type、ADT）とは、データ構造とそれを直接操作する手続きをまとめてデータ型の定義とすることでデータ抽象[1]を実現する手法またはそのひとまとまりとして定義されたデータ型を言う。通常のデータ型であれば変数宣言で変数に束縛されるものは値であるが、抽象データ型の世界において値に相当するものはデータ構造とその操作[2]のまとまり[3]である。

    wikipediaより

    • 言い換えると『データとその操作をひとまとまりとして扱うこと』です。実際のプログラミング言語ではクラスを定義することのうち、フィールドとメソッドをどう作るかというテーマです。

  • ADTを作ると、単純なデータ構造(キューとかMapとか)ではなく、待ち行列やスーパーの飲み物陳列をする店員だとか、より上位の抽象化を作り、下位の詳細を意識しなくてもよくなります。『〇〇から〇〇をとりだす』『〇〇を置く』のような命令から、『陳列する』のような一言で動き出すわけです。

  • 早速ADTを壊していきたいところですが、そもそもADTの破壊とはクラスを使わない状態にのことで、Javaで表現することは困難だと考えます。あえて書くのであれば、Activityクラス(ユーザ操作とのインターフェース)のなかでXMLを読んでデータマッピングする仕組みを独自で実装するような状態です。

public void onCreate(Bundle savedInstanceState) {
    super.onCreate(savedInstanceState);
    setContentView(R.layout.main_activity);
	
    // ビューを取得して
    ListView listView = findViewById(R.id.list_view);
    // データを用意して
    ArrayList<Map<String, String>> data = new ArrayList(){};
    // ListViewのrowデータの表現方法の定義、データをイテレートしてマッピングする仕組み　などなど
    ...
}
  • 言うまでもありませんが、Activity.onCreateメソッドの関心ごとが多くなりすぎます。
  • 『リストにデータをマッピングする』という命令ではなく『Listを取得して、データを用意して...』と逐次的に命令を並べることになり、共通の処理がでてきたときに同じコードをたくさん書く羽目になりそうです。
  • 可読性という観点でも、眠くなること必至です。

カプセル化を壊してみる

  • カプセル化はADTを作ったうえで、その操作をするためのインターフェースを作ります。具体的には、クラス内の変数にセッター、ゲッターを定義したりすることに当たります。
  • 『クラスの使い方を統一すること』と言い換えることができそうです。
  • カプセル化を壊すことは簡単です。
public class SimpleAdapter extends BaseAdapter implements Filterable, ThemedSpinnerAdapter {
    // フィールドをpublicで修飾した
    public final LayoutInflater mInflater;
    public int[] mTo;
    public String[] mFrom;
    public ViewBinder mViewBinder;
    @UnsupportedAppUsage(maxTargetSdk = Build.VERSION_CODES.P, trackingBug = 115609023)
    public List<? extends Map<String, ?>> mData;
    public int mResource;
    public int mDropDownResource;
    public LayoutInflater mDropDownInflater;
    public SimpleFilter mFilter;
    public ArrayList<Map<String, ?>> mUnfilteredData;
    
  	...
  • フィールドをpublicにするとカプセル化が壊れるリスクにさらされます。ゲッターやセッターを削除するとより強調されるでしょう。
  • 上のようにするとクライアントコードが自由にフィールドにアクセスすることを許可するため、例えば、mDataの値を代入する際に以下のようなソースコードがあちこちに紛れてしまう恐れがあります。
SimpleAdapter adapter = new SimpleAdapter(...);
adapter.mData = new HashMap(...);
  • こうなると、フィールドの名前や型の変更などのルールが加わったときに変更箇所を推測するのが難しくなります。

カプセル化の意味的な違反をしてみる

  • カプセル化の意味的な違反とは、クライアントコードがクラスのパブリックインターフェースではなく、プライベートなインターフェースに依存している状態です。
  • privateでフィールドを定義している場合(構文的にはカプセル化)でも注意しなくてはいけないところです。
  • 例えば、SimpleAdapterの実際の実装とはかけ離れることにはなりますが、コンストラクタがなくてパブリックなinitializeメソッドで各種フィールドを設定しているようなケースで、保持しているデータの要素を取得したいとする。
  • このときgetItemメソッドがデータ取得のインターフェースです。
public class SimpleAdapter extends BaseAdapter implements Filterable, ThemedSpinnerAdapter {
    ...
    
    public void initialize(Context context, List<? extends Map<String, ?>> data,
            @LayoutRes int resource, String[] from, @IdRes int[] to) {
    	...    
    }
    public long getItem(int position) {
        this.itinialize(...);
    }
  • 『クライアントが、その内部でinitializeメソッドを呼び出していることを知っているのでgetItemを単体で呼び出してしまうようなケース』が、カプセル化の意味的な違反にあたります。クライアントクラスがSimpleAdapterの詳細を知りすぎているため結合度が高くなっていまい、デバッグが難しくなるリスクを抱えます。

クラスの抽象化のレベルを壊してみる

  • クラスは通常なにかしらの、現実世界のオブジェクトや概念を抽象化させたもののはずです。
  • SimpleAdapterクラスはなにを抽象化しているのかを推測するため、継承元のBaseAdapterについて見ておきます。BaseAdapterを継承しているクラスは他にListAdapter,SpinnerAdapterがあります。それぞれ、ListView, SpinnerViewといったリソースファイル(XMLファイル)で表現された画面の部品に対してデータをセットする役割を持っています。
  • これらのクラスに共通していることは、リソースファイルにデータをセットするという部分です。この抽象化を行っているのがBaseAdapter抽象クラスです。SimpleAdapterクラスは、データの型がList< Map>の場合のためのクラスです。
  • この抽象化を壊す方法はいくつか挙げられます。
public class SimpleAdapter extends BaseAdapter implements Filterable, ThemedSpinnerAdapter {
	...

    public int getCount() {}
    public Object getItem(int position) {}
    public long getItemId(int position) {}
    public View getView(int position, View convertView, ViewGroup parent) {}
    
    // 関連のない(凝集度が弱い)インターフェースを追加する
    + public void changeResourceLayout() {}
    // リストデータを直接addする(他のインターフェースと抽象度が違う)
   	+ public void addItemToData(Map<String, String>) {}
    // privateにすべきメソッドがpublicになっている
    + private View createViewFromResource(LayoutInflater inflater, int position, View convertView,
            ViewGroup parent, int resource) {}
}
  • これらは短期的にみると問題がないのかもしれませんが、機能変更をしようとしたときのコストが高くなることは容易に想像できます。

リスコフ置換原則を破る(is-aとhas-aを間違えてみる)

  • クラス内のメンバをどのように獲得するのかという観点で見ていくと、has-a(包含)、is-a(継承する、特化)の二択の方法があります。

  • 継承元のBaseAdapterはいくつかパブリックなメソッドを提供しています。したがって、いくつかのメソッドのインターフェースは継承することによって獲得しています。問題は、このインターフェース規約にSimpleAdapterクラスが完全に従っているかどうかです。少しでも、インターフェース(シグネチャ、戻り値)が異なっている場合継承は使はない、もしくは、そのインターフェースは公開するべきではありません。

  • リスコフ置換原則は、基底クラスのメソッドのすべてが派生クラスで何の違和感もなく使用できるようにしなくてはいけないというルールです。

  • 抽象化を壊すことにもつながりますが、BaseAdapterを直接継承していて、誤った抽象度や関連度のクラスが用意されていると、クライアント開発者としては最悪です。

// SimpleAdapterManager ?
public class SimpleAdapterManager extends BaseAdapter implements Filterable, ThemedSpinnerAdapter {
    View[] views;
	...
    @Override
    public View getDropDownView(int position, View convertView, ViewGroup parent){
        ...
    }
    public View[] InitDropDownViews() {
        getDropDownView(...);
        getDropDownView(...);
        getDropDownView(...);
    } 
}

継承を『インターフェース』ではなく『実装』の獲得につかってしまう

public class SimpleAdapter extends BaseAdapter implements Filterable, ThemedSpinnerAdapter {
	...
    @Override　// ?
    public boolean areAllItemsEnabled(boolean flag){
        super.areAllItemsEnabled();
    }
}
  • SimpleAdapter上でBaseAdapterと同じ処理をしたいが、加えてなにか引数を新たに追加して実装しなければいけないというニーズがあったとしたら継承ではなく包含を使います。幸いJavaではコンパイルエラーとなります。
  • また、継承関連でクラスは親子間であってもカプセル化を保つために、基底クラスのフィールドはprivateで宣言して必要な時だけprotectedにするべきです。『カプセル化を壊してみる』と同じ現象がSimpleAdapter内で発生することになるからです。

デメテルの法則を破る

  • デメテルの法則とは、最小知識の原則とも言い換えられますが、平たく言うと『メソッドチェーンが続いたら気を付けよう』という原則です。
  • メソッドチェーンが続いている状態とは、メソッドから呼ばれる戻り値のメソッドについて知っているクラスをつくっている状態ということで、結合度が高くなってテストや改修が難しくなるらしいです。
  • オブジェクト指向では友達の友達はみな友達だ、なんていう笑っていいとものロジックは通用しません。
  • こちらはクラス設計というよりもステートメントレベルの議論なのかと思いきや、クラス同士の関係値で作られます。参考
  • 例えば、以下のようにリソースファイルにマッピングするデータのmDataフィールドがpublicなとき(純粋なカプセル化の違反にも該当しますが)
public class SimpleAdapter extends BaseAdapter implements Filterable, ThemedSpinnerAdapter {
    ...
    public List<? extends Map<String, ?>> mData;
    
    public SimpleAdapter(Context context, List<? extends Map<String, ?>> data,
            @LayoutRes int resource, String[] from, @IdRes int[] to) {}
   ...
}
  • クライアントコードで以下のように書いてしまうとデメテルの原則違反です。
public void onCreate(Bundle savedInstanceState) {
    super.onCreate(savedInstanceState);
    setContentView(R.layout.main_activity);
	
    ...
    SimpleAdapter adapter = new SimpleAdapter(Activity.this, data, from, to);
    // SimpleAdapterの内部実装を知りすぎている。
    adapter.mData.get(0).id;
    ...
}
  • メソッドチェーンをなくせばよいのか、単純にgetter,setterをつくればよいのかというとそういうわけでもない
public class SimpleAdapter extends BaseAdapter implements Filterable, ThemedSpinnerAdapter {
    ...
    private List<? extends Map<String, ?>> mData;
    
    public SimpleAdapter(Context context, List<? extends Map<String, ?>> data,
            @LayoutRes int resource, String[] from, @IdRes int[] to) {}
    // SimpeAdapterクラスが実現している抽象化を崩している。
    // さきほどよりも良いが、実装の内容のはなしではなく
    // getMDataItemIdがSimpleAdapterのインターフェースとして適切かということです
    // mDataという実装レベルの話がインターフェースになっているのが問題です。クラスの抽象化を壊しています。
   	publicMgetMDataItemId(int position){
        return mData.get(position).id();
    } 
}

その他(良い書き方)

  • クラスの名前は名詞にする。

  • 可能ならコンストラクタですべてのメンバを初期化する。

  • 基底クラスの変数をprotectedではなくprivateにする。

感想

  • クラス設計に対して面白いアプローチができたと思っているがが、もっと芸術的なクソコードが作れるように理解度を挙げていきたい。(動機がおかしい)
0
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
0
0