这个例子展示了对一处比较隐蔽的坏味道的优化,可能一不小心就被放任自流了,好在业务本身和优化前后的代码都很简单,适合作为一个工程实例写出来给大家分享。
业务场景
写一个数据层的loader,loader的数据拉取策略是本地有就走本地否则走网络,所以需要通过回调来返回数据,而且两种情况在view层的表现不一样(一个显示加载动画另一个不显示),也需要通过回调来告知。
优化前
最简单的做法是写一个这样的接口,把所有可能要做的事情都各写一个方法,比如这样:
public interface XXLoadAction<T> {
void showLoadingView();
void hideLoadingView();
void onDataBack(T model);
}
这样写非常简单直接,而且能用,但至少存在这么几个问题:
- 没有错误处理 当有网络错误或者其他错误需要体现出来的时候,你只能通过扩展model来实现,而往往这种扩展会重复出现,这样便有可能造成大量重复代码
- 没有扩展性 当前需求是在网络请求开始的时候显示加载动画,网络请求结束时隐藏加载动画,但如果需求改变,需要在开始和结束的时候多做一些事情,你无法在这个action上进行扩展,只能把这些事情加在其他地方做,但这些事情本身跟
load action
联系非常紧密,这样就会影响代码的内聚性 - 耦合程度太高 loader是model层的东西,如果在
load action
中还有对view层直接的操作,知道也太多了
于是我设计了两个接口来解决这些问题
- 耦合程度太高 抽象出耗时的概念,使用两个接口来分别处理长时间和短时间的请求,model层通过判断是否需要进行网络请求来选择
ShortLoadAction
还是LongLoadAction
,而不关心你在回调中要对view做什么操作 - 没有错误处理 、没有扩展性 对于每个接口,都像网络请求一样写了4个回调,把跟具体实现有关的东西移除了,提高了基础组件的可扩展性,同时增加常用错误处理回调和自定义错误处理回调
public interface ShortLoadAction<T> {
void onStartShort();
void onCancelShort();
void onBrokenShort();
void onCompleteShort(int stateCode, String stateMsg, T model);
}
public interface LongLoadAction<T> {
void onStartLong();
void onCancelLong();
void onBrokenLong();
void onCompleteLong(int stateCode, String stateMsg, T model);
}
你会发现这两个接口方法类似但名字不一样,因为我接下来写了一个适配器,结合目前简单的业务情况,为这两个action做了一层封装,方便view层或presenter层使用,这个适配器需要实现上面两个接口,如果两个接口方法名一样就乱套了,所以虽然这样有点怪,但也是没有办法
public abstract class LoadAction<T> implements ShortLoadAction<T>, LongLoadAction<T> {
@Override
public void onStartShort() {
// do nothing
}
@Override
public void onCancelShort() {
onFailed();
}
@Override
public void onBrokenShort() {
onFailed();
}
@Override
public void onCompleteShort(int stateCode, String stateMsg, T model) {
onSuccess(model);
}
/*-------------------------------------------------*/
@Override
public void onStartLong() {
showLoadingView();
}
@Override
public void onCancelLong() {
hideLoadingView();
onFailed();
}
@Override
public void onBrokenLong() {
hideLoadingView();
onFailed();
}
@Override
public void onCompleteLong(int stateCode, String stateMsg, T model) {
hideLoadingView();
onSuccess(model);
}
/*--------------------------------------------------------------------------------------------*/
public abstract void showLoadingView();
public abstract void hideLoadingView();
public abstract void onFailed();
public abstract void onSuccess(T model);
}
有了这个抽象类,使用者只管实现加载动画的显示隐藏、处理成功失败就行了,把扩展性强但略显复杂的四个回调以及耗时长短的区分都隐藏起来不需要去关心。
为了进一步方便使用,我还通过ViewHelper
(可以把它当成加载动画的实现类)写了一个用起来更简单的adapter,这下使用者只需要关心成功失败了:
public abstract class VHelperLoadAction<T> extends LoadAction<T> {
private ViewHelper viewHelper;
public VHelperLoadAction(ViewHelper viewHelper) {
this.viewHelper = viewHelper;
}
@Override
public void showLoadingView() {
viewHelper.showLoadingView();
}
@Override
public void hideLoadingView() {
viewHelper.hideLoadingView();
}
}
写完一用感觉很不错,完全没有感觉不对或者是不符合设计模式什么的,ShortLoadAction
和LongLoadAction
方法名的奇怪感觉早就忘得一干二净。比如这是一处使用:
VHelperLoadAction<List<WordPage>> loadAction = new VHelperLoadAction<List<WordPage>>(vHelper) {
@Override
public void onFailed() {
vHelper.toastNetworkBroken();
}
@Override
public void onSuccess(List<WordPage> model) {
CheckWordsActivity.start(getActivity(), model);
}
};
pageWordsLoader.load(pages, loadAction, loadAction);
这个loader的主要load方法长这样:
public void load(List<String> pageIds, ShortLoadAction<List<WordPage>> loadShortAction, LongLoadAction<List<WordPage>> loadLongAction) {
this.pageIds = pageIds;
dbHelper.createTableIfNotExist();
List<String> unsavedPageNumbers = getUnSavedPageNumbers();
if (unsavedPageNumbers.isEmpty()) {
loadWordsFromLocal(loadShortAction, null);
} else {
loadWordsFromRemote(unsavedPageNumbers, loadLongAction);
}
}
分析
这个代码还给别人讲过,讲完都没觉得有问题。直到一次codereview中,我刚跟老大解释完这几个action
的关系,老大就表示这块写得不太好。
开始他是觉得后边这个load()
方法中需要传两个action
进去非常奇怪,但我知道这块没有问题,因为loader
的使用者是不知道也不应该关心到底选择哪种action
,loader
应该自己去看本地有没有数据,然后选择short或者long的action。大家都同意这个观点。
但还有一个同事认为只要把两个action合成一个action一切就完美了。给每个回调方法增加一个参数:
isLong
,回调方法中就可以根据不同的参数来做不同的事,只需要传入一个action,也没有方法名啰嗦的问题。
但其实这个设计问题很多,至少有这两个:
- if else爆炸 本来只需要在上面那个load方法中做一次if else,在使用这种设计后你需要有几个回调方法就加两倍于此的if else,回调的调用处一次,回调的被调处一次
- 不便于扩展 假如有第三种action你是准备再加一种参数吗?
然后我们老大就指出了这部分代码的最大问题,在于ShortLoadAction
和LongLoadAction
俩个明显有相似关系的东西居然没有一点关系,是两个完全不同的接口;同时,在loader的最里层(关心选择short或者long的action的里层),本来可以不关心具体是short还是long的action,现在却直接关心了( 持有action的类型是ShortLoadAction
或LongLoadAction
:
loadWordsFromRemote(List<String> unsavedPageNumbers, LongLoadAction<List<WordPage>> loadLongAction)
)
这个问题在被提出之后变得异常刺眼,但好像很好解决。让他们实现自同一个接口就好了,但由于跟前面同样的原因,好用的适配器LoadAction
写不出来。虽然这只是个使用上的方便性问题,应该给设计的合理性让位,但两全其美还是最好的。
通过观察发现LongLoadAction
其实是在ShortLoadAction
的基础上进行了一些扩展,那么就可以使用继承,让ShortLoadAction
成为LongLoadAction
的父类,这样写用起来也不会太麻烦。但只是这个原因并不足以让我们使用继承,由于众所周知的原因,能使用组合的地方就不要用继承,那么对于这个场景,基于组合的装饰模式就是最为合适的了,在使用的方便性和设计的合理性上都有很好的表现。
优化后
装饰模式本身是实现起来非常简单的一个模式,这里我只需要写一个接口同时作为被装饰类和一个装饰类就OK了
public interface LoadAction<T> {
void onStart();
void onCancel();
void onBroken();
void onComplete(int stateCode, String stateMsg, T model);
}
public abstract class WaitLoadAction<T> implements LoadAction<T> {
private LoadAction<T> loadAction;
private WaitLoadAction() {
}
public WaitLoadAction(LoadAction<T> loadAction) {
this.loadAction = loadAction;
}
@Override
public void onStart() {
showLoadingView();
loadAction.onStart();
}
@Override
public void onCancel() {
hideLoadingView();
loadAction.onCancel();
}
@Override
public void onBroken() {
hideLoadingView();
loadAction.onBroken();
}
@Override
public void onComplete(int stateCode, String stateMsg, T model) {
hideLoadingView();
loadAction.onComplete(stateCode, stateMsg, model);
}
/*--------------------------------------------------------------------------------------------*/
public abstract void showLoadingView();
public abstract void hideLoadingView();
}
自然地我用ViewHelper
简化了WaitLoadAction
public class VHelperWaitLoadAction<T> extends WaitLoadAction<T> {
private ViewHelper viewHelper;
public VHelperWaitLoadAction(LoadAction<T> loadAction, ViewHelper viewHelper) {
super(loadAction);
this.viewHelper = viewHelper;
}
@Override
public void showLoadingView() {
viewHelper.showLoadingView();
}
@Override
public void hideLoadingView() {
viewHelper.hideLoadingView();
}
}
这样写完感觉用的时候还是不很方便,我还得把LoadAction
做一个封装
public abstract class SimpleLoadAction<T> implements LoadAction<T> {
@Override
public void onStart() {
// do nothing
}
@Override
public void onCancel() {
onFailed();
}
@Override
public void onBroken() {
onFailed();
}
@Override
public void onComplete(int stateCode, String stateMsg, T model) {
onSuccess(model);
}
/*--------------------------------------------------------------------------------------------*/
public abstract void onFailed();
public abstract void onSuccess(T model);
}
于是用的时候是这样,一样好用,还会更清晰,而且最初那个方法命名啰嗦的问题也没有了:
LoadAction<List<WordPage>> loadAction = new SimpleLoadAction<List<WordPage>>() {
@Override
public void onFailed() {
vHelper.toastNetworkBroken();
}
@Override
public void onSuccess(List<WordPage> model) {
CheckWordsActivity.start(getActivity(), model);
}
};
pageWordsLoader.load(pages, loadAction, new VHelperWaitLoadAction<>(loadAction, vHelper));
总结
回过头来看,优化前后虽然都是4个类,但显然优化后减少了重复的代码,用这些省下来的空间分离了耦合,还进一步让整个架构变得更加灵活,是一次比较成功的优化。
…
但后来发现我们的适配设备(是个专门给某个pad开发并且烧录进去一块出售的软件)性能实在捉急,以至于在本地取数据花的时间也不短,所以需要跟网络请求一样显示加载动画,于是最后我只用了一个VHelperWaitLoadAction
(摊手)