View Issue Details

IDProjectCategoryView StatusLast Update
00033813 - Current Dev ListMaintenancepublic2019-07-08 16:11
ReporterK7ZCZAssigned ToK7ZCZ 
PrioritynormalSeverityminorReproducibilityhave not tried
Status resolvedResolutionfixed 
Product Version6.6.0.236 
Target VersionFixed in Version 
Summary0003381: double calls to CString::Trim throughout code base, apparently due to careless search and replace
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.

TagsNo tags attached.
ModuleAll
Sub-ModuleGeneral
TestingNot Started

Activities

K7ZCZ

2019-07-08 16:11

administrator   ~0008207

fixed with this checkin in the 6.7 branch:
https://hrdsoftware.visualstudio.com/HRD/_versionControl/changeset/5058

Issue History

Date Modified Username Field Change
2019-07-08 14:53 K7ZCZ New Issue
2019-07-08 16:11 K7ZCZ Assigned To => K7ZCZ
2019-07-08 16:11 K7ZCZ Status new => resolved
2019-07-08 16:11 K7ZCZ Resolution open => fixed
2019-07-08 16:11 K7ZCZ Note Added: 0008207