わたしのコーディングの師匠かずきさんに、私のクソコードをレビューしてといったらオシャレなコードになって帰ってきたので、その学びを書き留めておきたい。
今回のゴール
私は Durable Functions というリポジトリに貢献しているのだが、前回実装した全件取得のメソッドに対して、検索条件を追加するメソッドを書きたいと思った。検索条件を表すオブジェクトをつくれば多分テストもしやすくていい感じのはず。
今回のクソコードの解説
最初に作ったクソコードはこんな感じ Builder 的な感じで作ってみた。TDDを使ってやってるのでテストもある。
リポジトリ
OrchestrationInstanceStatusQueryBuilder
using Microsoft.WindowsAzure.Storage.Table;
using System;
using System.Collections.Generic;
using System.Text;
namespace TableQuerySpike
{
public class OrchestrationInstanceStatusQueryBuilder
{
private string RuntimeStatus { get; set; }
private DateTime CreatedTimeFrom { get; set; }
private DateTime CreatedTimeTo { get; set; }
private DateTime LastUpdatedTimeFrom { get; set; }
private DateTime LastUpdatedTimeTo { get; set; }
public OrchestrationInstanceStatusQueryBuilder AddRuntimeStatus(string runtimeStatus)
{
this.RuntimeStatus = runtimeStatus;
return this;
}
public OrchestrationInstanceStatusQueryBuilder AddCreatedTime(DateTime createdTimeFrom, DateTime createdTimeTo)
{
this.CreatedTimeFrom = createdTimeFrom;
this.CreatedTimeTo = createdTimeTo;
return this;
}
public OrchestrationInstanceStatusQueryBuilder AddLastUpdatedTime(DateTime lastUpdatedTimeFrom, DateTime lastUpdatedTimeTo)
{
this.LastUpdatedTimeFrom = lastUpdatedTimeFrom;
this.LastUpdatedTimeTo = lastUpdatedTimeTo;
return this;
}
public TableQuery<OrchestrationInstanceStatus> Build()
{
var query = new TableQuery<OrchestrationInstanceStatus>()
.Where(
GetConditions()
);
return query;
}
private string GetConditions()
{
var conditions = new List<string>();
if (default(DateTime) != this.CreatedTimeFrom)
{
conditions.Add(TableQuery.GenerateFilterConditionForDate("CreatedTime", QueryComparisons.GreaterThanOrEqual, new DateTimeOffset(this.CreatedTimeFrom)));
}
if (default(DateTime) != this.CreatedTimeTo)
{
conditions.Add(TableQuery.GenerateFilterConditionForDate("CreatedTime", QueryComparisons.LessThanOrEqual, new DateTimeOffset(this.CreatedTimeTo)));
}
if (default(DateTime) != this.LastUpdatedTimeFrom)
{
conditions.Add(TableQuery.GenerateFilterConditionForDate("LastUpdatedTime", QueryComparisons.GreaterThanOrEqual, new DateTimeOffset(this.LastUpdatedTimeFrom)));
}
if (default(DateTime) != this.LastUpdatedTimeTo)
{
conditions.Add(TableQuery.GenerateFilterConditionForDate("LastUpdatedTime", QueryComparisons.LessThanOrEqual, new DateTimeOffset(this.LastUpdatedTimeTo)));
}
if (!string.IsNullOrEmpty(this.RuntimeStatus))
{
conditions.Add(TableQuery.GenerateFilterCondition("RuntimeStatus", QueryComparisons.Equal, this.RuntimeStatus));
}
if (conditions.Count == 1)
{
return conditions[0];
} else
{
string lastCondition = null;
foreach(var condition in conditions)
{
if (string.IsNullOrEmpty(lastCondition))
{
lastCondition = condition;
continue;
}
lastCondition = TableQuery.CombineFilters(lastCondition, TableOperators.And, condition);
}
return lastCondition;
}
}
}
}
使い方
public static async Task QueryAsync()
{
var storageAccount = CloudStorageAccount.Parse(Configuration["ConnectionString"]);
var client = storageAccount.CreateCloudTableClient();
var instanceTable = client.GetTableReference("DurableFunctionsHubInstances");
var builder = new OrchestrationInstanceStatusQueryBuilder();
builder.AddRuntimeStatus("Completed")
.AddCreatedTime(new DateTime(2018, 7, 30, 0, 0, 0, DateTimeKind.Utc), new DateTime(2018, 7, 30, 23, 59, 59, DateTimeKind.Utc));
var query = builder.Build();
TableContinuationToken continuationToken = null;
do
{
var request = await instanceTable.ExecuteQuerySegmentedAsync(query, continuationToken);
var instances = request.ToList();
Console.WriteLine(JsonConvert.SerializeObject(instances));
continuationToken = request.ContinuationToken;
} while (continuationToken != null);
}
何故レビューをしてもらおうと思ったか？
レビューをしてもらおうと思ったのは、もっときれいなコードを書きたいというのもあるし、Query の Condition オブジェクトを ServiceCollection のBuilderをこの前触ったのが頭にあったのか、似たような実装をしてみた。ただ、Builder にしちゃったのはどうなんだろう、もっと単純に Condition のオブジェクトにして、属性も隠蔽しなくてもよかったのでは？という設計的な判断に悩んで、悩んでも答えはでないので、自分の師匠のかずきさんにレビューしてもらうことにした。
かずきさんに師匠をお願いしている理由
なぜかずきさんに師匠をお願いしているか？というと、昔一緒に彼とお客さんとモブプログラミングをやったことがあるのだが、彼は何のリファレンスも検索もすることなく、高度で美しいコードを目の前で一瞬で書き上げて、尚且つそのインサイドを高度理解しながらその場で解説しているのを見て衝撃を受けたからです。師匠のコードを書くスピードと完成度は目を見張るばかり。そこで師匠をお願いしてみました。
レビュー結果のまとめ
かずきさんにURLをはってしばらくすると、なんとかずきさんは、私が結構時間をかけて書いたコードを一瞬でリファクタリングして送り返してくれた。彼が解説してくれたポイント、私が質問した内容と、回答に関して整理しておく
いろいろオシャレな感じになっているがポイントを解説していきたい。
using Microsoft.WindowsAzure.Storage;
using Microsoft.WindowsAzure.Storage.Table;
using Newtonsoft.Json;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
namespace ConsoleApp1
{
class Program
{
static void Main(string[] args)
{
var r = new DurableFunctionsHubInstancesRepository("接続文字列");
var results = r.GetAllAsync(new OrchestrationInstanceStatusQueryCondition
{
RuntimeStatus = "Completed",
CreatedTime = new DateTimePeriod(new DateTime(2018, 7, 30, 0, 0, 0, DateTimeKind.Utc), new DateTime(2018, 7, 30, 23, 59, 59, DateTimeKind.Utc)),
}).Result;
Console.WriteLine(JsonConvert.SerializeObject(results));
Console.ReadLine();
}
}
public class OrchestrationInstanceStatus : TableEntity
{
public string ExecutionId { get; set; }
public string Name { get; set; }
public string Version { get; set; }
public string Input { get; set; }
public string InputBlobName { get; set; }
public string Output { get; set; }
public string OutputBlobName { get; set; }
public string CustomStatus { get; set; }
public DateTime CreatedTime { get; set; }
public DateTime LastUpdatedTime { get; set; }
public string RuntimeStatus { get; set; }
}
public interface IDurableFunctionsHubInstancesRepository
{
Task<IEnumerable<OrchestrationInstanceStatus>> GetAllAsync(OrchestrationInstanceStatusQueryCondition condition);
}
public class DurableFunctionsHubInstancesRepository : IDurableFunctionsHubInstancesRepository
{
private string TableStorageConnectionString { get; }
private Lazy<CloudTable> CloudTable { get; }
public DurableFunctionsHubInstancesRepository(string tableStorageConnectionString)
{
TableStorageConnectionString = tableStorageConnectionString;
CloudTable = new Lazy<CloudTable>(() =>
{
var storageAccount = CloudStorageAccount.Parse(TableStorageConnectionString);
var client = storageAccount.CreateCloudTableClient();
return client.GetTableReference("DurableFunctionsHubInstances");
});
}
public async Task<IEnumerable<OrchestrationInstanceStatus>> GetAllAsync(OrchestrationInstanceStatusQueryCondition condition)
{
var instanceTable = CloudTable.Value;
TableContinuationToken continuationToken = null;
var result = new List<OrchestrationInstanceStatus>();
do
{
var request = await instanceTable.ExecuteQuerySegmentedAsync(condition?.ToTableQuery() ?? new TableQuery<OrchestrationInstanceStatus>(), continuationToken);
result.AddRange(request);
continuationToken = request.ContinuationToken;
} while (continuationToken != null);
return result;
}
}
public class DateTimePeriod
{
public DateTime From { get; }
public DateTime To { get; }
public DateTimePeriod(DateTime from, DateTime to)
{
From = from;
To = to;
}
}
public class OrchestrationInstanceStatusQueryCondition
{
public string RuntimeStatus { get; set; }
public DateTimePeriod CreatedTime { get; set; }
public DateTimePeriod LastUpdatedTime { get; set; }
public TableQuery<OrchestrationInstanceStatus> ToTableQuery() => new TableQuery<OrchestrationInstanceStatus>()
.Where(GetConditions());
private string GetConditions()
{
var conditions = new List<string>();
if (this.CreatedTime != null)
{
conditions.Add(TableQuery.GenerateFilterConditionForDate("CreatedTime", QueryComparisons.GreaterThanOrEqual, new DateTimeOffset(this.CreatedTime.From)));
conditions.Add(TableQuery.GenerateFilterConditionForDate("CreatedTime", QueryComparisons.LessThanOrEqual, new DateTimeOffset(this.CreatedTime.To)));
}
if (this.LastUpdatedTime != null)
{
conditions.Add(TableQuery.GenerateFilterConditionForDate("LastUpdatedTime", QueryComparisons.GreaterThanOrEqual, new DateTimeOffset(this.LastUpdatedTime.From)));
conditions.Add(TableQuery.GenerateFilterConditionForDate("LastUpdatedTime", QueryComparisons.LessThanOrEqual, new DateTimeOffset(this.LastUpdatedTime.To)));
}
if (!string.IsNullOrEmpty(this.RuntimeStatus))
{
conditions.Add(TableQuery.GenerateFilterCondition("RuntimeStatus", QueryComparisons.Equal, this.RuntimeStatus));
}
if (conditions.Count == 1)
{
return conditions[0];
}
else
{
return conditions.Aggregate((a, b) => TableQuery.CombineFilters(a, TableOperators.And, b));
}
}
}
}
1. Builder or Condition
かずきさんは、Condition をチョイス。確かにこっちのほうが良さげです。次のようにコメントをいただいたのですが
C# はオブジェクトのインスタンス組みたてるだけの Builder 系クラスってあんまり有意義じゃないんですよねぇ
あ、今回の TableQuery のような組み立てるのがめんどくさいクラスを組み立てるための Builder は有意義だと思いますよ
そういうのじゃなければオブジェクト初期化子で行けるので
後で気づくのですが、初期化子で行けるほうがこのユースケースではよさげです。
2. DateTimePeriod
私がCreatedTimeFrom, CreatedTimeTo をつかっていると、何気にさっとオブジェクトを作っていてセンスがいいなと思いました。
public class DateTimePeriod
{
public DateTime From { get; }
public DateTime To { get; }
public DateTimePeriod(DateTime from, DateTime to)
{
From = from;
To = to;
}
}
何故作ったかを聞いてみました。
因みに牛尾さんの Builder のメソッドがFrom, Toの日付をまとめて受け取るようになってたので、FromとToは必ずセットで設定されるという風に読み取ったので、こういう風にしました
わたしのコードだとそういう風に読み取れるのか、、、実際はFrom だけとか、To だけとかあるんですけど、、、コードがちゃんと表現できていないポイントですね。そのことをかずきさんにいうと
その場合だと牛尾さんのコードでも
Builder の AddCreatedTime メソッドもfrom, to を受け取るシグネチャなので
不便そうですね
あー、今回のユースケースは、Builder だめだわ。いろんなパターンの、Addxxx メソッド実装しなくてはならなくなるので面倒だわ。絶対初期化子のほうがいい！つまり、今回は Condition 一択で納得しました。
new Condition
{
RuntimeStatus = "xxx",
CreatedTimeFrom = xxxx,
CreatedTimeTo = xxxx,
LastUpdatedFrom = xxx,
};
3. Repository
私のコードはスパイクなので単なる書きなぐったコードになっていますが、かずきさんのコードをみるとしれっとリポジトリができています。こんなちょっとしたコードでもさっとそれを作るセンスが素敵です
public interface IDurableFunctionsHubInstancesRepository
{
Task<IEnumerable<OrchestrationInstanceStatus>> GetAllAsync(OrchestrationInstanceStatusQueryCondition condition);
}
public class DurableFunctionsHubInstancesRepository : IDurableFunctionsHubInstancesRepository
{
private string TableStorageConnectionString { get; }
private Lazy<CloudTable> CloudTable { get; }
:
使用
var r = new DurableFunctionsHubInstancesRepository("接続文字列");
var results = r.GetAllAsync(new OrchestrationInstanceStatusQueryCondition
{
RuntimeStatus = "Completed",
CreatedTime = new DateTimePeriod(new DateTime(2018, 7, 30, 0, 0, 0, DateTimeKind.Utc), new DateTime(2018, 7, 30, 23, 59, 59, DateTimeKind.Utc)),
}).Result;
こういうちょっとしたところでもさくっとこういう整理を手が勝手に動くようにやるのがおしゃれですが、自分もそうなりたいので、彼に何故これを作ったかきいてみました。名前も、わたしのしょぼこーどから、それぽっぽいネーミングをしっかりするところもさすがです。
この処理呼びたい人がみんなTableClientとか作るのめんどそうだなと思ったので、何かしらのクラスにラップしたいなって思った感じです別に static なメソッドが1つあるだけでもいいです
こういうのを思いつくのもしびれるあこがれるポイントです。
4. Aggregation
私のクソださコードだとこんなのを書いているところも
string lastCondition = null;
foreach(var condition in conditions)
{
if (string.IsNullOrEmpty(lastCondition))
{
lastCondition = condition;
continue;
}
lastCondition = TableQuery.CombineFilters(lastCondition, TableOperators.And, condition);
}
return lastCondition;
Linq をつかってさらっと直してくれています。
return conditions.Aggregate((a, b) => TableQuery.CombineFilters(a, TableOperators.And, b));
Aggregate に相当するのは、Ruby でも関数型言語をやったときもありましたが、Linq での使い方はしりませんでした。やっぱり Linq は一通りマスターしておくほうが良さげです。このメソッドはリファレンスを見るとかなりよくわからないですが、ブログで事例から学ぶほうがよさげ。(a, b) のうち、最初のものが、Aggregate をとおして共有される変数で、bのほうが、各エレメントを表します。
5. Lazy 句
こんな単純なサンプルですが、よく見ると、CloudTable のところに Lazy がついています。
CloudTable = new Lazy<CloudTable>(() =>
{
var storageAccount = CloudStorageAccount.Parse(TableStorageConnectionString);
var client = storageAccount.CreateCloudTableClient();
return client.GetTableReference("DurableFunctionsHubInstances");
});
なぜでしょうか？
newで作ってたらインスタンス生成が重そうなので
あと、自分でif文書いて、初回実行だったらCloudTable作るとか書くのがめんどくさかったとかですかねぇ
今回は指定してないですがLazyの第二引数にtrue指定すると、マルチスレッド環境でも、いい感じに動いてくれるようになります
プログラマは怠惰重要。ここも思いつきもしなかったポイントです。
6. ToTableQuery と Extension Method
最後に.NET のネーミングについて知りたかったので、この名前が標準なのか聞いてみました。
public TableQuery<OrchestrationInstanceStatus> ToTableQuery()
BuilderならBuildだけど、そうじゃないならToListやToArrayにならってそうしてますねー
でも、この処理はConditionにいるのは少し疑問を感じながら書いてます
例えばAzure Storageじゃなくてもイケるようにインターフェース切ったのに、引数がそもそもテーブル前提の処理持ってるのはいただけないなーとなので、拡張メソッドにするかRepositryに押し込んでもいいかもしれませんただ、Repositryに押し込むとテストしたかったらpublicにしますが、そうなるとRepositryにはふさわしくない感じになるので、拡張メソッドかなぁ…
なるほど、C# で拡張メソッドがでてくるのですが、テスタビリティが良くないので好きじゃないのですが、やっとみんな使う理由がわかってきました。一般的なものがあり、その拡張的なメソッドがあります。今回は StorageTable を使いますが、他の実装にもConditionを対応させたいなと思ったときに、ToTableQuery はStorageTable に依存しすぎです。だからといって、このメソッドを抽象化することもできませんし、あればStorageTable 的にはうれしいのは間違いない、、、、というケースで Extension メソッドを書けばいいのでしょう。実装するとしたらこんな感じでしょうか。
public static class OrchestrationInstanceStatusQueryConditionExtensions {
public static TableQuery<OrchestrationInstanceStatus> ToTableQuery(this OrchestrationInstanceStatusQueryCondition condition) {
:
}
まとめ
こんな小さなプログラムでもイケてる人にレビューをしてもらうとめっちゃくちゃ違うことがよくわかりました。もっと勉強しなくては、、、私は、かき捨てのプログラムを書くときはべたっとやってしまいがちですが、師匠のように普段からそういうセンスでやっていると普段からもっといいコードが書けるのかもしれません。ちなみに、今実際にコントリビュート中ですが、Pull Request はこんな感じにしました。
Accept されるかはしらんけどｗ