一些实用的重构代码技巧

目的

本文是来说明如何编写优秀的代码。好的代码不是一蹴而就，它需要不断重构，直到写出可读性，可测性，可拓展性的代码。

抛出代码的历史问题

这里用项目里面一个非常简单的功能做演示，引用了框架中对外的入口类
SmartHorizonSignalProvider。

可读性

下面列举了SmartHorizonSignalProvider的成员，可用发现类成员非常多，很难理解这个类的主要结构和依赖对象是什么。

Class Members

private static final int TIME_OUT_CHECK_INTERVAL = 5000;

private static final int TIME_OUT_LAST_MESSAGE_INTERVAL = 30000;

/**
* Flag that indicates if we are running the app on devices different than the HEAD UNIT.
*/
private boolean notOnHeadUnit;

/**
* adas message listener
*/
private AutoSdkNavigationService.AdasMessageListener adasMessageListener;

/**
* ADAS messages queue
*/
private Queue adasSmartHorizonMessageQueue;

/**
* broadcast thread
*/
private BroadcastThread broadcastThread;

/**
* value at which time frame the messages will be send
*/
private int frequencyValue;

/**
* last message received time
*/
private long lastMessageReceivedTime;

/**
* time out thread - used to stop the broadcasting process
*/
private TimeOutThread timeOutThread;

/**
* intent broadcast controller
*/
private IntentBroadcastController intentBroadcastController;

/**
* HeadUnit broadcast controller
*/
private HUBroadcastController huBroadcastController;

Method

下面这段方法，其中public方法和private方法混合调用，而且有结构很混乱，内部也违背了单一职责原则，它包括了多个操作，并不是startProvider()方法名所说的启动一个提供者。步骤：

读取系统属性.
判断系统设置是否打开.
如果设置打开，则注册监听器.

public void startProvider() {
        AutoSdkLog.log(LogEnum.LogPriority.debug, ArpLogFilter.LOG_MODULE, this.getClass(), "adasis startProvider");
        String smartHorizonFrequencyString = ApplicationContextHelper.getInstance().getConfigSettings(ApplicationContextHelper.KEY_ADAS_SMART_HORIZON_FREQUENCY);
        if (smartHorizonFrequencyString != null) {
            try {
                int frequencyValue = Integer.parseInt(smartHorizonFrequencyString);
                if (frequencyValue <= 0) {
                    this.frequencyValue = 20;
                } else {
                    this.frequencyValue = frequencyValue;
                }
            } catch (NumberFormatException e) {
                this.frequencyValue = 20;
            }
        }
 
        notOnHeadUnit = !ApplicationContextHelper.getInstance().isGMHeadUnit();
        if (notOnHeadUnit) {
            intentBroadcastController = new IntentBroadcastController();
        } else {
            huBroadcastController = new HUBroadcastController();
        }
 
        addAdasMessageListener();
}

public API name

在SmartHorizonSignalProvider类中，有个两个公共方法startProvider()、stopProvider()，但是类名中已经表示是provider这个动作，所以这里显得有点多余，用start\stop()命名就可以了。纵观上下文，BoradcaseControl、timeOutThread这样的命名也难以理解。

public void startProvider();
public void stopProvider();

可测试性

在SmartHorizonSignalProvider类是很难测试的，理由包括：
1.依赖于具体实现类，不容易被外部拓展，也难以被mock.
2.方法直接操作单例类.
3.内部类直接在类里面就被初始化.

可拓展性

在编写可扩展性代码一般是按照SOLID原则进行.
S- Single principle单一职责原则SmartHorizonSignalProvider这个类做了太多事，不止一件。如定义线程、开启线程、关闭线程、接收ADSD消息等。

/**
 * start time out thread
 */
private void startTimeOutThread() {
    AutoSdkLog.log(LogEnum.LogPriority.debug, ArpLogFilter.LOG_MODULE, this.getClass(), "adasis startTimeOutThread");
    timeOutThread = new TimeOutThread(TIME_OUT_CHECK_INTERVAL);
    timeOutThread.start();
}
 
/**
 * stop threads
 */
private void stopThreads() {
    AutoSdkLog.log(LogEnum.LogPriority.debug, ArpLogFilter.LOG_MODULE, this.getClass(), "adasis stopThreads");
    if (broadcastThread != null) {
        broadcastThread.cancel();
        broadcastThread = null;
    }
    if (timeOutThread != null) {
        timeOutThread.cancel();
        timeOutThread = null;
    }
}
 
/**
 * @param smartHorizonMessagesList List of SMART HORIZON messages received.
 */
private void smartHorizonMessageReceived(List<AdasMessage> smartHorizonMessagesList) {
    ...   
}
 
/**
 * handle adas messages queue
 */
private synchronized void handleMessagesQueue() {
    AdasMessage adasMessage = adasSmartHorizonMessageQueue.peek();
    if(adasMessage == null) {
        AutoSdkLog.log(LogEnum.LogPriority.debug, ArpLogFilter.LOG_MODULE, this.getClass(),
                "adasis handleMessagesQueue NULL broadcast. Nothing to sent.");
        return;
    }
 
    AutoSdkLog.log(LogEnum.LogPriority.debug, ArpLogFilter.LOG_MODULE, this.getClass(),
        "adasis handleMessagesQueue broadcast with messageSource = " + adasMessage.getMessageSource());
    broadcastMessage(adasMessage);
 
    adasSmartHorizonMessageQueue.poll();
    AutoSdkLog.log(LogEnum.LogPriority.debug, ArpLogFilter.LOG_MODULE, this.getClass(),
            "adasis handleMessagesQueue new queue size = " + adasSmartHorizonMessageQueue.size());
}
 
/**
 * broadcast ADAS message
 *
 * @param message the ADAS message
 */
private void broadcastMessage(AdasMessage message) {
    if (notOnHeadUnit) {
        if (intentBroadcastController != null) {
            AutoSdkLog.log(LogEnum.LogPriority.debug, ArpLogFilter.LOG_MODULE, this.getClass(), "adasis broadcastMessage on TABLET. "
                    + "Message with type = " + message.getType() + ", source = " + message.getMessageSource()
                    + ", content = " + message.getContent());
            intentBroadcastController.handleMessage(message);
        } else {
            AutoSdkLog.log(LogEnum.LogPriority.debug, ArpLogFilter.LOG_MODULE, this.getClass(),
                    "adasis broadcastMessage intentBroadcastController is NULL");
        }
    } else {
        if (huBroadcastController != null) {
            AutoSdkLog.log(LogEnum.LogPriority.debug, ArpLogFilter.LOG_MODULE, this.getClass(), "adasis broadcastMessage on HEAD UNIT. "
                    + "Message with type = " + message.getType() + ", source = " + message.getMessageSource()
                    + ", content = " + message.getContent());
            huBroadcastController.handleMessage(message);
        } else {
            AutoSdkLog.log(LogEnum.LogPriority.debug, ArpLogFilter.LOG_MODULE, this.getClass(),
                    "adasis broadcastMessage huBroadcastController is NULL");
        }
    }
}
 
private class BroadcastThread extends Thread {
 
    /**
     * wait time value when trying to broadcast a new message
     */
    private int waitTime;
 
    /**
     * false if the thread should stop, true otherwise
     */
    private boolean notCancelled = true;
 
    public BroadcastThread(int waitTime) {
        AutoSdkLog.log(LogEnum.LogPriority.debug, ArpLogFilter.LOG_MODULE, BroadcastThread.class,
                "adasis new BroadcastThread waitTime =  " + waitTime);
        this.waitTime = waitTime;
        adasSmartHorizonMessageQueue = new LinkedList<>();
        startTimeOutThread();
    }
 
    @Override
    public void run() {
        ...
    }
 
    /**
     * cancel thread
     */
    public void cancel() {
        AutoSdkLog.log(LogEnum.LogPriority.debug, ArpLogFilter.LOG_MODULE, this.getClass(), "adasis cancel thread");
        notCancelled = false;
    }
}
 
private class TimeOutThread extends Thread {
    /**
     * wait time when checking for last message received
     */
    private int waitTime;
 
    /**
     * false if the thread should stop, true otherwise
     */
    private boolean notCancelled = true;
 
    public TimeOutThread(int waitTime) {
        AutoSdkLog.log(LogEnum.LogPriority.debug, ArpLogFilter.LOG_MODULE, this.getClass(), "adasis new TimeOutThread waitTime =  " + waitTime);
        this.waitTime = waitTime;
    }
 
    @Override
    public void run() {
        ....
    }
 
    /**
     * cancel thread
     */
    public void cancel() {
        AutoSdkLog.log(LogEnum.LogPriority.debug, ArpLogFilter.LOG_MODULE, this.getClass(), "adasis cancel thread");
        notCancelled = false;
    }
}

O- Open/Close principle 开闭原则
在这里当前package目录下，如果不是必要，没必要全部类都是public类型。另外，要想不修改现有的代码，修改一些默认的UI行为，需要通过暴露一个接口，让其他Client去实现接口，这里没有专门的接口支持这个方案。

L- Liskov substitution principle 里斯提夫替代原则
这里没有用到这个原则。

I - Interface segregation principle接口隔离原则
因为接口少，这里没有用到这个原则。

D- Dependency inversion principle 依赖倒转原则
依赖倒置原则这里也没有实现，这是由于依赖的是具体实现类，而不是接口。SmartHorizonSignalProvider引用了具体实现类intentBroadcastController和huBroadcastController。

private IntentBroadcastController intentBroadcastController;
     
private HUBroadcastController huBroadcastController;
  
private class BroadcastThread extends Thread {
 
        /**
         * wait time value when trying to broadcast a new message
         */
        private int waitTime;
 
        /**
         * false if the thread should stop, true otherwise
         */
        private boolean notCancelled = true;
 
        public BroadcastThread(int waitTime) {
            AutoSdkLog.log(LogEnum.LogPriority.debug, ArpLogFilter.LOG_MODULE, BroadcastThread.class,
                    "adasis new BroadcastThread waitTime =  " + waitTime);
            this.waitTime = waitTime;
            adasSmartHorizonMessageQueue = new LinkedList<>();
            startTimeOutThread();
        }
 
        @Override
        public void run() {
            ...
        }
 
        /**
         * cancel thread
         */
        public void cancel() {
            AutoSdkLog.log(LogEnum.LogPriority.debug, ArpLogFilter.LOG_MODULE, this.getClass(), "adasis cancel thread");
            notCancelled = false;
        }
    }

而BoradCastControl中的handleMessage()也是用的具体实现。


public void handleMessage(AdasMessage message) {
 ....     
}

开始重构历史问题代码

可读性

我们依然从可读性开始改造SmartHorizonSignalProvider类的ClassMebers，首先让它的成员变量更加精简，私有化了构造函数，通过Builder类来初始化对象。同时也提供了默认的初始化属性。

/**
 *  Smart Horizon Provider is a proxy between OEM CAN bus interface and
 *  Telenav SmartHorizon ADAS solution. Requires OEM specific
 *  @see AdasMessageHandler implementation.
 *  Provider can be used as is on stock Android devices in which case default
 *  broadcast channel is Intent bus.
 */
public class SmartHorizonProvider {
    private final List<AdasMessageHandler> adasMessageHandlers;
    private final AdasMessageReceiver receiver;
    private Scheduler scheduler;
 
    private boolean hasStarted;
 
    private SmartHorizonProvider(Builder builder) {
        this.receiver = builder.receiver;
        this.adasMessageHandlers = Collections.unmodifiableList(builder.adasMessageHandlers);
        this.scheduler = builder.scheduler;
        this.scheduler.registerExecutor(new Executor() {
            @Override
            public void run() {
                // Peek ADAS message and broadcast
                for (AdasMessageHandler handler: adasMessageHandlers) {
                    handler.handleAdasMessage(receiver.getMessage());
                }
            }
        });
    }
 
    /**
     * Start provider
     * @param interval broadcast interval in milliseconds
     */
    public void start(long interval) {
        if (hasStarted || interval <= 0) {
            return;
        }
 
        receiver.start();
        hasStarted = scheduler.start(interval);
    }
 
    /**
     *  Stop provider
     */
    public void stop() {
        receiver.stop();
        scheduler.cancel();
        hasStarted = false;
    }
 
    @VisibleForTesting
    void setScheduler(Scheduler scheduler) {
        this.scheduler = scheduler;
    }
 
    public static final class Builder {
        private static final int MAX_MESSAGES_IN_QUEUE = 20;
        private final List<AdasMessageHandler> adasMessageHandlers = new ArrayList<>();
        private final Scheduler scheduler = new BroadcastScheduler();
        private AdasMessageReceiver receiver;
 
        public Builder(@Nullable Context context) {
            /// Default initializations
            /// TODO: move this dependency upstream such that library is ARP SDK agnostic
            this.receiver = new ARPAdasMessageReceiver(AutoSdkNavigationService.getInstance(), MAX_MESSAGES_IN_QUEUE);
 
            /// Optional dependency
            if (context != null) {
                this.adasMessageHandlers.add(new BroadcastIntentTransmitter(context));
            }
        }
 
        /**
         * Override default ADAS receiver implementation
         * @param receiver reference to new receiver implementation
         * @return builder instance
         */
        public Builder setAdasMessageReceiver(@NonNull AdasMessageReceiver receiver) {
            this.receiver = receiver;
            return this;
        }
 
        /**
         * Add ADAS message handler (OEM specific)
         * @param messageHandler reference to Adas message handler
         * @return builder instance
         */
        public Builder addAdasMessageHandler(@NonNull AdasMessageHandler messageHandler) {
            if (adasMessageHandlers.contains(messageHandler)) {
                return this;
            }
 
            adasMessageHandlers.add(messageHandler);
            return this;
        }
 
        public SmartHorizonProvider build() {
            return new SmartHorizonProvider(this);
        }
    }
}

Method

创建了一个职责单一的接口，暴露了一个方法处理message。让api更加可读，同时写好了注释。

/**
 * Higher level abstraction for ADAS message handler
 * Concrete implementations are OEM specific
 * @param <T> generic ADAS message type
 */
public interface AdasMessageHandler<T> {
    /**
     * Handle ADAS message, null should be handled by client
     * before processing ADAS message, null implies no messages
     * are available for processing at the moment
     * @param message custom ADAS message
     */
    void handleAdasMessage(@Nullable T message);
}

可测试性

因为我们的SmartHorizonProvider依赖的对象都是面向接口，现在我们更加容易mock他们，来看看测试类如何写：

public class SmartHorizonProviderTest extends BaseTest {
    @Mock
    private AdasMessageHandler messageHandler;
    @Mock
    private AdasMessageReceiver messageReceiver;
    @Mock
    private Scheduler scheduler;
 
    SmartHorizonProvider smartHorizonProvider;
 
    @Before
    public void setup() {
        smartHorizonProvider = new SmartHorizonProvider.Builder(null)
                .addAdasMessageHandler(messageHandler)
                .setAdasMessageReceiver(messageReceiver)
                .build();
        smartHorizonProvider.setScheduler(scheduler);
    }
}

可拓展性

重构这个类的可扩展性，我们也通过SOLID原则进行。

Single Principle

现在我们的SmartHorizonProvider只负责开关provider一件事。另外以前的成员变量已修改成对应的类或接口去做单一的事情。

Open/Close Principle

我们这里已经有了两个接口一个private类，同时接口也提供了其他client在不改原有的代码前提下修改默认的行为。

Lusiko substitution Principle

这里我们不需要这个原则

Interface segregation principle

现在我们新定义了三个接口，专门负责三个独立的事情；

/**
 * Higher level abstraction for ADAS message handler
 * Concrete implementations are OEM specific
 * @param <T> generic ADAS message type
 */
public interface AdasMessageHandler<T> {
    /**
     * Handle ADAS message, null should be handled by client
     * before processing ADAS message, null implies no messages
     * are available for processing at the moment
     * @param message custom ADAS message
     */
    void handleAdasMessage(@Nullable T message);
}
  
/**
 * Higher level abstraction for ADAS message receiver
 * Receiver is bound to a specific implementation of ADAS daemon
 * @param <T> ADAS message type, implementation specific
 */
public interface AdasMessageReceiver<T> {
    /**
     * Start receiver, normally that's where initialization
     * of receiver should happen
     */
    void start();
 
    /**
     * Stop receiver, normally that's where cleanup state
     * of receiver should happen
     */
    void stop();
 
    /**
     * Return latest ADAS message, if message is unavailable return null
     * @return latest ADAS message
     */
    @Nullable T getMessage();
}
  
/**
 * Higher level abstraction for scheduler
 */
interface Scheduler {
    /**
     * Register executor callback, code that
     * needs to be executed periodically
     * @param executor reference to executor
     */
    void registerExecutor(@NonNull Executor executor);
 
    /**
     * Start scheduler
     * @param interval in millisecondss
     * @return true if scheduler is started successfully
     */
    boolean start(long interval);
 
    /**
     * Cancel scheduler
     */
    void cancel();
 
    interface Executor {
        /**
         *  Execute block of code
         */
        void run();
    }
}

Dependency inversion Princinle

我们现在的code没有直接依赖实现类，而是依赖抽象类。如果我们在不同的平台使用AdasMessageReceiver ，scheduler，现在我们只需要通过Builder注入新的实现类，不需要改变SmartHorizonProvider原有的内部逻辑。

    private final AdasMessageReceiver receiver;
    private Scheduler scheduler;

其他可用的技巧

在模块之间用不可变量的数据进行交互。
根据可扩展性的原则，我们要仔细设计公共类，为将来的重构打好基础。
不要暴露过多的public方法，如果没有特殊，可将构造函数私有化，多用Builder来初始化成员变量。
每次添加了新的代码，应该注意保证单元测试覆盖量。
不要期待一次性就能写到不需要重构的代码，而是要逐步迭代改进它。

最后编辑于：2018.07.21 06:47:47

人面猴
序言：七十年代末，一起剥皮案震惊了整个滨河市，随后出现的几起案子，更是在滨河造成了极大的恐慌，老刑警刘岩，带你破解...
沈念sama阅读 156,757评论 4赞 359
死咒
序言：滨河连续发生了三起死亡事件，死亡现场离奇诡异，居然都是意外死亡，警方通过查阅死者的电脑和手机，发现死者居然都...
沈念sama阅读 66,478评论 1赞 289
救了他两次的神仙让他今天三更去死
文/潘晓璐我一进店门，熙熙楼的掌柜王于贵愁眉苦脸地迎上来，“玉大人，你说我怎么就摊上这事。” “怎么了？”我有些...
开封第一讲书人阅读 106,540评论 0赞 237
道士缉凶录：失踪的卖姜人
文/不坏的土叔我叫张陵，是天一观的道长。经常有香客问我，道长，这世上最难降的妖魔是什么？我笑而不...
开封第一讲书人阅读 43,593评论 0赞 203
港岛之恋（遗憾婚礼）
正文为了忘掉前任，我火速办了婚礼，结果婚礼上，老公的妹妹穿的比我还像新娘。我一直安慰自己，他们只是感情好，可当我...
茶点故事阅读 51,903评论 3赞 285
恶毒庶女顶嫁案：这布局不是一般人想出来的
文/花漫我一把揭开白布。她就那样静静地躺着，像睡着了一般。火红的嫁衣衬着肌肤如雪。梳的纹丝不乱的头发上，一...
开封第一讲书人阅读 40,329评论 1赞 210
城市分裂传说
那天，我揣着相机与录音，去河边找鬼。笑死，一个胖子当着我的面吹牛，可吹牛的内容都是我干的。我是一名探鬼主播，决...
沈念sama阅读 31,659评论 2赞 309
双鸳鸯连环套：你想象不到人心有多黑
文/苍兰香墨我猛地睁开眼，长吁一口气：“原来是场噩梦啊……” “哼！你这毒妇竟也来了？” 一声冷哼从身侧响起，我...
开封第一讲书人阅读 30,383评论 0赞 195
万荣杀人案实录
序言：老挝万荣一对情侣失踪，失踪者是张志新（化名）和其女友刘颖，没想到半个月后，有当地人在树林里发现了一具尸体，经...
沈念sama阅读 34,055评论 1赞 238
护林员之死
正文独居荒郊野岭守林人离奇死亡，尸身上长有42处带血的脓包…… 初始之章·张勋以下内容为张勋视角年9月15日...
茶点故事阅读 30,337评论 2赞 241
白月光启示录
正文我和宋清朗相恋三年，在试婚纱的时候发现自己被绿了。大学时的朋友给我发了我未婚夫和他白月光在一起吃饭的照片。...
茶点故事阅读 31,864评论 1赞 256
活死人
序言：一个原本活蹦乱跳的男人离奇死亡，死状恐怖，灵堂内的尸体忽然破棺而出，到底是诈尸还是另有隐情，我是刑警宁泽，带...
沈念sama阅读 28,227评论 2赞 251
日本核电站爆炸内幕
正文年R本政府宣布，位于F岛的核电站，受9级特大地震影响，放射性物质发生泄漏。R本人自食恶果不足惜，却给世界环境...
茶点故事阅读 32,820评论 3赞 231
男人毒药：我在死后第九天来索命
文/蒙蒙一、第九天我趴在偏房一处隐蔽的房顶上张望。院中可真热闹，春花似锦、人声如沸。这庄子的主人今日做“春日...
开封第一讲书人阅读 25,999评论 0赞 8
一桩弑父案，背后竟有这般阴谋
文/苍兰香墨我抬头看了看天上的太阳。三九已至，却和暖如春，着一层夹袄步出监牢的瞬间，已是汗流浃背。一阵脚步声响...
开封第一讲书人阅读 26,750评论 0赞 192
情欲美人皮
我被黑心中介骗来泰国打工，没想到刚下飞机就差点儿被人妖公主榨干…… 1. 我叫王不留，地道东北人。一个月前我还...
沈念sama阅读 35,365评论 2赞 269
代替公主和亲
正文我出身青楼，却偏偏与公主长得像，于是被迫代替她去往敌国和亲。传闻我的和亲对象是个残疾皇子，可洞房花烛夜当晚...
茶点故事阅读 35,260评论 2赞 258

一些实用的重构代码技巧

目的

抛出代码的历史问题

可读性

Class Members

Method

public API name

可测试性

可拓展性

开始重构历史问题代码

可读性

Method

可测试性

可拓展性

Single Principle

Open/Close Principle

Lusiko substitution Principle

Interface segregation principle

Dependency inversion Princinle

其他可用的技巧

推荐阅读更多精彩内容