记一次Lombok的Setter重载方法造成的事故及思考-CSDN博客

前言

Lombok是很多Java开发者会用到的一个非常方便的Java库。在lombok的帮助下，开发者将更加集中于业务逻辑的开发，而不受重复的，无聊的，非业务逻辑代码书写的影响。一个比较明显的例子就是我们不需要再手动为每一个类成员变量写Setter和Getter方法。

问题

Lombok在为我们带来便利的同时，不仅解放了开发者的双手，一定程度上也让开发者忘记了思考到底Lombok为我们自动生成了什么代码。如果不深究Lombok的自动代码生成原理，则会让我们忽略掉很多Java基础知识。

先看下面一段代码，请问request为”/hello”的时，我们所得到的Response的body是什么？

@Getter
@Setter
abstract class A {
	private boolean success;
}

@Setter
@Getter
class B extends  A {
	public Boolean success;

}


@Controller
@RequestMapping("/hello")
public class Hello {

	@GetMapping
	@ResponseBody
	public B returnB() {
		B b = new B();
		b.setSuccess(true);
		return b;
	}
}

复制代码

答案是：

body = {success: null}
复制代码

原因

其实原理很简单，但是很容易被我们忽略掉。我问过几个老Java开发，他们虽然知道其中的原理，但是在一开始问他们为什么success的值为null的时候，都没有人能立刻想到原因。

其实造成success的值为null的原因不是Lombok, Lombok生成的Setter重载了父类的方法。

首先，我们想象一下，如果我们不使用Lombok的Setter和Getter的话，我们自己也许会像下面手动写Getter和Setter。

class A {
  public boolean success;

  public void setSuccess(boolean success) {
  	this.success = success;
  }
  public boolean getSuccess() {
    return this.success;
  }
}

class B extends A {
  public Boolean success;

  public void setSuccess(Boolean success) {
  	this.success = success;
  }
  @Override
  public boolean getSuccess() {
    return this.success;
  }
}

复制代码

上面的代码， B的setSuccess重载了A的setSuccess,同时，B的getSuccess重写了A.getSuccess.

由于B的setSuccess重载了A的setSuccess.所以B的对象里面就会有两个方法，Signature分别是:

void setSuccess(boolean success), 
void setSuccess(Boolean success)
复制代码

所以当我们使用b.setSuccess(true)时，这里不是设置的b.success（Boolean）的值，而是设置的b.A.success(boolean)的值，同时，b.A.success是一个隐藏值，所以对于外部来说，根本看不见b.A.success的值。如下图所示。

而此时b.success的值则为初始值null。要想设置b.success的值的话，则需要使用b.setSuccess(Boolean.True)

如果这个setSuccess是自己去写的话，我们可以能注意到子类和父类的入参类型的变化，而加以注意，不至于造成选错setSuccess方法，而导致功能行为与预想的不一致。

事故模拟

在我们的Legacy代码里面，有一个父类，这个父类里面有一个成员变量和一个方法。如下：

@Setter
@Getter
class A {
  private boolean success;

  public void doSomethingAndSetSuccess() {
		// dosomething
     this.setSuccess(true);
  }
}
复制代码

同时，在Legeacy代码里面，还有一个子类继承了这个父类，如下：

class B extends A {
  // something not important
}
复制代码

然后，在这个在这个Legacy系统里面，有对B的对象调用。如下：

B b = new B();
b.doSomethingAndSetSuccess();

// something here
return b;  (通过@ResponseBody)
复制代码

这样的结构之前一直运行得很好，所返回的json结果一直都是我们想要的结果，比如{success: true(or false)}。直到有一天，有一个开发没有注意到他们之间的关系。在类B上加入了如下代码：

@Getter
@Setter
class B extends A {
  public Boolean success = null;

  // the last code same as before.
}
复制代码

Lombok的Setter，重载了A类里面的setSuccess(boolean success)，进而，在不修改A.doSomethingAndSetSuccess里面的setSuccess方法的话，则永远没有地方调用B的setSuccess(Boolean success)。也就是说B的success永远是初始值null.

事故反思与预防

到底类的成员变量使用封装的数据类型还是使用原始数据类型？

从上面的事故中，我们会容易得出一个结论，就是在子类中新添加的成员变量数据类型和父类不同造成了事故，亦即开发规范不同引起了意外。

那么，为了团队以后不再出现类似的类成员变量类型不一致的情况，我们到底应该是选择像父类那样，使用原始变量类型，还是使用包装类型呢？

先来看看阿里巴巴开发手册（下手册）上是怎么说的吧。

手册上举了两个例子来论证类属性强制使用包装数据类型。总结其论点就是 1. 包装数据类型的RPC返回值可以是null，代示其“不存在”。 2. 因为类属性是引用类型，就算直接赋值null，不会有NPE风险。

这两个论据很好理解，我也表示赞同。有一点需要补充的是，我认为比起返回原始数据的包装数据类型（Integer,Boolean,etc），返回一个自定义的数据类型会更加有语意。

假设你有一个RPC类A，代码如下：

// 返回原始数据类型
class A {
  public int a;

  public int getA(){
    // do some logic.
    return this.a;
  }
}
// code 1
复制代码

class A {
	public Integer a;

	public Integer getA(){
		// do some logic
		return this.a;
	}
}
//code2
复制代码

正如手册所说，code 2比起code 1，能够多返回一个null。因此也能告诉调用者你想调用的值根本不存在。

但是，我认为作为一个开发，当你知道当前所要返回的值将会是 -x，0，x和不存在（姑且叫null），虽然返回Integer是一个不错的解决方案，但我觉得从语意的角度上来说，还是不够好。毕竟null不是一个Integer，它只是一个空的Integer对象的引用。

所以我觉得在这种情况，最有语意的做法是自定义一个新的数据结构。

@Setter
@Getter
class SomeResult {
	// 使用这种编码方式，可以不用每一个POJO类属性都为包装数据类型。只需要保证RPC类属性为自定义数据类型即可。
	private boolean exist = true;
	private int value = 0;
}

// 可以再把SomeResult再抽象，以减少重复代码。这里不再列出。

class A {
  public SomeResult someResult;

  public SomeResult returnSomeResult() {
		// after doing some logic, return result
		// 我觉得这里，作为程序员，不应该主动返回null. 返回null一时爽，语意不清，造成后续Debug难度增加。
		// 关于返回null的博客，详细请移步  [https://juejin.im/post/5ba88342e51d450e6475f7cd](https://juejin.im/post/5ba88342e51d450e6475f7cd) 
		if(someResult exists) {
			return someResult;
		} else {
			someResult.setExist(fasle);
			return someResult;
		}
  }

}

class B {
  public void someMethod() {

		SomeResult someResult = getSomeResultBySomeService();
		
		// 作为调用者，确实有责任和义务确认RPC返回值是否为null。null在语意上来说这是一个Exception.不应该参与业务逻辑的意外。
		// 使用这种自定义数据结构，明确地表明是调用出了问题还是本身被调用者系统出了问题。
		if(someResult == null) {
			// 代表调用失败。
		} else if (!someResult.isExist) {
			// 调用虽然成功，但是所得到的a不存在。
		} else {
			// 调用成功，使用其值。
			a.value
		}

		// 我觉得更优美的写法应该是
		try {
			if(!someResult.isExist){}
			else {}
		} catch(Exception e) {
			// 处理以NPE为代表的Exception.
		}

	}
}

复制代码

除了遵循相同的编码规范和最佳实践，我们还能够如何保证代码上线更加安全。

人是会犯错的。我认为光是强调规范并不能保证开发者一定100%遵守规范。同时，我也不认为这些规范就能覆盖所有的开发质量问题。所以我认为还应该有更多的机制保证上线代码的质量。

TDD（Test-Driven Development）就是一个非常好的开发方法论。根据我自己对TDD的实践，我觉得TDD的最大好处就是让开发者知道自己正在做什么。

拿上面的例子来说，如果应用了TDD，开发者在代码里面所添加的任何一行代码都会得到Test-Case的检验。而且开发者会在编写业务逻辑代码之前，认真思考我为什么会加入这一行代码，这一行代码对我的逻辑有什么作用。

这样，就不至于会无脑地添加一些所谓的最佳实践却对业务逻辑没有帮助的代码。

比如上面的例子，当我们的Legacy系统有类A与其子类B。当前运行良好，我们假定当前的代码都是经过Test过的。

那么Legacy的当前代码snapshot如下：


@Setter
@Getter
class A {
  private boolean success;

  public void doSomeLogicAndSetSuccess() {
    // after doing some logic
    this.setSuccess(true);
  }
}

class B extends A {
}

class AUseCaseClass() {
	private B b = getBFromSomeService();

	public B doSomeLogicAndReturnB() {
		//after doing some logic 
		this.b.doSomeLogicAndSetSuccess();
		return this.b;
	}
}
//code 3
复制代码

这时有个开发因为有新的业务需求要修改code 3的代码，如果他是遵守TDD的话，他应该第一时间根据新的需求着手写Test-Case。根据笔者自身的实践经验，如果对新的需求了解的越是不明白，越是写不出Test-Case来，就更别说在原有代码上做改动了。就是这让开发者思考业务需求的过程和强制先写Test-Case的要求，让其每一行写入代码中的代码都不会成为废代码和错误代码。

比如上面的事故，要是遵守TDD的话，这个开发者在添加如下代码之前,他至少会问自己，我为什么添加success,难道A中没有success吗。同时，假如他没有发现A中有success,他也会首先写Test-Case，去验证自己要添加的这一行是否会让整个系统出现问题。

class B extends A {
  public Boolean success = null;
}
复制代码

结论

本文分析了笔者在工作中发现的事故，详细描述了事故发生的起因，经过和结果；同时，根据事故的根本原因，从团队未来预防同样错误的角度和提升团队整体代码质量的角度进行了深入的思考。笔者提出两个反思结果：1. 根据需要，类属性使用自定义数据类型（RPC方法返回值强制使用自定义数据类型）。2.使用TDD来保证代码质量（最好是做code-review的代码带上其Test-Case，否则其实和裸奔没有什么区别）。