FizzBuzz問題と仕様通りの実装
C++、COM、VBA、VBScript、C#には触れていたのですが、Javaになかなか触れなかったためしばらく期間があいてしまいました。そろそろ再開しようと思います。Javaの開発TLのはずなんですが。
最近は経験の浅いチームメンバのソースをレビューすることも多いので、ここ数回は簡単な話題や原則等を取り上げていきたいと思います。
FizzBuzz問題とは、即席で作るプログラミング問題で、詳しくはFizz Buzz - Wikipediaを参照ください。
日本でも中途採用する際に2人日くらいのものを作ってもらうと分かり易いと個人的には思っています。面接では素通りしてしまうこともあるので。海外に駐在していたときは、インターンで1週間くらい来た学生が腕を見せて、スカウトされていました。
仕様として、FizzBuzz問題を使って社内プログラミングコンテストを開催してみた - give IT a tryをそのまま使わせてもらい、
- 引数に数値を渡す。
- 1から渡された数値までをコンソールに表示するが、
- 3で割れるとFizz、
- 5で割れるとBuzz、
- 3と5両方で割れるとFizzBuzzと表示する。
- 数値と数値の間はスペースを開ける。
というものにします。
だいたい5分くらいでざっと作ると下のようになります。
という条件分岐になっていますが、上で書かれた通りの仕様になっています。
public static void main(String[] args) { if( args.length != 1 ) System.exit(1); int end = 0; try { end = Integer.parseInt(args[0]); } catch( Exception e) { System.exit(2); } StringBuffer buf = new StringBuffer(); for( int i=1 ; i<=end ; i++) { boolean isFizz = (i%3 == 0) ? true : false; boolean isBuzz = (i%5 == 0) ? true : false; if( isFizz && isBuzz ) buf.append( "FizzBuzz"); else if( isFizz ) buf.append( "Fizz"); else if( isBuzz ) buf.append( "Buzz" ); else buf.append(i); if( i != end ) buf.append(" "); } System.out.println(buf.toString()); }
- 実は(i%15==0), (i%3==0), (i%5==0)を使えばisFizz, isBuzzという変数を書かなくてもよいのですが3*5を計算しているのは分かりにくい+「両方で割れるなら」という意味で上の実装が忠実な実装となると思います。
- (i%3==0 && i%5==0), (i%3==0), (i%5==0)と書く方法もありですが、演算を2回書くことになるため、やはり一時変数に書くほうが忠実な実装となると思います。
最近レビューしたコードでは機能仕様書に例えば以下(完全再現していない仕様ですが)のように書かれていて
- 更新日時の値が空の場合で、新規作成の場合は作成日時で更新する、既存データの場合は実行日時で更新する。
- 更新日時の値が空でない場合で、新規作成の場合は作成日時で更新する。既存データの場合は何もしない。
- 作成日時が空の場合は、実行日時を代わりとし、作成日時も更新する
それに加えて状態遷移図まで提示されているにもかかわらず、if文は2つネストすればよいのですが、
「新規作成の場合で日付の値が入っていない場合は更新する、そうでない場合(既存の場合)で日付が空なら更新する
更新する場合は、作成日時が空なら実行日時を取得し、作成日時と更新日時を更新する……」
というような条件分岐の順番もifネストもなぜか3つ入ったコードが書かれていてました。動作は同じになるのですが、仕様通りに実装すればよいロジックが仕様通りになっていなかったので、また実装漏れもあったため結局リライトすることになりました。
Date createDate = getOrUpdateCreateDate(obj, executeDate);//作成日時の取得と必要なら更新 Date modifyDate = obj.getModifyDate(); boolean isNewData = (obj.getId() == null);//新規データはIdがnull if( modifiedDate == null ) { if( isNewData ) obj.setModifiedDate( createDate ); else obj.setModifiedDate( executeDate ); } else { if( isNewData ) obj.setModifiedDate( createDate ); else //do nothing }
レビュー結果として、メンバーには以下を伝えました。
コードの匂いには敏感になる必要があると感じました。
私も好きで、きっとみなさんも大好きな Hibernate の SessionFactoryImpl.java というクラスの実装で、仕様通りの実装がされている個所があるので紹介しておきます。流れるようなAPIを使用している例でもあります。
GC: SessionFactoryImpl - org.hibernate.internal.SessionFactoryImpl (.java) - GrepCode Class Source
1200 private CurrentSessionContext More ...buildCurrentSessionContext() { 1201 String impl = properties.getProperty( Environment.CURRENT_SESSION_CONTEXT_CLASS ); 1202 // for backward-compatibility 1203 if ( impl == null ) { 1204 if ( canAccessTransactionManager() ) { 1205 impl = "jta"; 1206 } 1207 else { 1208 return null; 1209 } 1210 } 1211 1212 if ( "jta".equals( impl ) ) { 1213// if ( ! transactionFactory().compatibleWithJtaSynchronization() ) { 1214// LOG.autoFlushWillNotWork(); 1215// } 1216 return new JTASessionContext( this ); 1217 } 1218 else if ( "thread".equals( impl ) ) { 1219 return new ThreadLocalSessionContext( this ); 1220 } 1221 else if ( "managed".equals( impl ) ) { 1222 return new ManagedSessionContext( this ); 1223 } 1224 else { 1225 try { 1226 Class implClass = serviceRegistry.getService( ClassLoaderService.class ).classForName( impl ); 1227 return ( CurrentSessionContext ) implClass 1228 .getConstructor( new Class[] { SessionFactoryImplementor.class } ) 1229 .newInstance( this ); 1230 } 1231 catch( Throwable t ) { 1232 LOG.unableToConstructCurrentSessionContext( impl, t ); 1233 return null; 1234 } 1235 } 1236 }
- プロパティの値が、jta, thread, managedまたは自作クラス名ならそれぞれのインスタンスを作成する
- JTASessionContextや、ThreadLocalSessionContext、ManagedSessionContextがどんなクラスかわからなくても、仕様は分かります。
- 戻り値の型だけで、上の3つのクラスがCurrentSessionContextを継承していることもわかります。
という「仕様のままの実装」で、美しいと思います。