代码异味 - if/else 构造答案

【问题标题】：Code smell - if/else construct代码异味 - if/else 构造
【发布时间】：2013-12-10 14:59:18
【问题描述】：

我有一个包含 9 个元素的列表。前三个代表位置、下一个速度、下一个力。

有时我需要来自阵列的力，有时需要速度，有时需要位置。

于是我写了一个函数如下：

def Extractor(alist,quantity):

    if quantity=='positions':
        x = alist[0]
        y = alist[1]
        z = alist[2]
        return (x,y,z)

    elif quantity=='velocities':
        vx = alist[3]
        vy = alist[4]
        vz = alist[5]
        tot_v = np.sqrt(vx**2 + vy**2 + vz**2)
        return (vx,vy,vz,tot_v)

    elif quantity=='forces':
        fx = alist[6]
        fy = alist[7]
        fz = alist[8]
        tot_f = np.sqrt(fx**2 + fy**2 + fz**2)
        return (fx,fy,fz,tot_f)
    else:
        print "Do not recognise quantity: not one of positions, velocities, force"

但是，由于重复代码，这对我来说似乎是一种巨大的代码气味。有没有更好，更蟒蛇的方式来做到这一点？我对 OOP 很陌生，但是我可以使用某种利用多态性的类继承吗？

【问题讨论】：

创建三个函数。
顺便说一下，像Extractor 这样的大写名称通常用于类，而不是函数。这是一个约定，而不是要求，但使用它可以让人们更轻松地阅读您的代码。
可以考虑使用x, y, z = alist[0:3]，这样可以减少代码大小。
将所有这些值放在同一个数组中好吗？似乎它迫使你做不必要的工作，除非它有一些我没有看到的好处。

标签： python oop code-duplication

【解决方案1】：

您的方法违反了Single Responsibility Principle。考虑像这样拆分它：

def positionExtractor(alist):
    return tuple(alist[0:3])

def velocityExtractor(alist):
    velocity = tuple(alist[3:6])
    return velocity + (np.sqrt(sum(x**2 for x in velocity)),)

def forcesExtractor(alist):
    forces = tuple(alist[6:9])
    return forces + (np.sqrt(sum(x**2 for x in forces)),)

您可以将它们放入字典中：

extractors = {
    'position' : positionExtractor,
    'velocity' : velocityExtractor,
    'forces' : forcesExtractor}

并使用：

result = extractors[quantity](alist)

这是一个继承的例子。不过，对于这样一个简单的任务来说，这似乎是过度设计：

import numpy as np

class Extractor:
    def extract(self, alist):
        raise NotImplementedError()

class IndexRangeExtractor(Extractor):
    def __init__(self, fromIndex, toIndex):
        self.fromIndex = fromIndex
        self.toIndex = toIndex

    def extract(self, alist):
        return tuple(alist[self.fromIndex:self.toIndex])

class EuclideanDistanceExtractorDecorator(Extractor):
    def __init__(self, innerExtractor):
        self.innerExtractor = innerExtractor

    def extract(self, alist):
        innerResult = self.innerExtractor.extract(alist)
        distance = np.sqrt(sum(x**2 for x in innerResult))

        return innerResult + (distance,)

#... 

class ExtractorFactory:
    def __init__(self):
        self.extractors = {
            'position':IndexRangeExtractor(0, 3),
            'velocity':EuclideanDistanceExtractorDecorator(
                IndexRangeExtractor(3, 6)),
            'forces':EuclideanDistanceExtractorDecorator(
                IndexRangeExtractor(6, 9))}

    def createExtractor(self, quantity):
        return self.extractors[quantity]


alist = [1,2,3,4,5,6,7,8,9]
ef = ExtractorFactory()
e1 = ef.createExtractor('position')
e2 = ef.createExtractor('velocity')
e3 = ef.createExtractor('forces')

print e1.extract(alist)
print e2.extract(alist)
print e3.extract(alist)

【讨论】：

我已经发布了用于说明这一点的工作代码。

【解决方案2】：

您可以先使用偏移量来挑选元素；除了positions 之外，其他所有人都需要一个公式：

_slices = {'positions': slice(3), 'velocities': slice(3, 6), 'forces': slice(6, 9)}

def Extractor(alist, quantity):
    try:
        a, b, c = alist[_slices[quantity]]
        tot = np.sqrt(a**2 + b**2 + c**2)
        return a, b, c, tot
    except KeyError:
         raise ValueError(
             "Do not recognise quantity: "
             "not one of {}".format(', '.join(_slices)))

这会返回一个一致个值；如果无法计算 positions 的平方根，我将返回 0.0 的总数：

tot = np.sqrt(a**2 + b**2 + c**2) if quantity != 'positions' else 0.0

【讨论】：

虽然这减少了一点尺寸，但offset 本质上是毫无意义的，一点也不漂亮。
@Ray：是的，我考虑过使用itemgetter()，然后没有，现在 Jon 有一个使用它的解决方案。我将在这里使用切片。
@Ray：总之，这更好，有一个异常处理程序来处理无法识别的数量。

【解决方案3】：

为什么不试试这样的：

    def extract_position(x,y,z):
        return (x, y, z)

    def extract_velocities(x,y,z):
        return (x, y, z, np.sqrt(x**2 + y**2 + z**2))

    def extract_forces(x,y,z):
        return (x, y, z, np.sqrt(x**2 + y**2 + z**2))

    extractor = { 'positions': extract_position,
                  'velocities': extract_velocities,
                  'forces': extract_forces }

    try:

        print extractor['positions'](1,2,3)

        print extractor['unknown'](4,5,6)

    except KeyError:
        print "Do not recognise quantity: not one of positions, velocities, force"

我更喜欢使用函数指针将数据与任意计算联系起来。此外，字典替换了 switch 样式语法，因此至少感觉与您正在寻找的相似。

你也有相同的计算来确定速度和力，所以你也可以浓缩它。

【讨论】：

extractor 的全部目的是从列表中提取元素。例如，您的函数 extract_position 没有做任何有趣的事情 - 您可以将三个变量分组到 tuple 中，而无需专用函数。因此（以及其他一些次要原因）这个例子实际上没有意义。
重点是他应该改变存储他正在处理的信息的方式。我的示例将两者分开。
也许这是个好主意，但在这种形式下，这些功能没有任何意义。他们的名字具有误导性——他们不提取任何东西。这个例子可以很容易地简化为一个函数：euclideanDistance，它计算结果 tuple 的最后一个元素。
@brandonsimpkins 您可能错过了 OP 的功能是从 9 个 列表中“提取”三个值的事实。你的接受三个参数并返回所有三个参数，这没有多大意义。
@brandonsimpkins 这正是 OP 的代码可能的用途 - 将输入映射到更有意义的数据结构......

【解决方案4】：

可能是这样的：

from operator import itemgetter

def extract(sequence, quantity):
    try:
        a, b, c = {
            'positions': itemgetter(0, 1, 2),
            'velocities': itemgetter(3, 4, 5),
            'forces': itemgetter(6, 7, 8)
        }[quantity](sequence)
        return a, b, c, np.sqrt(a**2 + b**2, c**2)
    except KeyError as e:
        pass # handle no suitable quantity found here

请注意，始终执行计算...保持返回值与 4 元组一致...除非它是一个真正昂贵的计算，否则这应该不是问题。

【讨论】：