条件式の塊を１つのメソッドに切り出すというのは、よくやる
リファクタリングの１つなんだけど。
たとえばjavax.sound.midi.MidiSystem#isAppropriateDeviceも、
そうした条件メソッドの１つ：
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;
}
この例では、「条件式の束をメソッドに切り出したところまでは
よかったけど、その切り出したメソッドの中身がゴチャゴチャ
していた」というわりとよくある光景を見た。
「素直に書く」というのはやっぱり間違いを生みやすい表現なので、
「バカ単純に書く」という表現のほうがいい。||や&&を巧みに
つなぎ合わせたほうが賢く見えるかもしれないけど、そんなことは
気にしない。バカに見えてもいいから単純に書く。
自分がよくいう「丁寧に書く」というのは、「バカ単純に書く」と
いうことも含まれていて。バカ単純なコードというのはサラっと読み
流せるもんだから、その凄みに気づかないことも多いんだけど、
手間をかけないとなかなかバカ単純にはならない。
Copyright © 1905 tko at jitu.org