意味のある名前をつける

昨日書いたコードには気になるところがあった。以下がそのコードだ。

public class MoveRM16SReg implements Instruction{
    public void execute(VM vm){
        int modrm = vm.getCode8(1);
        int index = VMUtil.getRegOp(modrm);
        int value = vm.getSegReg(index);

        VMUtil.setModRMData(vm, modrm, value);
        vm.nextEIP(2);
    }
}

ここにでてくるメソッドの名前から何をしてるかがわかりにくい。なので１つ１つ考え直していこうと思う。まずこれ

int modrm = vm.getCode8(1);

左の変数の名前からMod R/Mを取得してるのはわかるが、getCode8(1)ってのはわかりづらいと思う。メモリの(CS:EIP + 引数の値)番地の値を8bit分取得するコードなのだがそれ以上の意味がない。なのでgetModRMというメソッドを作り、getCode8(1)をラップする。まぁ、変数の名前で判断できそうなのでほっといてもよかったかもしれないが。次に

int index = VMUtil.getRegOp(modrm);

これはindexが何のindexかもわからないし、getRegOpというメソッド名からも何をしてるのかがわからないと思う。このgetRegOpはMod R/MからRegisterのindexかOpecodeを取得するものだが、どちらになるかはx86の命令によって異なる。どちらにしても取得する値は同じなので、分けると同じ内容のメソッドが２つできてしまうが、あえて２つ作った方がわかりやすい名前になりそうなのでそうした。以下の２つのメソッドに分離した。

int index = VMUtil.getRegisterIndex(modrm);

int code = VMUtil.getOpecode(modrm);

次に

int value = vm.getSegReg(index);

の部分。セグメントレジスタの値を取得している。が、なんとなく略しかたが気に入らなかったので修正。超個人的な問題だけど…SegRegやめてSRegisterにして、値を返すことを意識してValueをくっつけた。

int value = vm.getSRegisterValue();

次に

VMUtil.setModRMData(vm, modrm, value);

だが、ここはそのままにしておく。これはMod R/Mが表すレジスタまたはメモリに値を設定するものだが、Mod R/Mによっての分岐をここに書きたくなかったので、modrmを渡しるため、このような名前にした。最後に

vm.nextEIP(2);

であるが、EIPに引数の値を足してるだけなのでaddEIPが正しいだろうと思い変更

vm.addEIP(2)

これらの変更を加えたコード全体を載せておく

public class MoveRM16SReg implements Instruction{
    public void execute(VM vm){
        int modrm = vm.getModRM();
        int index = VMUtil.getRegisterIndex(modrm);
        int value = vm.getSRegisterValue(index);

        VMUtil.setModRMData(vm, modrm, value);
        vm.addEIP(2);
    }
}