我的同事向我展示了这段代码，我们都想知道为什么我们似乎无法删除重复的代码。
private List<Foo> parseResponse(Response<ByteString> response) {
if (response.status().code() != Status.OK.code() || !response.payload().isPresent()) {
if (response.status().code() != Status.NOT_FOUND.code() || !response.payload().isPresent()) {
LOG.error("Cannot fetch recently played, got status code {}", response.status());
}
return Lists.newArrayList();
}
// ...
// ...
// ...
doSomeLogic();
// ...
// ...
// ...
return someOtherList;
}
这是一个替代表示，使它不那么罗嗦：
private void f() {
if (S != 200 || !P) {
if (S != 404 || !P) {
Log();
}
return;
}
// ...
// ...
// ...
doSomeLogic();
// ...
// ...
// ...
return;
}
有没有更简单的方法来写这个，而不重复!P？如果没有，是否有一些关于情况或条件的独特属性，使得无法分解!P？
它看起来像很多代码的一个原因是它非常重复。使用变量来存储重复的部分，这将有助于提高可读性：
private List<Foo> parseResponse(Response<ByteString> response) {
Status status = response.status();
int code = status.code();
boolean payloadAbsent = !response.payload().isPresent();
if (code != Status.OK.code() || payloadAbsent) {
if (code != Status.NOT_FOUND.code() || payloadAbsent) {
LOG.error("Cannot fetch recently played, got status code {}", status);
}
return Lists.newArrayList();
}
// ...
// ...
// ...
return someOtherList;
}
编辑：正如斯图尔特在下面的评论中指出的，如果可以直接比较response.status()和Status.OK，那么你可以消除对.code()的无关调用并使用import static直接访问状态：
import static Namespace.Namespace.Status;
// ...
private List<Foo> parseResponse(Response<ByteString> response) {
Status status = response.status();
boolean payloadAbsent = !response.payload().isPresent();
if (status != OK || payloadAbsent) {
if (status!= NOT_FOUND || payloadAbsent) {
LOG.error("Cannot fetch recently played, got status code {}", status);
}
return Lists.newArrayList();
}
// ...
// ...
// ...
return someOtherList;
}
关于如何处理payloadAbsent逻辑重复的问题，Zachary提供了一些与我建议的内容兼容的好主意。还有一个选择是保留基本结构，但要更明确地检查有效负载。这将使逻辑更容易遵循，并使您不必在内部||中使用if。 OTOH，我自己并不热衷于这种方法：
import static Namespace.Namespace.Status;
// ...
private List<Foo> parseResponse(Response<ByteString> response) {
Status status = response.status();
boolean failedRequest = status != OK;
boolean loggableError = failedRequest && status!= NOT_FOUND ||
!response.payload().isPresent();
if (loggableError) {
LOG.error("Cannot fetch recently played, got status code {}", status);
}
if (failedRequest || loggableError) {
return Lists.newArrayList();
}
// ...
// ...
// ...
return someOtherList;
}
免责声明：我不会质疑所提供功能的签名，也不会质疑功能。
对我来说，这感觉很尴尬，因为这个功能本身就要做很多工作，而不是委托它。
在这种情况下，我建议吊起验证部分：
// Returns empty if valid, or a List if invalid.
private Optional<List<Foo>> validateResponse(Response<ByteString> response) {
var status = response.status();
if (status.code() != Status.NOT_FOUND.code() || !response.payload().isPresent()) {
LOG.error("Cannot fetch recently played, got status code {}", status);
return Optional.of(Lists.newArrayList());
}
if (status.code() != Status.OK.code()) {
return Optional.of(Lists.newArrayList());
}
return Optional.empty();
}
请注意，我更喜欢重复return语句而不是嵌套条件。这使代码保持平坦，降低了圈复杂度。此外，无法保证您始终希望为所有错误代码返回相同的结果。
之后，parseResponse变得容易：
private List<Foo> parseResponse(Response<ByteString> response) {
var error = validateResponse(response);
if (error.isPresent()) {
return error.get();
}
// ...
// ...
// ...
return someOtherList;
}
相反，您可以使用功能样式。
/// Returns an instance of ... if valid.
private Optional<...> isValid(Response<ByteString> response) {
var status = response.status();
if (status.code() != Status.NOT_FOUND.code() || !response.payload().isPresent()) {
LOG.error("Cannot fetch recently played, got status code {}", status);
return Optional.empty();
}
if (status.code() != Status.OK.code()) {
return Optional.empty();
}
return Optional.of(...);
}
private List<Foo> parseResponse(Response<ByteString> response) {
return isValid(response)
.map((...) -> {
// ...
// ...
// ...
return someOtherList;
})
.orElse(Lists.newArrayList());
}
虽然我个人觉得额外的筑巢有点烦人。
我认为以下内容是等效的。但正如其他人所指出的那样，代码透明度可能比“简单”代码更重要。
if (not ({200,404}.contains(S) && P)){
log();
return;
}
if (S !=200){
return;
}
// other stuff
您可以拥有一组要记录的代码以及有效负载的常见条件
Set<Code> codes = {200, 404};
if(!codes.contains(S) && !P){
log();
}
return new ArrayList<>();
条件更正。
通过将它与一组有限的显式值进行比较来对整数进行分支最好由switch处理：
if (P) {
switch (S) {
case 200: return B;
case 404: return A;
}
}
Log();
return A;
只需使用像JLRishe的答案那样的变量。但我认为代码清晰度远比重复简单的布尔检查更重要。您可以使用早期的return语句来使它更清晰：
private List<Foo> parseResponse(Response<ByteString> response) {
if (response.status().code() == Status.NOT_FOUND.code() && !response.payload().isPresent()) // valid state, return empty list
return Lists.newArrayList();
if (response.status().code() != Status.OK.code()) // status code says something went wrong
{
LOG.error("Cannot fetch recently played, got status code {}", response.status());
return Lists.newArrayList();
}
if (!response.payload().isPresent()) // we have an OK status code, but no payload! should this even be possible?
{
LOG.error("Cannot fetch recently played, got status code {}, but payload is not present!", response.status());
return Lists.newArrayList();
}
// ... got ok and payload! do stuff!
return someOtherList;
}
可以将重复的P检验分解出来。以下（伪代码）在逻辑上等同于您问题中的代码。
private List<Foo> f() {
List<Foo> list(); /*whatever construction*/
if (P) {
if (S==200) {
// ...
// ...
// ...
list.update(/*whatever*/);
}
else if (S!=404) {
Log();
}
}
else {
Log();
}
return list;
}
在可读性方面，我将使用以下（再次伪代码）：
private bool write_log() {
return (S!=200 && S!=404) || !P
}
private bool is_good_response() {
return S==200 && P
}
private List<Foo> f() {
List<Foo> list(); /*whatever construction*/
if (write_log()) {
Log();
}
if (is_good_response()) {
// ...
// ...
// ...
list.update(/*whatever*/);
}
return list;
}
可能更适合命名的功能。
有了这些条件，我认为没有办法解决一些重复问题。但是，我更愿意在必要时将条件与合理分开并复制其他区域。
如果我写这个，保持当前的风格，它将是这样的：
private void f() {
if(!P) {
Log(); // duplicating Log() & return but keeping conditions separate
return;
} else if (S != 200) {
if (S != 404) {
Log();
}
return;
}
// ...
// ...
// ...
return;
}
代码的简单性具有一些主观元素，可读性非常主观。鉴于此，如果我要从头开始编写这个方法，这就是我的偏见。
private static final String ERR_TAG = "Cannot fetch recently played, got status code {}";
private List<Foo> parseResponse(Response<ByteString> response) {
List<Foo> list = Lists.newArrayList();
// similar to @JLRishe keep local variables rather than fetch a duplicate value multiple times
Status status = response.status();
int statusCode = status.code();
boolean hasPayload = response.payload().isPresent();
if(!hasPayload) {
// If we have a major error that stomps on the rest of the party no matter
// anything else, take care of it 1st.
LOG.error(ERR_TAG, status);
} else if (statusCode == Status.OK.code()){
// Now, let's celebrate our successes early.
// Especially in this case where success is narrowly defined (1 status code)
// ...
// ...
// ...
list = someOtherList;
} else {
// Now we're just left with the normal, everyday failures.
// Log them if we can
if(statusCode != Status.NOT_FOUND.code()) {
LOG.error(ERR_TAG, status);
}
}
return list; // One of my biases is trying to keep 1 return statement
// It was fairly easy here.
// I won't jump through too many hoops to do it though.
}
如果我删除我的评论，这仍然几乎使代码行加倍。有些人认为这不能使代码更简单。对我来说，确实如此。
我不确定代码试图做什么：老实说，只记录404状态并在代码不是200时返回一个空列表感觉就像你试图避免NPE ...
我认为这样的方式更好：
private boolean isResponseValid(Response<ByteString> response){
if(response == null){
LOG.error("Invalid reponse!");
return false;
}
if(response.status().code() != Status.OK.code()){
LOG.error("Invalid status: {}", response.status());
return false;
}
if(!response.payload().isPresent()){
LOG.error("No payload found for response!");
return false;
}
return true;
}
private List<Foo> parseResponse(Response<ByteString> response) throws InvalidResponseException{
if(!isResponseValid(response)){
throw InvalidResponseException("Response is not OK!");
}
// logic
}
如果由于任何原因if逻辑无法更改，我将无论如何将验证移动到单独的函数中。
另外，尝试使用Java命名约定：
LOG.error("") // should be log.error("")
Status.OK.code() // why couldn't it be a constant like Status.OK, or Status.getOkCode()?
如果您想澄清条件检查并保持相同的逻辑结果，则以下内容可能是合适的。
if (!P) {
Log();
return A;
}
if (S != 200) {
if (S != 404) {
Log();
}
return A;
}
return B;
或者（这是OP首选）
if (S == 404 && P) {
return A;
}
if (S != 200 || !P) {
Log();
return A;
}
return B;
或者（我个人更喜欢这个，如果你不介意开关）
if (P) {
switch (S) {
case 200: return B;
case 404: return A;
}
}
Log ();
return A;
您可以通过删除大括号并将单行主体移动到与if语句相同的行来压缩if语句。但是，单行if语句可能会令人困惑，也可能是不正确的不良行为。根据我从评论中收集到的内容，您的偏好将不利于此用途。虽然单行if语句可以压缩逻辑并使代码更清晰，但清晰度和代码意图应该被重视为“经济”代码。需要明确的是：我个人认为有些情况下单行if语句是合适的，但是，由于原始条件很长，在这种情况下我强烈建议不要这样做。
if (S != 200 || !P) {
if (S != 404 || !P) Log();
return A;
}
return B;
作为辅助节点：如果Log();语句是嵌套if语句中唯一可到达的分支，则可以使用以下逻辑标识来压缩逻辑（Distributive）。
(S != 200 || !P) && (S! = 404 || !P) <=> (S != 200 && S != 404) || !P
编辑重要编辑以重新排列内容并解决评论中提到的问题。
请记住，简洁并不总是最好的解决方案，在大多数情况下，编译器会优化本地命名变量。
我可能会用：
boolean goodPayload = response.status().code() == Status.OK.code() && response.payload().isPresent();
if (!goodPayload) {
// Log it if payload not found error or no payload at all.
boolean logIt = response.status().code() == Status.NOT_FOUND.code()
|| !response.payload().isPresent();
if (logIt) {
LOG.error("Cannot fetch recently played, got status code {}", response.status());
}
// Use empty.
return Lists.newArrayList();
}
代码闻到我的味道是你要求Response对象而不是告诉它。问问自己为什么Parse方法在Response对象的外部而不是它的方法（或者更可能是它的超类）。是否可以在Response对象构造函数中调用Log（）方法而不是其Parse方法？在构造函数中计算属性status().code()和payload().isPresent()时，是否可以将默认解析对象分配给私有属性，以便只有简单（和单个）if ... else ...保留在Parse（）中？
如果有人能够使用实现继承来编写面向对象的语言，那么应该查询每个条件语句（和表达式！），以查看它是否被提升到构造函数或方法中（s） ）调用构造函数。所有课堂设计的简化都是非常重要的。
实际上多余的主要是！P（有效载荷存在）。如果你把它写成一个布尔表达式，你有：
(A || !P) && (B || !P)
正如你所观察到的，！P似乎是重复的，而且它是不必要的。在布尔代数中，您可以将AND处理为类似乘法，而OR类似于加法。因此，您可以像使用简单代数一样将这些括号扩展为：
A && B || A && !P || !P && B || !P && !P
您可以将所有具有！P的ANDed表达式组合在一起：
A && B || (A && !P || !P && B || !P && !P)
由于所有这些术语都有！P，你可以像乘法一样把它拿出来。当你这样做时，你将它替换为true（就像你1那样，因为任何东西本身都是1次，真实且任何东西本身）：
A && B || !P && (A && true || B && true || true && true)
请注意，“true && true”是OR表达式之一，因此整个分组始终为true，您可以简化为：
A && B || !P && true
-> A && B || !P
也许，我在这里用正确的符号和术语生锈了。但这就是它的要点。
回到代码，如果你在if语句中有一些复杂的表达式，就像其他人已经注意到的那样你应该把它们放在一个有意义的变量中，即使你只是要使用它一次并扔掉它。
所以把这些放在一起你会得到：
boolean statusIsGood = response.status().code() != Status.OK.code()
&& response.status().code() != Status.NOT_FOUND.code();
if (!statusIsGood || !response.payload().isPresent()) {
log();
}
请注意，即使您将其否定，该变量也会被命名为“statusIsGood”。因为带有否定名称的变量确实令人困惑。
请记住，您可以对真正复杂的逻辑进行上述简化，但您不应该总是这样做。你最终会得到技术上正确的表达，但没有人能通过观察它来说明原因。
在这种情况下，我认为简化澄清了意图。
恕我直言，问题主要是重复和嵌套。其他人建议使用我推荐的clear变量和util函数，但您也可以尝试分离验证的问题。
如果我错了，请纠正我，但似乎您的代码在实际处理响应之前尝试验证，因此这是编写验证的另一种方法：
private List<Foo> parseResponse(Response<ByteString> response)
{
if (!response.payload.isPresent()) {
LOG.error("Response payload not present");
return Lists.newArrayList();
}
Status status = response.status();
if (status != Status.OK || status != Status.NOT_FOUND) {
LOG.error("Cannot fetch recently played, got status code {}", status);
return Lists.newArrayList();
}
// ...
// ...
// ...
return someOtherList;
}
您可以反转if语句以使其更清晰，如：
private void f() {
if (S == 200 && P) {
return;
}
if (S != 404 || !P) {
Log();
}
return;
}
然后，您可以使用有意义的方法名称（例如“responseIsValid（）”和“responseIsInvalid（）”重构条件。
辅助函数可以简化嵌套条件。
private List<Foo> parseResponse(Response<ByteString> response) {
if (!isGoodResponse(response)) {
return handleBadResponse(response);
}
// ...
// ...
// ...
return someOtherList;
}
最尴尬的部分是像response.status()这样的吸气剂，当逻辑似乎需要一个单一的，一致的值时被多次调用。据推测它是有效的，因为getter保证总是返回相同的值，但是它会严重地表达代码的意图并使其对当前的假设更加脆弱。
要解决这个问题，代码应该获得response.status()一次，
var responseStatus = response.status();
，然后再使用responseStatus。对于其他getter值，应该重复此操作，这些值在每次获取时都假定为相同的值。
此外，如果此代码稍后可以在更动态的上下文中重构为线程安全实现，则理想情况下，所有这些获取都将在相同的顺序点完成。要点是，您的意思是在某个特定时间点获取response的值，因此代码的关键部分应该在一个同步过程中获得这些值。
通常，正确指定预期的数据流会使代码更具弹性和可维护性。然后，如果某人需要为getter添加副作用或使response成为抽象数据类型，则更有可能继续按预期工作。