Cheap code



  • This is what you get for farming development to the lowest bidder:

    /// <summary>
    /// For creating the XML
    /// </summary>
    /// <param name="fooRequest">Object of fooRequest</param>
    /// <returns>Returns the XML</returns>
    private string GetFooActivity(FooRequest fooRequest)
    {
        return "<FOO_ACTIVITY" + " EntryMethod=\"" + fooRequest.EntryMethod +
        "\" CriteriaType=\"" + fooRequest.CriteriaType +
        "\" Criteria=\"" + fooRequest.Criteria + "\" SearchRC=\"0\"" +
        " HeaderAdded=\"" + fooRequest.HeaderAdded + "\"></FOO_ACTIVITY>";
    }

    Also this:

    StringBuilder build = new StringBuilder();
    foreach (string number in lineNumber) // Loop through List with foreach
    {
        s = "&lt;LineNumber" + number + "&gt;&lt;/LineNumber" + number + "&gt;";

     // Append the string to build

       build.Append(s);
    }



  • If the XML is only used internally to their program why do you care what it looks like?  If it is used by other systems why didn't you provide a schema (or worse yet you did and it was just so bad that you got crap back)?



  • TRWTF is using StringBuilder and not String, amirite?

    </sarcasm>

    Code like this smacks of someone trying to migrate programming skills from another language. I'm sure somwehere there's a website containing code smatterings with "guess the programmers core language".



  • @locallunatic said:

    If the XML is only used internally to their program why do you care what it looks like?  If it is used by other systems why didn't you provide a schema (or worse yet you did and it was just so bad that you got crap back)?

     I should have added more emphasis.  TRWTFs are:

    • XML (triple-slash) documentation that tells you fuckall
    • Creating XML via string concatenation when .NET provides some very handy classes for XML manipulation
    • The comments in the second snippet exist in the code; they weren't added by me


  • @Smitty said:

    I should have added more emphasis.  TRWTFs are:

    • XML (triple-slash) documentation that tells you fuckall
    • Creating XML via string concatenation when .NET provides some very handy classes for XML manipulation

    OK, the first one is annoying (but at least they put something).  Second is very much a problem; I didn't mean to imply that there weren't issues with the code you posted, just that some of the obvious ones weren't so big.



  • @locallunatic said:

    Second is very much a problem; I didn't mean to imply that there weren't issues with the code you posted, just that some of the obvious ones weren't so big.

    I read these forums for the learning experience, So I just want to double-check something:

    Is the use of the Stringbuilder outright a WTF, or just the use in this case?

    In university, I was taught to use Stringbuilder whenever a string is frequently getting data appended to it, as stringVar = stringVar + "test" will allocate memory for the full new value of stringVar, not simply append "test" onto the old location of stringVar, as string objects don't have buffers.

    Or is the WTF simply that they're creating string s, then appending it? Or is it that a suitable buffer in this case isn't specified after the creation of the StringBuilder?

     



  • @Adanine said:

    @locallunatic said:

    Second is very much a problem; I didn't mean to imply that there weren't issues with the code you posted, just that some of the obvious ones weren't so big.

    I read these forums for the learning experience, So I just want to double-check something:

    Is the use of the Stringbuilder outright a WTF, or just the use in this case?

    In university, I was taught to use Stringbuilder whenever a string is frequently getting data appended to it, as stringVar = stringVar + "test" will allocate memory for the full new value of stringVar, not simply append "test" onto the old location of stringVar, as string objects don't have buffers.

    Or is the WTF simply that they're creating string s, then appending it? Or is it that a suitable buffer in this case isn't specified after the creation of the StringBuilder?

     


    Basically, if you're adding to a string in a loop, use a StringBuilder. If you add strings with + and then immediately shove the result into a StringBuilder, you're losing all the benefits of using a StringBuilder.



  • You still get most of the benefit of StringBuilder since the long string is built with it. It could be better but performance will be the same order at least.



  • @Jonathan said:

    performance will be the same order

    "the same order" and "thousands of times slower" are not mutually exclusive.



  • @... said:

    StringBuilder
    its very name identifies it as a leaky abstraction.



  • @Ben L. said:

    @Jonathan said:
    performance will be the same order

    "the same order" and "thousands of times slower" are not mutually exclusive.
    one thousand is three orders of magnitude!



    or maybe that should be

    @Ben L. said:
    @Jonathan said:
    performance will be the same order

    "the same order" and "thousands of times slower" are not mutually exclusive?
    FTFY



  • @esoterik said:

    @Ben L. said:
    @Jonathan said:
    performance will be the same order

    "the same order" and "thousands of times slower" are not mutually exclusive.
    one thousand is three orders of magnitude!



    or maybe that should be

    @Ben L. said:
    @Jonathan said:
    performance will be the same order

    "the same order" and "thousands of times slower" are not mutually exclusive?
    FTFY

    O(1000n2) and O(n2) are the same order.



  • @Ben L. said:

    O(1000n2) and O(n2) are the same order.
     

    Do you have any citations for your claim that it's a thousand times slower? As far as I know, in this particular case the C# compiler will convert the "const + variable + const + variable" string concatenation operation into a single call to String.Concatenate(string, string, string, string) .

    I kinda doubt that a single StringBuilder.Append(String.Concat(stuff)) is 1000x slower than StringBuilder.Append 4x, especially since your two const strings are going to be internalized anyway.

    And fundamentally, it's significantly easier to read StringBuilder.Append(stuff + more_stuff + more_stuff) than it is to read StringBuilder.Append(stuff).Append(more_stuff).Append(more_stuff).



  • @Ben L. said:

    O(1000n2) and O(n2) are the same order.

    You guys are using the same word to mean "order of magnitude" on the one hand, and "asymptotic complexity" on the other hand.



  • @Adanine said:

    @locallunatic said:

    Second is very much a problem; I didn't mean to imply that there weren't issues with the code you posted, just that some of the obvious ones weren't so big.

    I read these forums for the learning experience, So I just want to double-check something:

    Is the use of the Stringbuilder outright a WTF, or just the use in this case?

    Using a StringBuilder to build a string isn't the problem, but using one to build XML is.



  •  @Smitty said:

    Also this:

    StringBuilder build = new StringBuilder();
    foreach (string number in lineNumber) // Loop through List with foreach
    {
        s = "&lt;LineNumber" + number + "&gt;&lt;/LineNumber" + number + "&gt;";

     // Append the string to build

       build.Append(s);
    }

    While I hate blueprint comments as much as the next guy, did it occur to anyone that they're creating escaped XML here which looks like <LineNumber1></LineNumber1><LineNumber2></LineNumber2> ? Good luck validating, querying or even parsing that one...

     



  • @Smitty said:

    This is what you get for farming development to the lowest bidder:

    /// <summary>
    /// For creating the XML
    /// </summary>
    /// <param name="fooRequest">Object of fooRequest</param>
    /// <returns>Returns the XML</returns>
    private string GetFooActivity(FooRequest fooRequest)
    {
        return "<FOO_ACTIVITY" + " EntryMethod="" + fooRequest.EntryMethod +
        "" CriteriaType="" + fooRequest.CriteriaType +
        "" Criteria="" + fooRequest.Criteria + "" SearchRC="0"" +
        " HeaderAdded="" + fooRequest.HeaderAdded + ""></FOO_ACTIVITY>";
    }

    Also this:

    StringBuilder build = new StringBuilder();
    foreach (string number in lineNumber) // Loop through List with foreach
    {
        s = "&lt;LineNumber" + number + "&gt;&lt;/LineNumber" + number + "&gt;";

     // Append the string to build

       build.Append(s);
    }

    Maybe it's me but if we take apart the fact that they are creating XML... that's the fastest way to concatenate strings in C#

    If the string constant is known the best way to join them is to use String.Concat (or use the + operator), otherwise, if it's not known, the best way is to use a StringBuilder and they are mixing both to in the second method to have a proper performance


  • Trolleybus Mechanic

    @locallunatic said:

    Using a StringBuilder to build a string isn't the problem, but using one to build XML is.
     

    Easy fix in one line:

       public class XMLBuilder : StringBuilder { }




  • @Khalin said:

    Maybe it's me but if we take apart the fact that they are creating XML...
    "If we take out the biggest failure of all..."@Khalin said:
    that's the fastest way to concatenate strings in C#

    If the string constant is known the best way to join them is to use String.Concat (or use the + operator), otherwise, if it's not known, the best way is to use a StringBuilder and they are mixing both to in the second method to have a proper performance

    I highly doubt you've done the performance metrics.  And since the StringBuilder is not being initialized with a length, I highly doubt this could not be optimized.  Regardless, the CORRECT way to concatenate strings is what is most readable and maintainable, since the performance will not matter in your application (99.9% of the time).


Log in to reply