【问题标题】:Suggested refactoring for if-statement of the following form?建议重构以下形式的 if 语句?
【发布时间】:2019-04-09 14:21:26
【问题描述】:

我正在尝试使其尽可能通用。假设在 if 语句中,我正在检查某个布尔表达式 A 是否为真。假设 A 为真,A.a 和 A.b 存在特定情况,它们是互斥的。另一个布尔表达式 B 也是如此。然后,考虑以下代码:

if (A) {
  if (A.a) {
    somethingSpecificToAa();
    foo();
  }
} else if (B) {
  if (B.a) {
    somethingSpecificToBa();
    foo();
  }
} else {
    foo();
}

在我的实际代码中,foo() 不是单个函数,而是多行长代码。重复这么多次对我来说似乎很臭,所以我认为需要进行一些重构。

由于foo() 在以下情况下执行:

  • A.a 是真的
  • B.a 是真的
  • A 和 B 都不正确

我想到了以下几点:

if (A.a) {
  somethingSpecificToAa();
} else if (B.a) {
  somethingSpecificToBa();
}

if (A.a || B.a || !(A || B)) {
  foo();
}

应该具有相同的行为。这是最好的方法吗?请注意,第二个示例的第二个 if 语句中的条件实际上最终会非常长,这就是为什么我的代码仍然看起来像第一个示例(我讨厌将单个 if 分成几行。)我也有考虑过制作一个返回 bool 的 lambda,它等效于 A.a || B.a || !(A || B),并将 lambda 插入到第二个 if 语句中。或者,我可以保留第一个示例的结构,但将每个 foo() 的许多行替换为具有相同作用的 (void) lambda,尽管我不确定这是否能解决异味。

此时我是否过度设计,考虑 lambdas?哪种方法最适合维护干净的代码?

编辑:似乎我已经把它通用了。我正在处理 STL 容器,而不是我自己的类,更“准确”的示例是:

int shirtACleanliness = calculateCleanliness(shirtA);
if (itemsToWash.contains(shirtA)) { //itemsToWash is a std::set
  if (shirtA.cleanliness > shirtACleanliness) {
    itemsToWash.erase(shirtA);
    shirtA.cleanliness = shirtACleanliness;
    itemsToWash.insert(shirtA); //the set is ordered on cleanliness, so this re-inserts in the correct position
    doSomeOtherStuff(shirtA);
  }
} else if (itemsToDry.contains(shirtA)) { //itemsToDry is a std::vector
  if (shirtA.cleanliness > shirtACleanliness) {
    itemsToDry.erase(shirtA);
    shirtA.cleanliness = shirtACleanliness;
    itemsToWash.insert(shirtA);
    doSomeOtherStuff(shirtA);
  }
} else {
  shirtA.cleanliness = shirtACleanliness;
  itemsToWash.insert(shirtA);
  doSomeOtherStuff(shirtA);
}
//am aware aware contains() is not a method for either type
//and std::vector does not have erase() by value, this is just conceptual

【问题讨论】:

  • 如果例如AB 是指针(这意味着你确实有 A->aB->a)然后你总是必须首先检查 AB
  • 请记住,如果出于任何原因,您必须在分支中使用略微不同的foo(),那么您将不得不创建一个额外的foo2()。我不会更改if 的格式,而是将foo() 分解成更小的、可管理的函数/类,这样每个if 子分支看起来就足够小了。
  • @Someprogrammerdude 它们不是指针,只是我正在尝试做的事情的抽象表示。但是,是的,即使在我的实际代码中,我也应该检查A && A.aB && B.a,而不仅仅是A.aB.a
  • @Tetix foo() 根据我上面的代码,无论何时调用它都是一样的;我已将所有特定于分支的内容放在somethingSpecificToAa()somethingSpecificToBa() 中。除非我误解了你的意思?
  • @ampharos 我只是提前考虑,以防foo() 在其中一种场景中变得不同,而somethingspecificA/B 无法合并。然后需要一个新的foo2() 等等。这就是为什么我建议尽可能将foo() 分解为更小的函数/类。

标签: c++ if-statement refactoring theory


【解决方案1】:

根据您最近的评论,这里有一些可能与您的用例相匹配的伪代码。

Element* element=nullptr;

if(vectorA.contains(X)){
   element = vectorA.get(X);
}else if(setB.contains(X)){
   element = setB.get(X);
}

if(element != nullptr && element->a){
   something(element);
}

if(element == nullptr || element->a){
    foo();
}

【讨论】:

  • 这是迄今为止最简洁、最容易理解的方法。可悲的是,除非我很厚,否则element 是从集合还是向量中获得的没有区别,我们只知道它是否在其中找到。所以我缺乏somethingExclusiveToAa()somethingExclusiveToBa() 区分的排他行为。在第二块(我们在第一块中分配)中使用bool 来决定 something(element) 和 somethingElse(element) 会不好?
  • 我在想 somethingExclusiveToAa()somethingExclusiveToBa() 只是获取元素,但如果不是这种情况,您可以删除 if(element != nullptr && element->a){ something(element);} 并将您的详细信息放在 get(X); 之后。似乎您的具体情况很难表达更通用的内容。在这一点上,我想说我们知道的还不够,无法继续前进。
  • 我现在编辑了这个问题,添加了一个更具体的例子,这样当我接受你的答案时,它的相关性就很清楚了,而无需浏览 cmets。我还根据您的建议对您的答案进行了修改。谢谢!
【解决方案2】:

既然里面有一些通用代码,为什么不把它分解成一个函数呢?你可以有类似的东西

template<typename T, typename F>
bool doSomething(T const& t, F f)
{
    if (t && t.a)
    {
        f();
        foo();
    }

    return static_cast<bool>(t);
}

并像使用它

if (!doSomething(A, &somethingSpecificToAa) && !doSomething(B, &somethingSpecificToBa))
{
    foo();
}

【讨论】:

  • 我真的很喜欢这个。也许我只是很慢,但在这种情况下,我如何为函子调用 f() 定义排他行为?无论我们是在看 A 还是 B,它应该是不同的。
  • 只要在A和B中定义一个公共函数f(),替换f();通过 t.f()。这意味着您不需要@Some程序员老兄提供的模板函数中的第二个参数。
猜你喜欢
  • 1970-01-01
  • 2020-08-15
  • 1970-01-01
  • 2021-04-11
  • 1970-01-01
  • 1970-01-01
  • 1970-01-01
  • 2011-02-01
  • 2012-02-12
相关资源
最近更新 更多