C++重构案例1：repeat in switch

本文最后更新于：2022年2月5日晚上

前情提要

这是图片查看器(ImageViewer2.0)中的一段代码，用于检测按键并切换图片：

void Widget::keyPressEvent(QKeyEvent* event)
{
    const int N = fileList.size();//fileList存放图片文件名
    switch (event->key()) {
    case Qt::Key_Left: {
        index = qMax(index - 1, 0);
        QString filePath = curDirPath + '/' + fileList[index];
        setPixmap(filePath);
    } break;
    case Qt::Key_Right: {
        index = qMin(index + 1, N - 1);
        QString filePath = curDirPath + '/' + fileList[index];
        setPixmap(filePath);
    } break;
    default:
        break;
    }
}

可以发现，这段代码检测小键盘的Left/Right按键，并更改index以切换图片

問題点

由于Left/Right仅有index自增/自减的区别

导致7、8 && 12、13行代码重复

1 2	`QString filePath = curDirPath + '/' + fileList[index];//拼接文件路径 setPixmap(filePath);//设置图片`

代码重复！

我焯

重复了两行代码！

你能忍吗？！

我不能忍

士可杀不可辱！

插叙

在开始解决这个问题之前，先来解开大家对于代码的一点疑问

1.index增减

1	`index = qMax(index - 1, 0);`

这行代码是为了防止index在减小的过程中变成负数

看起来没什么问题对吧

确实没什么问题，但是nie

按照我风格，怎么的也得写成：

1	`index = qMax(--index, 0);`

这种前置自减运算符形式吧

虽说帅了一点，简洁了一点，这实际上这是undefined行为：在一个序列点前对index产生了两次副作用

--index自减操作对index的改变保证在;分号结束前完成
等号左边的index也会在;分号结束前被改变

无法判断这两种副作用发生顺序，是undefined行为

所以不建议这么写

2.case内花括号{}

case Qt::Key_Left: {
    index = qMax(index - 1, 0);
    QString filePath = curDirPath + '/' + fileList[index];
    setPixmap(filePath);
} break;

为何case内要加上花括号(line 1 & 5)呢

那我们不妨吧花括号去掉试试：

E:\Qt5.14.2\Projects\ImageViewer_2\widget.cpp:302: error: jump to case label [-fpermissive]
     default:
     ^~~~~~~
E:\Qt5.14.2\Projects\ImageViewer_2\widget.cpp:299: note:   crosses initialization of 'QString filePath'
         QString filePath = curDirPath + '/' + fileList[index];
                 ^~~~~~~~

哦吼报错了

这是因为变量filePath的作用域是初始化点到switch的结尾处

由于我们无法确定其他case中是否会使用到这个变量，也无法判断使用之前变量是否被初始化（选择支具有不确定性）

导致编译器晕乎

所以只要用花括号{}限定filePath的作用域在单一case中即可（不会有人不知道{}限制作用域吧，不会吧不会吧）

Okey-dokey，解决了这些疑问，我们可以开始重构代码了

重构

其一：case fall through

众所周知，case标签内的代码执行完后并不会主动退出switch，而是自由落体至下一个case

（这也就是要加上break的原因）

だから、只要利用这一特性，就能让case Qt::Key_Left滑落到case Qt::Key_Right

从而实现复用代码的目的啦（小天才）

void Widget::keyPressEvent(QKeyEvent* event)
{
    const int N = fileList.size();
    switch (event->key()) {
    case Qt::Key_Left:
        index -= 2;
    case Qt::Key_Right: {
        index = qBound(0, index + 1, N - 1);//std::clamp
        QString filePath = curDirPath + '/' + fileList[index];
        setPixmap(filePath);
    } break;
    default:
        break;
    }
}

当然这里会给出Warning：this statement may fall through

hhh 用的就是这个特性

然后利用qBound来限制index范围（std::clamp也行）

其二：优雅のlambda

虽说case自由落体很帅，不过可读性基本是负数，估计一个月后自己也一脸懵逼了（自食恶果）

既然代码重复，那就封装函数就好了嘛，多简单的道理

而lambda可以简化小函数的书写，且可以写在另一个函数内

void Widget::keyPressEvent(QKeyEvent* event)
{
    static auto alterPixmap = [=](int i) {
        QString filePath = curDirPath + '/' + fileList[i];
        setPixmap(filePath);
    };
    const int N = fileList.size();
    switch (event->key()) {
    case Qt::Key_Left:
        index = qMax(index - 1, 0);
        alterPixmap(index);
        break;
    case Qt::Key_Right:
        index = qMin(index + 1, N - 1);
        alterPixmap(index);
        break;
    default:
        break;
    }
}

稳健

不过，这代码怎么变长了，emmm 等下

既然封装，我为什么不把index范围判断也扔进去，焯

void Widget::keyPressEvent(QKeyEvent* event)
{
    static auto alterPixmap = [=](int i) {
        const int N = fileList.size();
        index = qBound(0, i, N - 1);//this->index
        QString filePath = curDirPath + '/' + fileList[index];
        setPixmap(filePath);
    };
    switch (event->key()) {
    case Qt::Key_Left:
        alterPixmap(index - 1);
        break;
    case Qt::Key_Right:
        alterPixmap(index + 1);
        break;
    default:
        break;
    }
}

啊哈哈哈哈，我滴任务完成辣

只能用帅气来形容

不过这里有个小知识点需要注意

lambda & this

lambda的捕获列表要如何处理类内的数据成员 & 成员函数呢？

其实际上，所有的类内变量 & 函数都是通过this指针来调用的

只要用[=]拷贝this指针即可

如：index，即this->index，也就可以随意修改调用了

其三：正经函数

虽然lambda很优雅，但是在函数内定义，无法被外界调用

所以，我们还是老老实实写成函数吧（顺便改个名）

int Widget::switchPixmap(int i)
{
    const int N = fileList.size();
    index = qBound(0, i, N - 1);
    QString filePath = curDirPath + '/' + fileList[index];
    setPixmap(filePath);
    return index;
}
void Widget::keyPressEvent(QKeyEvent* event)
{
    switch (event->key()) {
    case Qt::Key_Left:
        switchPixmap(index - 1);
        break;
    case Qt::Key_Right:
        switchPixmap(index + 1);
        break;
    default:
        break;
    }
}