🪴 Obsidian Publish

代码坏味道 Bad-Smell

9分钟阅读

命名

命名不具有业务含义

命名不清晰

// 函数命名不清晰
public void processChapter(long chapterId) {
	Chapter chapter = this.repository.findByChapterId(chapterId);
	if (chapter == null) {
		throw new IllegalArgumentException("Unknown chapter [" + chapterId + "]");
	}
	chapter.setTranslationState(TranslationState.TRANSLATING);
	this.repository.save(chapter);
}

用技术术语命名

技术名词的出现，往往就代表着它缺少了一个应有的模型。
这里就只是获取一些书，从业务逻辑上，不需要关注是 List 还是 Set 形式。

// 下面的更好
List<Book> bookList = service.getBooks();
List<Book> books = service.getBooks();

这里就是需要一个缓存，不需要知道是 Redis 缓存还是本地缓存。

Book cachedBook = redisBookStore.get(isbn);
Book cachedBook = cache.get(isbn);

命名不具有英语语法

  • 违法语法规则
  • 不准确的英语词汇 —— 建立团队词汇表（名称/英文/说明）

函数

函数行数过长

Python、Ruby 等表达能力强的动态语言，函数代码尽量 5 行内。
Java 等表达能力弱等动态语言，函数代码尽量 10 行内。
若因为 bool 型的标记参数使得函数过长，应该拆分出来。
在 Java 中，我们就可以把代码行的约束加到 CheckStyle 的配置文件中，就像下面这样：

<module name="MethodLength">
	<property name="tokens" value="METHOD_DEF"/>
	<property name="max" value="20"/>
	<property name="countEmpty" value="false"/>
</module>

函数的参数过多

函数参数过多 & 变化频率相同时，可以考虑将其封装为一个类。
函数参数过多 & 变化频率不同时，静态不变的参数作为这个函数所在类的一个字段。
优化前

public void createBook(final String title,
					 final String introduction,
					 final URL coverUrl,
					 final BookType type,
					 final BookChannel channel,
					 final String protagonists,
					 final String tags,
					 final boolean completed) {
	...
	Book book = Book.builder
	  .title(title)
	  .introduction(introduction)
	  .coverUrl(coverUrl)
	  .type(type)
	  .channel(channel)
	  .protagonists(protagonists)
	  .tags(tags)
	  .completed(completed)
	  .build();
	this.repository.save(book);
}

优化后

public void createBook(final NewBookParamters parameters) {
	...
	Book book = parameters.newBook();
	this.repository.save(book);
}

类

类的字段过多

最容易产生大类的原因在于职责的不单一。
比如，将 User 变成 User、Author、Editor 三个类。
大类的产生往往还有一个常见的原因，就是字段未分组。
比如，将 User 中的 email、phone 属性拿出来作为 Contact 类。
后者有点类似于 DDD 中实体和值对象之间的关系。

对象的封装不够完善

对象的过长消息链

若存在对于对象的过长消息链，考虑是否封装的不够完善。

// 链式获取书籍的作者名字
String name = book.getAuthor().getName();
// 书籍的名字直接作为 Book 的某个方法，使用者无需关心 Book 内如何存储作者名字
String name = book.getAuthorName();

用基本类型表示对象

将对象直接用基本数据类型/字符串来替代。
对于下面的例子来说，如果使用价格的地方比较多，应该将其封装为一个对象，减少校验次数。

public double getEpubPrice(final boolean highQuality, final int chapterSequence) {
	...
	if (price <= 0) {
		throw new IllegalArgumentException("Price should be positive");
	}
}

这样封装为对象后，代码更简洁，后续基于价格的改动也更容易，如让价格在对外呈现时只有两位。

class Price {
	private long price;
	public Price(final double price) {
		if (price <= 0) {
			throw new IllegalArgumentException("Price should be positive");
		}
		this.price = price;
	}
}

注意对象的可变性

无业务含义的 setter 方法

如果可以不创建任何一个 setter 方法，而是创建有业务含义的方法。
这和“函数命名方式”类似，不同点在于，类的 setter 方法可能会导致不可控的变化。
例如下面的例子，有可能外部调用的时候没有按照规定传入参数，setter 中也没有进行输入检查。

public void approve(final long bookId) {
	...
	book.setReviewStatus(ReviewStatus.APPROVED);
	...
}

更合理的方法是下面这种，避免将不必要的内容暴露出来。

public void approve(final long bookId) {
...
book.approve();
...
}
 
class Book {
	public void approve() {
		this.reviewStatus = ReviewStatus.APPROVED;
	}
}

另一种消除可变性的方法就是，设置成不变类。
值对象就要设计成不变类，实体类则要限制数据变化。

class Book {
	public Book approve() {
		return new Book(..., ReviewStatus.APPROVED, ...);
	}
}

依赖继承

多使用组合/聚合的形式，而不是盲目使用继承来实现。

语句

声明和赋值相隔较远

变量声明和赋值相隔较远

在声明前面增加 final，用不变性的限制约束代码。此时，赋值时就会选择通过函数来进行。
优化前

EpubStatus status = null;
CreateEpubResponse response = createEpub(request);
if (response.getCode() == 201) {
	status = EpubStatus.CREATED;
} else {
	status = EpubStatus.TO_CREATE;
}

优化后

final CreateEpubResponse response = createEpub(request);
final EpubStatus status = toEpubStatus(response);
private EpubStatus toEpubStatus(final CreateEpubResponse response) {
	if (response.getCode() == 201) {
		return EpubStatus.CREATED;
	}
	return EpubStatus.TO_CREATE;
}

集合声明后未立刻添加元素

除此之外，对于集合来说，Java 可以 9 还新增了专门的工具类来进行初始化。

List<Permission> permissions = List.of(
	Permission.BOOK_READ,
	Permission.BOOK_WRITE
);

重复结构的代码

优化前

@Task
public void sendBook() {
    try {
        this.service.sendBook();
    } catch (Throwable t) {
        this.notification.send(new SendFailure(t)));
    throw t;
}
}
 
@Task
public void sendChapter() {
    try {
        this.service.sendChapter();
    } catch (Throwable t) {
        this.notification.send(new SendFailure(t)));
    throw t;
}
}
 
@Task
public void startTranslation() {
    try {
        this.service.startTranslation();
    } catch (Throwable t) {
        this.notification.send(new SendFailure(t)));
    throw t;
}
}

优化后

private void executeTask(final Runnable runnable) {
    try {
        runnable.run();
    } catch (Throwable t) {
        this.notification.send(new SendFailure(t)));
    throw t;
}
}
@Task
public void sendBook() {
    executeTask(this.service::sendBook);
}
 
@Task
public void sendChapter() {
    executeTask(this.service::sendChapter);
}
 
@Task
public void startTranslation() {
    executeTask(this.service::startTranslation);
}

控制语句嵌套过多

For 嵌套过多，应该将被迭代部分转换为函数。
If 嵌套过多，应该在不满足的情况时返回。
Else 层数过多，也应该在不满足的情况时返回。
Switch 复杂，考虑用多态来代替。
优化前

public void distributeEpubs(final long bookId) {
	List<Epub> epubs = this.getEpubsByBookId(bookId);
	for (Epub epub : epubs) {
		if (epub.isValid()) {
			boolean registered = this.registerIsbn(epub);
			if (registered) {
				this.sendEpub(epub);
			}
		}
	}
}

优化后

public void distributeEpubs(final long bookId) {
	List<Epub> epubs = this.getEpubsByBookId(bookId);
	for (Epub epub : epubs) {
		this.distributeEpub(epub)；
	}
}
private void distributeEpub(final Epub epub) {
	if (!epub.isValid()) {
		return;
	}
	boolean registered = this.registerIsbn(epub);
	if (!registered) {
		return;
	}
	this.sendEpub(epub);
}

使用不合理的全局变量

Java 类中 static 对象和语句块、Python 中的全局变量，这些都属于不容易测试的代码，最好不要轻易去写。

业务

依赖混乱

接口和业务之间缺少防腐层

缺少防腐层，会让请求对象传导到业务代码中，造成了业务与外部接口的耦合，也就是业务依赖了一个外部通信协议。一般来说，业务的稳定性要比外部接口高，这种反向的依赖就会让业务一直无法稳定下来，继而在日后带来更多的问题。解决方案自然就是引入一个防腐层，将业务和接口隔离开来。

业务代码中出现具体实现类

业务代码中出现具体的实现类，实际上是违反了依赖倒置原则。因为违反了依赖倒置原则，业务代码也就不可避免地受到具体实现的影响，也就造成了业务代码的不稳定。

参考链接

反向链接