Inherited code



  • Found this in a project I have inherited at work:

                 if (pLine.Length > 4)
                    if (pLine.Substring(0, 4) == ("and "))
                        pLine = pLine.Substring(0, 4).Replace("and ", "").Trim() + pLine.Substring(4);

                if (pLine.Length > 2)
                {
                    while (pLine.IndexOf("&") == 0)
                    {
                        pLine = pLine.Substring(0, 1).Replace("&", "") + pLine.Substring(1);
                        pLine = pLine.Trim();
                    }
                }



  • I... words...



  •  So if I understand correctly, the thought process leading to this code went something like:

    "Hmmm, how can I make sure, that whatever 'and ' is in the beginning of this string gets moved to the end without being changed to another 'and '?"

     

    ...do I? Because... well... really??



  • @SEMI-HYBRID code said:

    So if I understand correctly, [...]
    You don't.

     



  • You understood wrongly, the code removes the following strings from the beginning of a string: "and ", "& ". Oh, and it also removes white space at the ends afterwards. It's likely list formating cleanup code.

    I don't see any real issue with the code.



  • If only there were some kind of method that tells you whether one string StartsWith another string... oh, wait.

    Given the apparent "brillancy" of the original author of the code, even if the existence of the StartsWith method were pointed out to him, that would probably only have made it worse, since he seems unable to grasp even the most basic concepts of string manipulation (and possibly coding in general - people like that should be kept away from code by all means).

    Probably something like if (pLine.Substring(0, 4).StartsWith("and ")) ... and going downhill from there.



  • I think some people failed to grasp the beauty of this line:

     pLine = pLine.Substring(0, 1).Replace("&", "") + pLine.Substring(1);

    You know the line starts with "&", then you take the substring (0, 1), which gives "&", replace the ampersand by the empty string, and then concatenate the rest of the string. Which is of course equivalent to

    pLine = pLine.Substring(1);

    The same happens in the first replacement. Plus that the tests for length are unnecessary, and that he might also have just used a regexp to remove all & at the start of the line.

    It's like a little cluster of evidence that we're looking at the work from someone who doesn't quite understand how procedural programming works.



  • @TGV said:

    Plus that the tests for length are unnecessary, and that he might also have just used a regexp to remove all & at the start of the line.

    Not quite. If the original string is "and ", then it is kept intact. Then, after removing the ANDs, if it starts with a "&" but is no more than 2 characters, it is also kept as is. So the tests for length are not redundant. On the other hand, they are quite likely to be buggy.



  • @henke37 said:

    I don't see any real issue with the code.
    !!



  • @TGV said:

    I think some people failed to grasp the beauty of this line:

     pLine = pLine.Substring(0, 1).Replace("&", "") + pLine.Substring(1);

    You know the line starts with "&", then you take the substring (0, 1), which gives "&", replace the ampersand by the empty string, and then concatenate the rest of the string. Which is of course equivalent to

    pLine = pLine.Substring(1);

    The same happens in the first replacement. Plus that the tests for length are unnecessary, and that he might also have just used a regexp to remove all & at the start of the line.

    It's like a little cluster of evidence that we're looking at the work from someone who doesn't quite understand how procedural programming works.

    I wouldn't think that he understands better how object oriented programming works. Nor functional programming. Nor any kind of programming for that matter...


Log in to reply