View Revisions: Issue #3381

Summary 0003381: double calls to CString::Trim throughout code base, apparently due to careless search and replace
Revision 2019-09-07 09:09 by WA9PIE
Description A best practice is to treat any external input as dirty: data that must be verified and cleaned before trusted and used. One simple edit is to trim whitespace from the ends of strings. " Hello" is, after all, a different string than "Hello", and users almost never mean to keep the trailing or leading whitespace.

While investigating an un-related issue, I noticed Change 0001668:


The only description I have for this change, which touches about 250 different files, is the comment "trimming stuff.. shit" so I have little insight into the motivation for the change or the desired effect.

Investigating the change, I see that about 800 lines of code which call TrimRight() or TrimLeft() to trim the whitespace from the right end or the left end (respectively) of a string were replaced with a call to "Trim()". Since I have no insight into the reasoning for the change, I can't be sure, but it seems to me to have been carelessly executed and ill-advised.

If we have this code:

    strInput.TrimRight();
    strInput.TrimLeft();


it's pretty clear that we wanted to trim both the leading and trailing whitespace. It's correct to replace that with a single call:

    strInput.Trim();


is less source code, less object code, and a bit faster.

However, the search-and-replace operation that I assume took place changed it to this:

    strInput.Trim();
    strInput.Trim();


Trimming both leading and trailing whitepsace twice in a row doesn't make sense. Only one call is necessary. I had always noticed such doubled calls in the code, but never knew why until I discovered Change 0001668.

I'm worried that other bugs are possible. Since the replacement blindly swapped "TrimRight" for "Trim", for example, it's possible that the code wanted leading white space to remain and became buggy after this careless search-and-replace work.

Revision 2019-07-08 14:53 by K7ZCZ
Description
A best practice is to treat any external input as dirty: data that must be verified and cleaned before trusted and used. One simple edit is to trim whitespace from the ends of strings. " Hello" is, after all, a different string than "Hello", and users almost never mean to keep the trailing or leading whitespace.

While investigating an un-related issue, I noticed Change 0001668:


The only description I have for this change, which touches about 250 different files, is the comment "trimming stuff.. shit" so I have little insight into the motivation for the change or the desired effect.

Investigating the change, I see that about 800 lines of code which call TrimRight() or TrimLeft() to trim the whitespace from the right end or the left end (respectively) of a string were replaced with a call to "Trim()". Since I have no insight into the reasoning for the change, I can't be sure, but it seems to me to have been carelessly executed and ill-advised.

If we have this code:

    strInput.TrimRight();
    strInput.TrimLeft();


it's pretty clear that we wanted to trim both the leading and trailing whitespace. It's correct to replace that with a single call:

    strInput.Trim();


is less source code, less object code, and a bit faster.

However, the search-and-replace operation that I assume took place changed it to this:

    strInput.Trim();
    strInput.Trim();


Trimming both leading and trailing whitepsace twice in a row doesn't make sense. Only one call is necessary. I had always noticed such doubled calls in the code, but never knew why until I discovered Change 0001668.

I'm worried that other bugs are possible. Since the replacement blindly swapped "TrimRight" for "Trim", for example, it's possible that the code wanted leading white space to remain and became buggy after this careless search-and-replace work.