オブジェクト倶楽部、コーディング規約の会の「C# コーディング標準」の駄目なところ
C# のコーディング規約としては、オブジェクト倶楽部のもの (PDF) が有名だけど・・・正直、これ使いたくない。
冒頭に「このドキュメントは Java コーディング標準(オブジェクト倶楽部バージョン)、VB.NET コーディング標準を C#用に変更したもの」なんて堂々と書いてる時点で・・・
で、この規約のどこが駄目なのか、なぜ駄目なのか、どうすればいいのかをまとめてみた。
なんだかんだで長文エントリ。
追記:
ちなみに、C# の規約としてはクラス ライブラリ開発者向けのデザイン ガイドラインで十分だと思う。
更に追記:
ブコメで教えてもらったんだけど、どうやらクラス ライブラリ開発のデザイン ガイドラインの方が新しいらしい。
2. ファイル構成
(1) ファイル名
public クラスはそのクラス名の 1 ファイルにする。
例:public class Customer は、Customer.cs に入れる。
パッケージ内の非パブリッククラスは、そのクラスが主に使われるパブリッククラスのファイルに含めて良い。
例外クラスは、1 ファイルに複数クラスをまとめてもよい。
public クラスをそのクラス名の 1 ファイルにするってのは、C# にそういった制限はないものの、悪くはない。
どこに何が入っているか、ってのが一目瞭然になるからね。
で、非 public なクラスを独立させないのもいい考えだと思う。というか、「含めて良い」じゃなくて、「含めるべき」くらい言ってもいいんじゃないかと思う。
なぜなら、外部から直接このクラスを使うことはないから。全部のクラスにつきファイルを作るると量が増えすぎるし、public じゃないクラスはそもそも外部から見る必要もない。
で、ここまではいいとして、例外クラスだけど・・・なんで例外クラスは複数をまとめていいの?
これは多分、中身が空っぽな例外クラスを大量生産してるんだと予想。
中身がない例外クラスを作る目的としては、クラス名で例外の具体的な状況が表したいというのが大きいと思う。
でも、そういうのはフィールドに持たせれば十分な場合も多いので、あまり例外クラスは増やすべきではない。
ということで、これはむしろ、「他の例外クラスを継承しただけの例外クラスを作らない」とするのがいいんじゃないだろうか?
(2) ファイルの位置
プロジェクトのルートディレクトリを決め、名前空間の “.” をディレクトリ階層に置き換えた位置に入れる。
C# では名前空間とディレクトリ構成は対応付けなくてもいいんじゃないだろうか。
Java でパッケージとディレクトリ構成が対応付いているのは、Java ではパッケージ (リリースの単位) と名前空間が一致しているからであって、C# ではそうじゃないんだから、ここはわざわざ Java にあわせる必要はないはず。
パッケージに気を使う必要がないのだから、C# では名前空間はもっと気軽に使っていいと思う。
や、ある程度のルールは必要だろうけどね。そこまでして Java にあわせる必要もないというか。
(3) テストクラス名、(4) テストクラスの位置
クラス ClassName のユニットテストクラス名は、ClassNameTest とする。ソリューション毎テストは SolutionNameTests とする。
例:Customer クラスなら CustomerTest.cs を作成
例:CsSample ソリューションなら、CsSampleTests.csproj とする
テストクラスの位置は、被テストクラスと同じ階層のディレクトリまたはそのサブディレクトリに配置する。
例:
被テストクラスの位置:
C:\CompanyName\OrganizationName\SolutionName\ProjectName
テストクラスの位置:
C:\CompanyName\OrganizationName\SolutionName\SolutionNameTests
C:\CompanyName\OrganizationName\SolutionName\SolutionNameTests\ProjectName
理由:物理的に近い位置でないとメンテが忘れ去られる。製品コードとの分離については、別のツール(NAnt の build ファイルなど)で調整可能。
ソリューション毎テスト?日本語でおk・・・ってのは置いとくとして、テスト用にソリューションを分ける必要が果たしてあるのかどうか。
物理的に近い位置に置くべきってのは大賛成で、それを更に推し進めたような感じ。
製品コードとの分離は別のツールでやればいいしね。
追記:
テスト用にソリューション分けるって読んでたけど、そうじゃなかった・・・
プロジェクト分けずに更に製品コードとテストコードを近づける、って方法もあるけど、まぁ、そこまでするかどうかは好みの問題かな。
いや、多分普通はプロジェクトは分けるんだろうな・・・
3. 命名規則
(5) 名前空間
企業略式名.組織略式名.テクノロジー名.機能名を使用する。また、テクノロジー名にはソリューションが、機能名にはプロジェクトが対応していること。
using CompanyName.OrganizationName.TechnologyName.FeatureName
そんなに長ったらしい名前付けて嬉しいことってある?
クラスライブラリばっかり作るわけでもないんだし、別にそんなに長ったらしい名前空間付けなくてもいいと思う。
というかですね、これ、名前空間のルートになる名前空間決めてるだけで、他のこと何も決めてないんですよね・・・
むしろそっちが重要だと思うんだけど・・・
ま、例に出ているように、Pascal 形式でいいと思うけどね。書いとけと。
(6) ファイル名
パブリックなクラス名は、ファイル名と同じでなくてはならない。(大文字小文字の区別を含めて)
ファイル構成のところにも書いたので省略。
(7) クラス名、(8) 例外クラス名
先頭大文字。あとは区切りを大文字。
簡潔すぎw
略語をどうするか、とか、どういう名前にすべきか、とか、もっと書くことあるでしょ。
(9) インターフェイス名
クラス名に同じ。また常に最初にIを付ける。
interface INameOfInterface
また、クラスにある能力を加える様な利用の場合、その能力を示す形容詞とし、-ableを接尾にする。
I ってプレフィックス付けるやり方はあまり好きじゃないんだけど、まぁ標準ライブラリに合わせる、ってのは大事 (なので C# で組む場合はしぶしぶ I を付けてます)。
問題は、「クラスにある能力を加える様な利用の場合、その能力を示す形容詞とし、-ableを接尾にする」て部分なんだけど、これはどうなんだろ。
能力って表現は違和感がありすぎる。もっとこう、特徴だとか、特性だとか、よりよい言い方はあるだろう。
そもそもインターフェイスにより能力が付け加えられるわけじゃなくて、そのインターフェイスを実装することでそのインターフェイスの持つメソッドが使えるようになるんだから、やっぱりおかしい。
(10) 実装クラス名
特にInterfaceと区別の必要があれば、最後にImplを付ける。
class ClassNameEndsWithImpl
いやいやいやいや、そもそも interface にはプレフィックス I を付けるんだから、これ不要でしょ。
それとも、Impl と付けることによりなんか素晴らしいメリットがあるの?
タイプ量が増えるとか、ノイズが増えるとか、デメリットしかない気がするんだけど。
(11) 抽象クラス名
抽象クラス名に適当な名前が無いとき、 Abstract から始まりサブクラス名を連想させる名前を付ける。
abstract class AbstractBeforeSubClassName
抽象クラスに適当な名前がないときってのはつまり、スケルトン実装ってことじゃないのかな。
その場合 Abstract ってプレフィックスは全然適切ではない。
てことで HogeBase とかどうでしょ?
HogeSkeleton って付けてた頃もあったんだけど、Skeleton って、一瞬で入ってこないんだよね・・・
(12) 定数(Const)
大文字もしくは大文字を “_” でつないだもの。
const int UPPERCASE = 0; const int UPPERCASE_WITH_UNDERSCORES = 0;
これ。このあたりが受け付けない。
何度か書いているけど、.NET Framework では定数に Pascal 記法を使っているので、合わせるべき。
実例を挙げると、int.MaxValue とか。
定数だけじゃなくて、読み取り専用フィールドにも Pascal 記法を使っているので、これも合わせるべき。
実例は、string.Empty とか。
あと関係ないけど日本語としても読みにくいよね。最初読んだとき、大文字もしくは大文字?何言ってんだ?って思った人は多いはず!
(18) コンバータメソッド(オブジェクトをべつのオブジェクトに変換するもの)
X ToX()
As もあると思うんだよね。
使い分けとしては、ディープコピーを返すものが To、中身を共有するものが As、みたいな?
よく分かりません><
(24) ループカウンタ
スコープ(通用範囲)が狭いループカウンタに i, j, k という名前をこの順に使う。
まぁ、変な名前付けられるよりよほどいいんだけど、例えば行と列のループにはそれぞれ r と c を使うとかさ、あるじゃん。
他にも x と y の方が分かりやすかったり。
そういうケースについての追記も欲しいな、とか。
(27) 無意味な名前
Info, Data, Temp, Str, Bufという名前は再考を要する。
悪い例:double temp = Math.Sqrt(b*b - 4*a*c);
良い例:double determinant = Math.Sqrt(b*b - 4*a*c);
うん、まぁ、前半はいいんだよ。
問題は、良い例が全然良くない点。むしろ悪い例より悪い。
determinant ってさ、まぁ行列式のことだと思うけど、 の D ってさ、discriminant (判別式) だよね。
無意味な名前もいけないけど、間違った名前を付けるのはもっといけないと思う。
それに、定数がどうとか関係なく「D」でいいじゃん、とか。ある程度の前提条件は置いてもいいというかなんというか。
4. ガイドライン
(34) #Region / #End Region ディレクティブ
コード領域は#Region / #End Region ディレクティブで宣言し、その領域についての説明を含める。
例: #Region "インスタンス変数" private string name; #End Region #Region "コンストラクタ" public MyClass(string name) { this.name = name; } #End Region
あまりこういう分け方はしたくないな・・・
畳みたいほどフィールド書くのはなんか違うし、コンストラクタにしたってそう。
言語仕様上の意味でグループ化するんじゃなくて、もっと抽象的な意味でグループ化すべきなんじゃないだろうか?とかそんな感じ。
あと Region / End Region は VB だから。
(35) メソッド/プロパティの宣言
メソッド/プロパティの宣言ではスコープを明示的に指定する。
これはまぁ好みの問題だけど、個人的には省略したいなぁ。
(36) 長い行
double xSquared = Math.Pow(new Random().NextDouble(), 2.0); double ySquared = Math.Pow(new Random().NextDouble(), 2.0); double length = Math.Sqrt(xSquared + ySquared);
xSquared・・・x と squared 逆じゃ?
return (this is obj) _ || (obj is Class1 _ and this.Field == obj.Field);
えっと・・・なんか所々に VB のコードが見えるw
(38) 長いメソッド宣言行
メソッドの宣言が長い場合はカンマで改行とする。
例: public void LongMethodSignature(int a, int b, int c , int d, int e, int f)
うーん、SQL じゃあるまいし、頭カンマ?
しかも全部の引数を分割するわけでもなく、頭カンマ?
これはない。
(39) abstract Class vs. interface
抽象クラス(abstract Class)はなるべく使わず、interfaceを多用せよ。abstract Classは、一部実装があり、一部抽象メソッドであるような場合にのみ使う。
理由:interfaceは幾つでも継承できるが、Classは1つだけ。1つから継承してしまうと、もう継承できずもったいない。
理由が MOTTAINAI って、ふざけてんのかと。
inerface と abstract class はそもそも目的が異なるんだから、vs どうのこうのって話ではない。
あと、interface は継承じゃなくて、実装ね。複数の interface を継承した interface も作れるけど、ここで言ってるのは多分それじゃないし。
(40) public Variable
インスタンス変数は、極力publicにせず、妥当なプロパティを設ける。
理由:オブジェクト指向の標準。クラスの内部状態に勝手にアクセスさせるのはよくない。ただし、以下の条件をすべて満たす場合、インスタンス変数をpublicにし、直接アクセスさせてもよい。
- そのインスタンス変数が他のインスタンス変数と独立であり、単独で変更されても内部の整合性をくずさない。
- どちらにしても,get / setアクセサを書く。
- インスタンス変数の実装が将来に渡って変更されないことが根拠付けられる。
また、上記に当てはまらない場合でも、極度に速度を気にする場合は、この限りではない。(ただし、慎重にコメントすること)
そのインスタンス変数が他のインスタンス変数と独立であり、単独で変更されても内部の整合性をくずさないなら、それはそのクラスに置いとく必要ないんじゃ?
それに、get / set アクセサを書くって、この規約では public フィールドもアクセサも Pascal 記法なんですけど・・・
C# ではフィールドもプロパティもメソッドも、全部同じ名前空間なのでこの 40 を守りながら (19) プロパティ名 先頭大文字。あとは区切りを大文字
と (29) Publicスコープのインスタンス変数 先頭大文字。あとは区切りを大文字
を守ろうとすると、public フィールドとプロパティのどちらかの名前を変えなくてはいけない。
追記:
誤読してました。詳しくはコメント参照。
(43) private vs. protected
privateよりは、protectedを使用する。
理由:privateは確実にそのクラス外からの使用をシャットアウトできるが、クライアントが、より細かいチューニングをSubClass化によって行うことを出来なくしてしまう。
別法: private をより好んで使え。protected にしてしまうと以降、変更が起ったときにそれを継承している全クラスに影響が出てしまう。
C# では private がデフォルトの修飾子であることをもっとよく考えるべきだと思う。
それに、protected はそもそも目的がはっきりとしているんだから、比べるとしたら private と internal だろう。
サブクラスでスーパークラスのフィールドいじるのはあまりいい設計ではないサインであることが多いので、理由は理由になってないし。
(44) 変数隠し
基本クラスの変数名と、同じ変数名を使う事は避けよ。
理由:一般的にはこれはバグである。もし意図があるならコメントせよ。
避けることには同調するんだけど、意図がある場合の対処が・・・
C# には new という機能があるのだから、それを使うべき。
public Hoge { public int a; } public ExHoge : Hoge { public new int a; }
いや、その上で更にコメントするのはアリだけどさ。
(46) public メソッド
クラスのpublicメソッドは、「自動販売機のインターフェイス」を目標に。分かりやすく、使いかたを間違っても内部の整合性はこわれないように設計する。また、可能ならば契約による設計(Design by Contract)を行い、クラスの不変条件と共にメソッドの事前・事後条件をコードで表現せよ。
自動販売機のインターフェイスって何?
何を目標にすればいいの?
(48) this の return
クライアントの便宜を考えたつもりでも、thisをreturnすることはなるべく避ける。
理由:A.Meth1().Meth2().Meth3() というような連鎖は、一般的にSynchronization上の問題の元になる。
んー?なんでこれが synchronization 上の問題の元になるのかよく分からない。
まぁ、安直に this を返すな、ってのは賛成だけど。
(49) メソッドの多重定義
引数のタイプによるメソッドのオーバーロードはなるべく避ける(引数の数が違うものはOKである)。特に、継承と絡むと厄介である。
例: ×:override void Draw(Rectangle rectangle) override void Draw(Point point) ○:void DrawRectangle(Rectangle rectangle) void DrawPoint(Point point)
えっと・・・継承と絡む、の意味を取り違えている気がする。
多分、言いたいのはこういうこと。
class Hoge {} class ExHoge : Hoge {} class Program { static void Piyo(Hoge h) { ... } static void Piyo(ExHoge h) { ... } public static void Main() { Hoge h = new Hoge(); ExHoge e = new ExHoge(); Hoge h2 = new ExHoge(); Piyo(h); // Piyo(Hoge) Piyo(e); // Piyo(ExHoge) Piyo(h2); // Piyo(Hoge) } }
(51) Clone()
もし、Clone() メソッドが使われるようなら、 ICloneable を実装し明示的にそれを書く。
例: using System; public class Foo : ICloneable { public object ICloneable.Clone() { Foo myFoo = (Foo) this.Clone(); // Foo クラスの属性のクローン //... } }
「明示的にそれを書く」ってのは、明示的に実装しろ、ってことを言ってるんだと思う。
が、例では明示的に実装しようとして余計な public が付いている。
失礼ながら、これ書いた人はまともに C# を理解してないんじゃないか?と思った。
(52) デフォルトコンストラクタ
可能ならいつでもデフォルトのコンストラクタ(引数がないもの)を用意せよ。
理由:リフレクションを使用して、Assembly.CreateInstance(TypeName)で型名文字列からダイナミックにそのクラスを作成可能。
デフォルトコンストラクタって、勝手に用意されるコンストラクタのことなのか、引数を持たないコンストラクタのことなのか曖昧だよね。
ってのは置いといて、理由がひどくないか?
リフレクションを使用して・・・って、そんな使うか使わないかわかんないもののために常に引数なしのコンストラクタを用意しろって・・・
イミュータブルなクラスとかどうすんだ。わざわざデフォルトの初期値を考えろと言うことか。
(53) abstract Method in abstract classes
abstractクラスでは、no-opのメソッドを書くより、明示的にabstractメソッドと宣言せよ。また、共有可能なデフォルトの実装を用意できるなら、それをprotected とし、サブクラスが1行で処理を書けるようにせよ。
理由:.NETのIDEは、実装されていないabstractメソッドを検出できるため、単に実装を忘れていただけ、というバグを回避できる。
これはケースバーケースでしょう。
no-op がデフォルトの動作の場合、カスタマイズするためにサブクラス作るんだから、実装忘れもなにもないだろうし。
(54) オブジェクトの同値比較
オブジェクトの比較では Equals() メソッドを使い、= を使うな。特に、Stringの比較では = を使用してはならない。
理由:もし実装者がEquals()を用意しているなら、それを使ってほしくて実装したはず。また、Stringの比較ではOption Compare Textに設定されていると大文字/小文字が区別されない。
理由:ユニットテストでは、AssertEquals が Equals() を利用しているため、簡単に同値テストが書ける。
ちょっと待て。これはどこの VB or Java の話ですか。
= じゃなくて == だろう、ってのは typo だからいいとして、C# で文字列の比較に == を使うなだ?
何を言っているんだ・・・
もし実装者が operator == を用意しているなら、それを使って欲しくて実装したはず。それに C# には Option Compare Text なんてオプションは存在しない。
ユニットテストでは、AssertSame が以下略。
(55) 宣言と初期化
ローカル変数は、初期値と共に宣言せよ。
理由:変数の値に関する仮定を最小化する。
悪い例: void f(int start) { int i, j; // 初期値なしの宣言 // 多くのコード // ... i = start + 1; j = i + 1; // i, jを使う // ... } 良い例: void f(int start) { // 多くのコード // ... int i = start + 1; int j = i + 1; // i, jを使う // ... }
まずは、多くのコードをどうにかしてください。話はそれからです。
というか、数行で終るようなメソッドの場合、ローカル変数が宣言とともに初期化されてなくても平気なんですよね−。
いや、でも宣言とともに初期化するけど。
(56) ローカル変数の再利用は悪
ローカル変数を使い回しするより、新しいものを宣言して初期化せよ。
理由:変数の値に関する仮定を最小化する。
理由:コンパイラの最適化を助ける。悪い例: void f(int n, int delta) { int i; // 初期値なしの宣言 for (i = 0; i < n; i++) { // iを使う } for (i = 0; i < n; i++) // またiを使う { if (...) { break; } } if (i != n - 1) // 最後まで回ったかの判定にiを使っている { // ... } i = n - delta * 2; // またまた再利用 //... } 良い例: void f(int n, int delta) { for (int i = 0; i < n; i++) { // iを使う } bool found = false; for (int j = 0; i < n; i++) { // jを使う if (...) { found = true; break; } } if (found) { // ... } int total = n - delta * 2; // 別の意味ある変数 //... }
えっとですね、だからまずはメソッドを分割してください。
そうすれば自然とローカル変数の使い回しとかなくなりますから。
(57) 大小比較演算子
“<”、 “<=” を好んで使い、”>”、 “>=” はなるべく避けよ。
理由:大小の方向を統一し、右側を大きい方にすることで、混乱を避ける。
混乱て・・・
for ループでは < を好んで使うけど、その他の条件式では自然なものを使えばいいでしょ。
・・・どうなんだろ?一方向に統一してあった方が読みやすいんだろうか?
(58) if/while 条件中の "="
if, while の条件には、代入 "=" を使ってはならない。
理由: ほとんどの場合、バグである。
C/C++ じゃないんだから、ほとんどバグは言い過ぎでしょ。
むしろバグの方が少ないはず。だって
bool isHoge = ...; bool isPiyo = ...; if (b = isPiyo) { ... }
くらいしかバグになるような状況が思いつかないし。
まぁ、確かに禁じたい気持ちも分かる (読みにくいという人もいる) んだけど、これはさすがに無理がある。
読みにくいなら素直にそう言えばいいのに。
(59) キャスト
キャストは、できる限り is classの条件文で囲め。
例: C cx = null; if (x is C) { cx = (C) x; } else { evasiveAction(); }理由:これで、「オブジェクトがそのインスタンスじゃなかったら?」とういことを常に考える癖がつく。ただし、キャスト出来ない場合がバグである、と判断できる場合は、この限りではない。
この場合は素直に as 使う状況でしょ。
C cx = x as C; if (cx == null) { evasiveAction(); }
ほらすっきり。
(60) 例外クラス
例外クラスは大域的な性格をもち、多用するとプログラムの流れを読みにくくしてしまうことを認識する。
例外クラスは、新たに作成するよりも、.NETクラスライブラリに含まれているものを利用できれば利用する。
例: IOException, FileNotFoundException, ArgumentException などは利用しやすい標準例外。
新たな例外の作成は、そのパッケージ全体のインターフェイスとして検討すること。
例外クラス自体が大域的な性格を持つわけではないと思うけど・・・多分、例外機構のことを言いたかったと予想。
それ以降は同意。
まぁでも、言語標準の例外って Java にしても .NET にしても、センスないよね・・・
(61) メソッド引数の変更は悪
原則としてメソッドの引数は入力であり、出力としては使わないこと。すなわちメソッド内部で引数の状態を変更するメソッドを呼ばないこと。出力引数に新たなオブジェクトを代入しないこと。
悪い例: void MoveX(Point p, int dx) { p.X = p.X() + dx; // 引数を変更している(なるべく避ける) } void MoveX(Point p, int dx) { Point p = New Point(p.X + dx, p.Y); // これは呼び出し側に伝わらない } 例外:パフォーマンスを気にする場合。
えっと・・・Point って System.Drawing.Point だよな・・・
これは struct なんで引数変更しても呼出し元には影響ないですよ?
それに、なんかここでもいい感じに VB が・・・
(64) switch, if/elseの繰り返しは悪
switch文で分岐する処理が現れた時には、よくない設計の兆候だと考え、ポリモーフィズムで実現できないか再考する。特に同じようなswitchが2箇所以上現れたら、必ずポリモーフィズム、FactoryMethod,Prototypeパターン等でリファクタリングすること。if / elseの連続も同様。さらに、nullチェックを行う同様のIfが多くの場所に現れたら、NullObjectパターンの利用を検討せよ。
え・・・と、なんでここで Prototype パターンが出てくるんだろう。
なんのパターンと間違えたのかちょっと気になる。
5. コメント
(65) ドキュメントコメントの活用
public クラス、メソッド、プロパティには、必ず /// コメントをつける。
interface からのメソッドには、この種のコメントは不要。
プロパティについても、名前がしっかりしていればほとんどの場合でコメントは不要。
なんか中で特別なことをしてる場合に限ってコメントを付ければいいと思う。
7. その他
(74) トリッキーなコードは悪
.NETの平均プログラマに分かるようなコードを書く。演算子の順序、初期化に関する規則など、誰もが必ずしも自信をもって答えられないような仮定を持ち込まず、() を使って演算順序を明確にしたり、明示的な初期化をしたりする方が読みやすい。
悪い例: return cond == 0 ? a < b && b < c : d == 1; 良い例: return (cond == 0) ? ((a < b) && (b < c)) : (d == 1);
.NET の平均的プログラマは、C のプリプロセッサ並みに冗長な括弧が必要なのか?
正直、これでいいだろうと思う。
return cond == 0 ? a < b && b < c : d == 1;
Python みたいに
return cond == 0 ? a < b < c : d == 1;
こんな感じに書けたらもっといいんだけどね。
(75) 100%正しいことはない
ここに書かれていることに、100% 準拠する必要はない。迷ったら考えを整理し、相談すること。十分な理由があってルールから外れることはよくある。コミュニケーションができるチームの助けとなることが、このコーディング標準の目的である。
まぁそうなんですけど・・・
この規約をベースにしているチームをいくつか見たことがあるんですが、どこも杓子定規でどうにもこうにも。
コミュニケーションができるチームの助けどころか、まずいコードを許容するための盾になってるのが現状です。
正直、公開を中止して「間違いでしたごめん」してほしい。