CODEWITHSUNDEEP
c++
moo
moo

Reputation:

Simple C++ function -- Is this code "good"?

The following code was produced by a consultant working for my group. I'm not a C++ developer (worked in many languages, though) but would like some independent opinions on the following code. This is in Visual Studio C++ 6.0. I've got a gut reaction (not a good one, obviously), but I'd like some "gut reactions" from seasoned (or even not so unseasoned) C++ developers out there. Thanks in advance!

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

...snip...

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);
}

Upvotes: 6

Views: 1481

Answers (13)

Adam Liss
Adam Liss

Reputation: 48290

Is this consultant being paid by the line of code? Several folks have pointed out that the CString class already provides this functionality, so even if you're not a programmer, you know:

  • The function is unnecessary. It adds to the complexity, size, and possibly the execution time of the program.
  • The CString function probably works and is probably efficient; this one may or may not be.
  • The CString function is documented, and is therefore supportable.
  • The consultant is either unfamiliar with the standard CString function or thought he/she could do better by writing a new one.
    • One might conclude that the consultant is unfamiliar with other standard features and best practices.
    • Choosing to write new code for a basic feature, without considering that a standard version may exist, is an accepted bad practice.

And perhaps the biggest, reddest flag of all: your instincts prodded you to get opinions from the StackOverflow community.

Trust your instincts.

Upvotes: 1

paercebal
paercebal

Reputation: 83309

I won't provide an alternative code, as it would only add to the code already provided.

But, my gut feeling is that there's something wrong with the code.

To prove it, I will enumerate some points in the original function that shows its developer was not an experienced C++ developer, points that you should investigate if you need a clean implementation:

  • copy : parameters are passed as copy instead of const-reference. This is a big NO NO in C++ when considering objects.
  • bug I guess there is an error in the "if(occurenceInd>0)" part. By reading CString's doc on MSDN, the CString::Find method returns -1, and not 0 when the find failed. This code tells me if a comma was the first character, it would not be escaped, which is probably not the point of the function
  • unneeded variable : "continueLoop" is not needed. Replacing the code "continueLoop = false" by "continue" and the line "while(continueLoop)" by "while(true)" is enough. Note that, continuing this reasoning enable the coder to change the functions internal (replacing a do...while by a simple while)
  • changing return type : Probably picking at details, but I would offer an alternative function which, instead of returning the result string, would accept is as a reference (one less copy on return), the original function being inlined and calling the alternative.
  • adding const whenever possible again, picking at detail: the two "char" parameters should be const, if only to avoid modifying them by accident.
  • possible multiple reallocation the function relies on potential multiple reallocations of the CString data. Josh's solution of using std::string's reserve is a good one.
  • using fully CString API : But unlike Josh, because you seem to use CString, I would avoid the std::string and use CString::GetBufferSetLength and CString::ReleaseBuffer, which enable me to have the same code, with one less object allocation.
  • Mysterious Insert method? it's me, or there is no CString::Insert ??? (see http://msdn.microsoft.com/en-us/library/aa252634(VS.60).aspx). In fact, I even failed to find CString in the same MSDN for Visual C++ 2008 and 2005... This could be because I should really go to sleep, but still, I guess this is worth investigating

Upvotes: 2

Eclipse
Eclipse

Reputation: 45493

There's an off-by-one bug sitting there right in the middle: Take a look at what happens if the first character is a comma: ",abc,def,ghi": I'm assuming the desired output would be "\,abc\,def\,ghi", but instead you get the original string back:

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

OccurrenceInd returns 0, since it found charFind at the first character.

if(occurenceInd>0)

0 isn't greater than 0, so take the else branch and return the original string. CString::Find returns -1 when it can't find something, so at the very least that comparison should be:

if(occurrenceInd >= 0)

The best way would be to use the Replace function, but if you want to do it by hand, a better implementation would probably look something like this:

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());
}

Upvotes: 11

Sol
Sol

Reputation: 2185

The mistakes have already been mentioned. But they strike me as the sort of problems anyone might have with quickly dashed-off code which has not been properly tested, particularly if they are not familiar with CString.

I'd worry more about the stylistic things, as they suggest someone who is not comfortable with C++. The use of the bool continueLoop is just plain poor C++. It represents a third of the function's code that could be eliminated by the use of a simple if...break construct, making the code easier to follow in the process.

Also, the variable name "originalString" is very misleading. Because they pass it by value, it's not the original string, it's a copy of it! Then they modify the string anyway, so it no longer is the same object or the same string of text as the original. This double lie suggests muddled thought patterns.

Upvotes: 5

Ateş G&#246;ral
Ateş G&#246;ral

Reputation: 140050

My gut reaction is: WTF. Initially by how the code is formatted (there are lots of things I don't like about the formatting) and then by examination of what the code is actually doing.

There's a serious issue with this developer's understanding of object copying in C++. The example is a WTF in itself (if the developer of the function really used his/her own function like this):

// 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 )
  1. Pass a copy of strColHeader as originalString (notice that there's no &)
  2. The function modified this copy (fine)
  3. The function returns a copy of the copy, which in turn replaces the original strColHeader. The compiler will probably optimize this out to a single copy but still, passing around object copies like this doesn't work for C++. One should know about references.

A more seasoned developer would have designed this function as:

void insert_escape(CString &originalString, char charFind, char charInsert)

or:

CString insert_escape(const CString &originalString, char charFind, char charInsert)

(And probably would have named the parameters a bit differently)

And as many have pointed out, the sane thing the developer could have done was to check the API documentation to see if CString already had a Replace method...

Upvotes: 3

Jackson
Jackson

Reputation: 5657

I always worry when I see a do .. while loop; IMO they're always harder to understand.

Upvotes: 0

Konrad Rudolph
Konrad Rudolph

Reputation: 545588

This is in Visual Studio C++ 6.0.

Gut reaction: blech. Seriously! The C++ compiler shipped with VC++ 6 is known to be buggy and generally performing very badly and it's 10 years old.

@Downvoters: consider it! I mean this in all seriousness. VC6 is just comparatively unproductive and should not be used any more! Especially since Microsoft discontinued its support for the software. There are cases where this can't be avoided but they are rare. In most cases, an upgrade of the code base saves money. VC++ 6 just doesn't allow to harness the potential of C++ which makes an objectively inferior tool.

Upvotes: 0

JohnMcG
JohnMcG

Reputation: 8805

If you're wanting an evaluation of this developer's C++ skill level, I'd say this demonstrates the lower end of intermediate.

The code gets the job done, and doesn't contain any obvious "howlers," but as others have written, there are better ways of doing this.

Upvotes: 2

Phil Hannent
Phil Hannent

Reputation: 12317

There is always a better implementation. If you are using the function as an example of the consultant not being very good you might also want to consider that while they did not know a function that already existed they might have experience and understanding of project construction.

Software development is not just about the perfect function but also how good the architecture of the whole thing is.

Upvotes: 0

Ed Altorfer
Ed Altorfer

Reputation: 4371

Looks like people have tackled some of the functionality aspects of this code for you, but I'd suggest staying away from variable naming like you've employed here.

With the exception of UI controls, it is generally frowned upon to use Hungarian notation. This is more important with numbers...for example:

I declare:

float fMyNumber = 0.00;

And then I use that in my entire application. But then, later, I change that to a double because I realize that I need more precision. Now I have:

double fMyNumber = 0.00;

It's true that most good refactoring tools could fix this for you, but it's probably best not to attach those prefixes. They are more common in some languages than others, but from a general style perspective, you should try to avoid them. Unless you're using Notepad, you probably have something akin to Intellisense, so you don't really need to look at the variable name to figure out what type it is.

Upvotes: 0

Nick
Nick

Reputation: 6846

CString has a Replace() method... (that was my 1st reaction)

I have seen a lot of bad code, and lots worse than this. However, not using built-in functionality when there's no apparent good reason not to is... poor.

Upvotes: 3

Paul Nathan
Paul Nathan

Reputation: 40309

Looks alright, if a bit, I dunno, hack. Better to use the library, but I wouldn't go and rewrite this routine.

Upvotes: 0

gbjbaanb
gbjbaanb

Reputation: 52679

hmm. I think 

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

would do just as well.

I don't like the code, I tend to break from the while loop instead of having an unnecessary bool 'continue' flag. That goes double when he could have used while (occurenceInd != 0) as his loop control variable instead of the boolean.

Incrementing the counter also relies on "+2" which doesn't seem immediately understandable (not at a quick glance), and lastly (and most importantly) he doesn't seem to do comments.

Upvotes: 17

Related Questions