简单的C++函数 - 这段代码"好"吗？

Question

以下代码由为我的小组工作的顾问制作.我不是一个C++开发人员(虽然工作在很多语言中)但是想对以下代码有一些独立的意见.这是在Visual Studio C++ 6.0中.我有一个直觉反应(显然不是一个很好的反应),但我想要那些来自经验丰富(甚至不是那么没有经验)的C++开发人员的"直觉反应".提前致谢!

// Example call
strColHeader = insert_escape(strColHeader, ',', '\\'); //Get rid of the commas and make it an escape character

...略...

CString insert_escape ( CString originalString, char charFind, char charInsert ) {
    bool continueLoop = true;   
    int currentInd   = 0;

    do {
        int occurenceInd = originalString.Find(charFind, currentInd);

        if(occurenceInd>0) {
            originalString.Insert(occurenceInd, charInsert);
            currentInd = occurenceInd + 2; 
        }
        else {
            continueLoop = false;   
        }
    } while(continueLoop);
    return(originalString);
}

Answer 1

哼.我认为

CString strColHeader;
strColHeader.Replace(",", "\\,") 

也会这样做.

我不喜欢代码,我倾向于从while循环中断,而不是有一个不必要的bool'continin'标志.当他可以用作while (occurenceInd != 0)他的循环控制变量而不是布尔值时,它会加倍.

增加计数器也依赖于"+2",这似乎不能立即理解(不是一瞥),最后(最重要的)他似乎没有做评论.

Answer 2

中间有一个一个一个错误的错误:看看如果第一个字符是逗号会发生什么:",abc,def,ghi":我假设所需的输出是"\, abc \,def \,ghi",而是你得到原来的字符串:

int occurenceInd = originalString.Find(charFind, currentInd);

OccurrenceInd返回0,因为它在第一个字符处找到了charFind.

if(occurenceInd>0) 

0不大于0,所以取else分支并返回原始字符串. CString :: Find在找不到内容时返回-1,所以至少比较应该是:

if(occurrenceInd >= 0)

最好的方法是使用Replace函数,但如果你想手动完成,更好的实现可能看起来像这样:

CString insert_escape ( const CString &originalString, char charFind, char charInsert ) {
    std::string escaped;
    // Reserve enough space for each character to be escaped
    escaped.reserve(originalString.GetLength() * 2); 
    for (int iOriginal = 0; iOriginal < originalString.GetLength(); ++iOriginal) {
        if (originalString[iOriginal] == charFind)
            escaped += charInsert;
        escaped += originalString[iOriginal];
    }
    return CString(escaped.c_str());
}

Answer 3

已经提到了错误.但是,他们认为任何人都可能遇到的问题很快就会被快速破解,而且还没有经过适当的测试,特别是如果他们不熟悉CString的话.

我更担心风格的东西,因为他们建议有人不熟悉C++.使用bool continueLoop只是简单的C++.它代表函数代码的三分之一,可以通过使用简单的if ... break构造来消除,使代码更容易在流程中遵循.

此外,变量名称"originalString"非常具有误导性.因为它们是按值传递的,所以它不是原始字符串,而是它的副本!然后他们无论如何都要修改字符串,因此它不再是与原始文本相同的对象或相同的文本字符串.这种双重谎言暗示了混乱的思维模式.

Answer 4

CString 有一个 Replace() 方法...（这是我的第一反应）

我见过很多糟糕的代码，还有比这更糟糕的。然而，在没有明显的充分理由的情况下不使用内置功能是……很糟糕的。