<< 前 ホーム 次 >>

bakaid: 20120204

条件式の塊を1つのメソッドに切り出すというのは、よくやる
リファクタリングの1つなんだけど。

たとえばjavax.sound.midi.MidiSystem#isAppropriateDeviceも、
そうした条件メソッドの1つ:

    private static boolean isAppropriateDevice(MidiDevice device,
                                               Class deviceClass,
                                               boolean allowSynthesizer,
                                               boolean allowSequencer) {
        if (deviceClass.isInstance(device)) {
            // This clause is for deviceClass being either Synthesizer
            // or Sequencer.
            return true;
        } else {
            // Now the case that deviceClass is Transmitter or
            // Receiver. If neither allowSynthesizer nor allowSequencer is
            // true, we require device instances to be
            // neither Synthesizer nor Sequencer, since we only want
            // devices representing MIDI ports.
            // Otherwise, the respective type is accepted, too
            if ( (! (device instanceof Sequencer) &&
                  ! (device instanceof Synthesizer) ) ||
                 ((device instanceof Sequencer) && allowSequencer) ||
                 ((device instanceof Synthesizer) && allowSynthesizer)) {
                // And of cource, the device has to be able to provide
                // Receivers or Transmitters.
                if ((deviceClass == Receiver.class &&
                     device.getMaxReceivers() != 0) ||
                    (deviceClass == Transmitter.class &&
                     device.getMaxTransmitters() != 0)) {
                    return true;
                }
            }
        }
        return false;
    }

まぁ、コメントがゴチャゴチャうるさいので消しておくと:

    private static boolean isAppropriateDevice(MidiDevice device,
                                               Class deviceClass,
                                               boolean allowSynthesizer,
                                               boolean allowSequencer) {
        if (deviceClass.isInstance(device)) {
            return true;
        } else {
            if ( (! (device instanceof Sequencer) &&
                  ! (device instanceof Synthesizer) ) ||
                 ((device instanceof Sequencer) && allowSequencer) ||
                 ((device instanceof Synthesizer) && allowSynthesizer)) {
                if ((deviceClass == Receiver.class &&
                     device.getMaxReceivers() != 0) ||
                    (deviceClass == Transmitter.class &&
                     device.getMaxTransmitters() != 0)) {
                    return true;
                }
            }
        }
        return false;
    }

あ、ちなみに、この書き方、Javaの標準から外れてるんで、それも
直すと:

    private static boolean isAppropriateDevice(MidiDevice device,
                                               Class deviceClass,
                                               boolean allowSynthesizer,
                                               boolean allowSequencer) {
        if (deviceClass.isInstance(device)) {
            return true;
        } else {
            if ((!(device instanceof Sequencer)
                 && !(device instanceof Synthesizer))
                || ((device instanceof Sequencer) && allowSequencer)
                || ((device instanceof Synthesizer) && allowSynthesizer)) {
                if ((deviceClass == Receiver.class
                     && device.getMaxReceivers() != 0)
                    || (deviceClass == Transmitter.class
                        && device.getMaxTransmitters() != 0)) {
                    return true;
                }
            }
        }
        return false;
    }

まぁ、returnなのにelseでつなぐ人が多いんだけど、インデントが
深くなるだけコードのムダなので:

    private static boolean isAppropriateDevice(MidiDevice device,
                                               Class deviceClass,
                                               boolean allowSynthesizer,
                                               boolean allowSequencer) {
        if (deviceClass.isInstance(device)) {
            return true;
        }
        if ((!(device instanceof Sequencer)
             && !(device instanceof Synthesizer))
            || ((device instanceof Sequencer) && allowSequencer)
            || ((device instanceof Synthesizer) && allowSynthesizer)) {
            if ((deviceClass == Receiver.class
                 && device.getMaxReceivers() != 0)
                || (deviceClass == Transmitter.class
                    && device.getMaxTransmitters() != 0)) {
                return true;
            }
        }
        return false;
    }

で、これだけやってもまだよくわからんと。で、引数であるallowSynthesizerと
allowSequencerに注目する。これがfalseのときの挙動を
考えると:

    private static boolean isAppropriateDevice(MidiDevice device,
                                               Class deviceClass,
                                               boolean allowSynthesizer,
                                               boolean allowSequencer) {
        if (deviceClass.isInstance(device)) {
            return true;
        }
        if ((device instanceof Sequencer) && !allowSequencer) {
            return false;
        }
        if ((device instanceof Synthesizer) && !allowSynthesizer) {
            return false;
        }
        if ((!(device instanceof Sequencer)
             && !(device instanceof Synthesizer))
            || (device instanceof Sequencer)
            || (device instanceof Synthesizer)) {
            if ((deviceClass == Receiver.class
                 && device.getMaxReceivers() != 0)
                || (deviceClass == Transmitter.class
                    && device.getMaxTransmitters() != 0)) {
                return true;
            }
        }
        return false;
    }

こうすると、最後のダラダラと長いinstanceofの連なりがまったくの
無意味であることがわかったのでバッサリ消す:

    private static boolean isAppropriateDevice(MidiDevice device,
                                               Class deviceClass,
                                               boolean allowSynthesizer,
                                               boolean allowSequencer) {
        if (deviceClass.isInstance(device)) {
            return true;
        }
        if ((device instanceof Sequencer) && !allowSequencer) {
            return false;
        }
        if ((device instanceof Synthesizer) && !allowSynthesizer) {
            return false;
        }
        if ((deviceClass == Receiver.class
             && device.getMaxReceivers() != 0)
            || (deviceClass == Transmitter.class
                && device.getMaxTransmitters() != 0)) {
            return true;
        }
        return false;
    }

これで終わりにしてもいいんだけど、最後の条件式がまだ長いん
で、これもバラす:

    private static boolean isAppropriateDevice(MidiDevice device,
                                               Class deviceClass,
                                               boolean allowSynthesizer,
                                               boolean allowSequencer) {
        if (deviceClass.isInstance(device)) {
            return true;
        }
        if ((device instanceof Sequencer) && !allowSequencer) {
            return false;
        }
        if ((device instanceof Synthesizer) && !allowSynthesizer) {
            return false;
        }
        if (deviceClass == Receiver.class && device.getMaxReceivers() != 0) {
            return true;
        }
        if (deviceClass == Transmitter.class && device.getMaxTransmitters() != 0) {
            return true;
        }
        return false;
    }

この例では、「条件式の束をメソッドに切り出したところまでは
よかったけど、その切り出したメソッドの中身がゴチャゴチャ
していた」というわりとよくある光景を見た。

「素直に書く」というのはやっぱり間違いを生みやすい表現なので、
「バカ単純に書く」という表現のほうがいい。||や&&を巧みに
つなぎ合わせたほうが賢く見えるかもしれないけど、そんなことは
気にしない。バカに見えてもいいから単純に書く。

自分がよくいう「丁寧に書く」というのは、「バカ単純に書く」と
いうことも含まれていて。バカ単純なコードというのはサラっと読み
流せるもんだから、その凄みに気づかないことも多いんだけど、
手間をかけないとなかなかバカ単純にはならない。

本家Permlink

<< 前 ホーム 次 >>


Copyright © 1905 tko at jitu.org

バカが征く on Rails