読者です 読者をやめる 読者になる 読者になる

$shibayu36->blog;

プログラミングの話や自分の考えを色々と書いています。特にperl、emacsや読んだ本の話が多いです。

コードレビューを段階的に行ってもらう話

tech

 最近コードレビューをどのように回していくかについて考えたことがあったのでブログに書いておく。

コードレビューの目的

 コードレビューには誤りの発見以外にいろいろな目的がある。自分の中ではid:hisaichi5518が昔プレゼンでまとめていた目的が結構しっくり来ている。

  • 機械的に発見できない誤りの発見
  • 技術力の向上
  • 属人性の排除

 コードレビューの目的としては誤りの発見と同様に、技術力の向上や属人性の排除といった教育的側面も重要である。

コードレビューで課題に思っていたこと

 自分のチームでは基本的に一人がコードレビューをして、OKだったらmergeをして良いという運用だった。ただし、チームにずっといる人(ベテラン)とチームに新しく入った人(新人)が混ざっている場合に以下のような課題があった

  • レビューするコードへの知識がないと、誤りを発見することが難しい
  • 誤りの発見が出来ないのでは、と感じることで新人がコードレビューを躊躇する
  • 結果としてベテランがコードレビューをし続け、負担が大きい
  • またコードレビューによる教育的側面があまり満たされないため、その後もベテランがコードレビューし続ける

コードレビューを段階的に行ってもらう

 上のような課題の解決として、PullRequestを投げた人以外の全メンバーがコードレビューをするということも考えられる。ただ、それでは教育的な側面を担保するためにコストが多すぎる。

 そこで、教育的側面を満たしながら、誤りの発見をおろそかにしないように、新人にはコードレビューを段階的に行ってもらうという解決方法を取ることにした。

段階1 : 新人にレビューはしてもらうが、mergeするにはベテランのレビューが必要

 最初は、

  • 新人にもいろいろとレビューをしてもらう
  • ただし、新人のレビューだけではPull Requestをmergeしてはならず、mergeするにはベテランのレビューが必要

という段階を取った。また、この時はレビューをしてもらうだけではなく、わからないことがあったらとにかく質問してもらうくらいで良い、ということを意識してもらった。

 この段階は、新人には教育的側面のメリットを受けてもらい、ベテランが誤りの発見の責任を持つ、というスタイルである。

段階2 : 本番で動かないコードであれば、新人のレビューだけでmergeしてよい

 少し慣れてきたなーと感じたら、次の段階に進む。

  • 本番で動かないコードなら新人のレビューだけでmergeしてよい
    • 本番で動かないコードとは、テストとか、ちょっとした便利スクリプトとか、開発環境でしか使わないコードとかそういうもの
  • ただし、本番に出るコードはまだベテランのレビューを受けないとmergeしてはいけない

 この段階は、誤りの発見の責任を少しずつ新人に委譲していき、ベテランの負荷を下げていくという段階である。

段階3 : 一人のレビューでmergeしてよい

 さらに進んで、大分コードへの知識が溜まってきたなーと感じたら、最終段階として全ての制約を取り払って一人のレビューでmergeして良いところに進む。これで、ベテランと同じようにレビューをして良いという状態となる。

 この段階でレビューが出来る人が一人増えるので、ベテランの負荷も下がる。

 もちろん最終段階に進んだとしても、このコードは非常に重要だから二人くらい見たほうが良いとか、この部分の知識は浅いから別の人に見てもらおうとか、柔軟に取り扱っている。

まとめ

 今回はコードレビューで課題に思っていたことに対して、コードレビューを段階的に行ってもらうというアプローチを取ったことを紹介した。個人的には段階的にコードレビューをしてもらうことで教育的側面が満たされやすく、結果としてレビュワーが増えるので、ベテランの負荷が少しずつ下がっていくだろうと思っている。

JSDOM環境でlocalStorageをfakeする(with TypeScript)

tech

JSDOM環境を使っていると、いくつか実装されていないAPIがある。もしそのAPIを使っている実装のテストを書きたい場合、そのAPIと近い動きをする仮のオブジェクトに置き換える、つまりfakeする必要がある。

localStorageもJSDOMでは現在動かないAPIなので、localStorageを使った機能をテストするにはfakeする必要がある。普通のJSの環境だと、https://github.com/tmpvar/jsdom/issues/1137#issuecomment-173039751 に書いてあるようなオブジェクトを代入すれば良いのだけど、TypeScriptだと型定義どおりに実装しないとうまくいかないので、今回はTypeScript環境でfakeするのをやってみた。

一番単純にfakeする

方法としては同じインターフェースを持った別のものをwindow.localStorageに代入すればよい。TypeScriptの型定義lib.d.tsとかを参照すると、localStorageはStorageインターフェースを実装する必要があり、

interface WindowLocalStorage {
    localStorage: Storage;
}

さらにStorageインターフェースを確認すると

interface Storage {
    length: number;
    clear(): void;
    getItem(key: string): any;
    key(index: number): string;
    removeItem(key: string): void;
    setItem(key: string, data: string): void;
    [key: string]: any;
    [index: number]: string;
}

のようにプロパティとしてlength, 関数としてclear, getItem, key, removeItem, setItem, 他にプロパティとしてはstring -> anyか、number -> stringのものを入れられることがわかる。

この中空最低限getItem, setItem, removeItemくらいを実装してみれば、簡単なfakeはできそう。fake用のクラスを定義したのは以下のとおり。

class FakeStorage implements Storage {
    length: number;
    [key: string]: any;
    [index: number]: string;

    constructor() {}

    getItem(key: string): any {
        return this[key];
    }

    setItem(key: string, data: string): void {
        this[key] = data;
    }

    removeItem(key: string): void {
        delete this[key];
    }

    key(index: number): string {
        // not implement
    }

    clear(): void {
        // not implement
    }
}

export { FakeStorage };

あとはこれを使って、JSDOM環境のwindow.localStorageに代入しておけば使える。

import { FakeStorage } from "./FakeStorage";
window.localStorage = new FakeStorage();
localStorage.setItem('hoge', 'fuga');
console.log(localStorage.getItem('hoge')); // fuga
localStorage.removeItem('hoge');
console.log(localStorage.getItem('hoge')); // null

// 使い終わったら元に戻す
window.localStorage = undefined;

これで実装でsetItem, getItem, removeItemしか使っていないなら、最低限のテストができるようになった。

他のAPIも実装する

上ので大体良いけど、興味本位でさらに他のAPIも実装してみた。実装したのは以下のとおり。

class FakeStorage implements Storage {
    [key: string]: any;
    [index: number]: string;

    constructor() {}

    get length(): number {
        return this._keys().length;
    }

    getItem(key: string): any {
        return this[key];
    }

    setItem(key: string, data: string): void {
        this[key] = data;
    }

    removeItem(key: string): void {
        delete this[key];
    }

    key(index: number): string {
        let key = this._keys()[index];
        return !!key ? key : null;
    }

    clear(): void {
        this._keys().forEach((key) => {
            this.removeItem(key);
        });
    }

    private _keys(): string[] {
        return Object.keys(this);
    }
}

lengthをgetterで実装し、あとはこのオブジェクトのkeysを取得し実装している。

localStorageをfakeするための便利ユーティリティを用意する

このままでもやりたいことは既にできたが、もしJSDOM環境にlocalStorageが実装された場合、最後にundefinedを代入してしまうとおかしくなってしまう。もし実装されたとしても、最初にwindow.localStorageを退避しておいて、最後に退避したオブジェクトを代入すればおかしくならないはず。

そこでそのようなことを簡単にできる便利なユーティリティを作ってみた。

interface FakeLocalStorage {
    restore(): void;
}

function useFakeLocalStorage(): FakeLocalStorage {
    let _origLocalStorage = window.localStorage;
    let fakeLocalStorage  = new FakeStorage();

    window.localStorage = fakeLocalStorage;

    return {
        restore: function () {
            window.localStorage = _origLocalStorage;
        }
    }
}

このuseFakeLocalStorage()は以下のように使える。

// useFakeLocalStorage()を呼び出すとwindow.localStorageが置き換わり
// まっさらなlocalStorageを使っている状態になる
let fake = useFakeLocalStorage();

// fakeしたものを使っていろいろ書ける
localStorage.setItem('hoge', 'fuga');
console.log(localStorage.getItem('hoge')); // fuga
localStorage.removeItem('hoge');
console.log(localStorage.getItem('hoge')); // null

// fake.restore()を呼ぶと元の状態に戻る
fake.restore();

これを使ったらもしlocalStorageがJSDOMに実装されたとしても、特定のテストで一回localStorageがまっさらな状態でテストし、その後元に戻すということを簡単にできて嬉しい。

まとめ

今回はTypeScriptでJSDOM環境のlocalStorageをfakeする方法について書いた。TypeScriptの型定義ファイルを見るとインターフェースを参照できるので、それにしたがって作るだけで良いということが分かってよかった。他にもいい方法があれば教えて下さい。

そういえば https://github.com/tmpvar/jsdom/issues/1137#issuecomment-215997211 の議論で見つけた、node.js v6を使っていると、localStorageがPROXIESされる(?)ということがよく分かっていないけど、どういうことだろう?node v6を使えばJSDOMでlocalStorage使える?

シンタックスハイライトにTypeScriptを導入して欲しい

お題「シンタックス・ハイライト機能で対応してほしい言語」

最近TypeScriptをブログによく書くので導入して欲しい!JavaScriptを使ってもいいけど、シンタックスハイライトされないものもあるので...