FizzBuzz問題と仕様通りの実装

C++、COM、VBAVBScriptC#には触れていたのですが、Javaになかなか触れなかったためしばらく期間があいてしまいました。そろそろ再開しようと思います。Javaの開発TLのはずなんですが。

最近は経験の浅いチームメンバのソースをレビューすることも多いので、ここ数回は簡単な話題や原則等を取り上げていきたいと思います。

FizzBuzz問題とは、即席で作るプログラミング問題で、詳しくはFizz Buzz - Wikipediaを参照ください。
日本でも中途採用する際に2人日くらいのものを作ってもらうと分かり易いと個人的には思っています。面接では素通りしてしまうこともあるので。海外に駐在していたときは、インターンで1週間くらい来た学生が腕を見せて、スカウトされていました。

仕様として、FizzBuzz問題を使って社内プログラミングコンテストを開催してみた - give IT a tryをそのまま使わせてもらい、

  • 引数に数値を渡す。
  • 1から渡された数値までをコンソールに表示するが、
  • 3で割れるとFizz
  • 5で割れるとBuzz、
  • 3と5両方で割れるとFizzBuzzと表示する。
  • 数値と数値の間はスペースを開ける。

というものにします。

だいたい5分くらいでざっと作ると下のようになります。

  • 3と5で割れる場合は、FizzBuzz
  • それ以外で、3で割れる場合は、Fizz
  • それ以外で、5で割れる場合は、Buzz
  • それ以外は数値そのまま

という条件分岐になっていますが、上で書かれた通りの仕様になっています。

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を継承していることもわかります。

という「仕様のままの実装」で、美しいと思います。