Better be sure that string is empty!



  • Why do people have so much trouble with strings?! Strings are pretty straight-forward in C#, but I found this is some code that I've been lucky enough to do bug fixes on.

    // Get the Account ID....
    string AccountID = "";
    AccountID = GetAccountID(UserID).ToString();
    if (AccountID == "")
        AccountID = "";

    And here are the relevant lines of the GetAccountID method:

    private string GetAccountID(string UserID) {
        if (something) {
            ...
            return "foo";
        } else {
            ...
            return "";
        }
    }

    This is just a brief sample of the fun I've been having. The original programmer was obsessed with the ToString() method. Every single method or property referenced gets ToString()ed. Now, this doesn't seem like a huge issue, but it sure makes tracking down a data conversion bug so much more fun! Oh, and the comment is so useful - thanks champ!



  • Looks like they came from a C background, as in string == NULL is not the same as strlen(string) == 0.  So they make sure if the function/method returned "null" it would make sure the string was empty.  I saw this a lot in the past from C programmer attempting to write C++.



  • @skippy said:

    Looks like they came from a C background, as in string == NULL is not the same as strlen(string) == 0.  So they make sure if the function/method returned "null" it would make sure the string was empty.  I saw this a lot in the past from C programmer attempting to write C++.

    That would at least be understandable, but he was a VB programmer before switching to C#/.Net. That explains why he declares and instantiates the variable on different lines. It doesn't explain all the ToString() calls or the set to empty if empty code. He's the sort of VB programmer that gives VB programmers a bad name.



  • Sheesh - the problems people have with Strings and dates. I especially like the ones where they try to convert back and forth, and invariably get all of the conversion methods wrong. I'd complain, but it's the kind of thing that guarantees that I'll always have a job :)

     



  • Hmm instantiation separate from declaration, I actually like that attitude.  This way you can declare a variable then instantiate within the try block.  Why is this important?  Because instantiation can actually cause errors.  If you have objects that require Dispose() then you want to call Dispose() even if there is an error, so you call it in the Finally block.  The only way to make sure you can dispose is if the object is declared.  If it is at least declared you can check to see if it is NULL and if not then Dispose().  Since this is a common practice I split my instantiation from my declaration for everything.

     As for the ToString() on everything, very bad form.  Keep in mind this is supposed to be an ID, I am assuming from an autonumber field in a database.  No reason to potentially lose the original data by saying ToString() and doing other funky things to it.

    ToString() has gotten a very bad rap, but it is useful for many things.  One is if you have a class, you can overload the ToString() call on that class so it returns a nice formatted version of the data it contains.  For example, a Person object may have a Salutation, FirstName, LastName and MiddleName properties.  Now overload the ToString() method to return Mr. John William Doe so your client code doesn't have to do the concatenation itself.  Another example is addresses, the ToString() can be overloaded to return the nicely formatted version like the following:

    123 This Street

    Somewhere, IN 12345

    Nicely done with ToString(), but yes, let's not overuse it.
     



  • Can someone please explain the fetish of writing

    if(cond) {

    /* ... code ... */

    return;

    } else {

    /* ... more code ... */

       return;

    }

    when it's pretty obvious that if the first branch is taken and returns, the second branch can't be taken, and therefore needn't be protected by an else?   I hate to say this, but the same pointless construct appears in the original K&R C book, so I understand it's a time-honored tradition for those that mimic Dennis Ritchie, but geez...it really says "I have no idea what I'm doing" when you write something like that.



  • @mrprogguy said:

    Can someone please explain the fetish of writing

    if(cond) {

    /* ... code ... */

    return;

    } else {

    /* ... more code ... */

       return;

    }

    when it's pretty obvious that if the first branch is taken and returns, the second branch can't be taken, and therefore needn't be protected by an else?   I hate to say this, but the same pointless construct appears in the original K&R C book, so I understand it's a time-honored tradition for those that mimic Dennis Ritchie, but geez...it really says "I have no idea what I'm doing" when you write something like that.

    Personally I prefer the idea of one point of return so I would write the following:

    var retval; 

    if(cond) {

    /* ... code ... */

    retval = 'foo'; 

    } else {

    /* ... more code ... */

    retval = ''; 

    }

    return retval;

    The exact same results are attained but follows my personal rules of good development.  If you need to return a value, set the value in the block and return it after the block.

     



  • @KattMan said:

    Personally I prefer the idea of one point of return so I would write the following:

    I tend to follow this as well, but as some deep logic can get complicated it can really take away from readability, especially in parsing functions.  You end up with a lot of if/else's that cause clutter.

     
    Each method has their appropriate place.  I've been in countless arguments with people about this.


     



  • Depends on whether the two branches are logically on the same level or not.

    So, I would write:

        if(bt->value > value)
            return BinarySearch(bt->left, value);
        else
            return BinarySearch(bt->right, value);

    (of course, this can just as well be done using the ?: operator)
    but:

        FILE *fh = fopen(...);
        if(!fh)
        {
            // handle this error...
            return;
        }
        goahead(fh);

    However, in old (non-C99) C, there are good reasons to write the else paths: one can then declare new variables where they get initialized (in old C, they can only be declared at the beginning of a block).



  • @skippy said:

    @KattMan said:

    Personally I prefer the idea of one point of return so I would write the following:

    I tend to follow this as well, but as some deep logic can get complicated it can really take away from readability, especially in parsing functions.  You end up with a lot of if/else's that cause clutter.

     
    Each method has their appropriate place.  I've been in countless arguments with people about this.

     

    If you end up with a lot of it/elses that cause clutter, then you should be using some other construct. 



  • @mrprogguy said:

    it really says "I have no idea what I'm doing" when you write something like that.

    No, it says "My preferred style is not the same as mrprogguy's.  Teh horrors!!!!!!2"



  • @skippy said:

    @KattMan said:

    Personally I prefer the idea of one point of return so I would write the following:

    I tend to follow this as well, but as some deep logic can get complicated it can really take away from readability, especially in parsing functions.  You end up with a lot of if/else's that cause clutter.

    Each method has their appropriate place.  I've been in countless arguments with people about this.

    Not meant as a "You are wrong!"  but as an addition to this single-point discussion:

    Return ends the function. Forcing a "retval" variable (I hate that name. I really do. That's personal.) doe not end the function. It forces extra logic.

    The readability is either the same, or less for single-exit functions.



  • @KattMan said:

    Hmm instantiation separate from declaration, I actually like that attitude.  This way you can declare a variable then instantiate within the try block.  Why is this important?  Because instantiation can actually cause errors.  If you have objects that require Dispose() then you want to call Dispose() even if there is an error, so you call it in the Finally block.  The only way to make sure you can dispose is if the object is declared.  If it is at least declared you can check to see if it is NULL and if not then Dispose().  Since this is a common practice I split my instantiation from my declaration for everything.



    But it isn't needed, they made the using statement for that

    using (SqlConnection conn = new SqlConnection("some connection string"))
    {
      // You can even throw an exception here yourself and the conn will STILL be disposed
    }

    No need to call Dispose yourself, no need to seperate declaration, no need to check if conn isn't null

    About the ToString(), they are called on a string :| which means it just returns itself.



  • @skippy said:

    Looks like they came from a C background, as in string == NULL is not the same as strlen(string) == 0.  So they make sure if the function/method returned "null" it would make sure the string was empty.  I saw this a lot in the past from C programmer attempting to write C++.
    Being reference types in C#, a string can also be null (which is also different to it being empty), hence the String.IsNullOrEmpty method. Either way, it's still ugly. :-)



  • @dhromed said:

    @skippy said:
    @KattMan said:

    Personally I prefer the idea of one point of return so I would write the following:

    I tend to follow this as well, but as some deep logic can get complicated it can really take away from readability, especially in parsing functions.  You end up with a lot of if/else's that cause clutter.

    Each method has their appropriate place.  I've been in countless arguments with people about this.

    Not meant as a "You are wrong!"  but as an addition to this single-point discussion:

    Return ends the function. Forcing a "retval" variable (I hate that name. I really do. That's personal.) doe not end the function. It forces extra logic.

    The readability is either the same, or less for single-exit functions.

    I agree retval is not good, but this is pseudo-code.

    As for single return points, I will break this rule on occasion (we always break our own rules don't we) usually in the case where a certain value for a parameter causes the entire function to be a moot point.  In that case I will check the value and inside that if() I will immediately return. This is usually right at the beginning of a function so it won't be missed as a return point and commented as such since it breaks my standard rule.

    Personally larger if blocks with processing within them can be missed as a return point while reading code.  I will agree that the same can be said for the return value being set, but it is much easier to see that value being returned and then set a watch for when it changes rather then trying to read through a overly verbose function to see if there is a return I missed.

    Of course if your functions are overly verbose, think about splitting them up. 



  • @XIU said:

    @KattMan said:

    Hmm instantiation separate from declaration, I actually like that attitude.  This way you can declare a variable then instantiate within the try block.  Why is this important?  Because instantiation can actually cause errors.  If you have objects that require Dispose() then you want to call Dispose() even if there is an error, so you call it in the Finally block.  The only way to make sure you can dispose is if the object is declared.  If it is at least declared you can check to see if it is NULL and if not then Dispose().  Since this is a common practice I split my instantiation from my declaration for everything.



    But it isn't needed, they made the using statement for that

    using (SqlConnection conn = new SqlConnection("some connection string"))
    {
      // You can even throw an exception here yourself and the conn will STILL be disposed
    }

    No need to call Dispose yourself, no need to seperate declaration, no need to check if conn isn't null

    About the ToString(), they are called on a string :| which means it just returns itself.

    You are falling into the trap I see so often in this argument.  You are thinking to narrowly.  Think about two variables being declared, the first passes and gets instantiated, the second fails and only cleans up itself, the first is left hanging and dispose is not called for it.  If we only ever worked with one object, your proposed method would always work. 



  • @KattMan said:

    @XIU said:
    @KattMan said:

    Hmm instantiation separate from declaration, I actually like that attitude.  This way you can declare a variable then instantiate within the try block.  Why is this important?  Because instantiation can actually cause errors.  If you have objects that require Dispose() then you want to call Dispose() even if there is an error, so you call it in the Finally block.  The only way to make sure you can dispose is if the object is declared.  If it is at least declared you can check to see if it is NULL and if not then Dispose().  Since this is a common practice I split my instantiation from my declaration for everything.



    But it isn't needed, they made the using statement for that

    using (SqlConnection conn = new SqlConnection("some connection string"))
    {
      // You can even throw an exception here yourself and the conn will STILL be disposed
    }

    No need to call Dispose yourself, no need to seperate declaration, no need to check if conn isn't null

    About the ToString(), they are called on a string :| which means it just returns itself.

    You are falling into the trap I see so often in this argument.  You are thinking to narrowly.  Think about two variables being declared, the first passes and gets instantiated, the second fails and only cleans up itself, the first is left hanging and dispose is not called for it.  If we only ever worked with one object, your proposed method would always work. 

    Except you can nest using blocks, and at least for VB.NET (not sure on C#) you can also declare and init multiple variables with one using block. Correct me if I'm wrong, but with either method, all will be disposed properly.

    So you could do:

    using (SqlConnection conn = new SqlConnection("some connection string")) {
        conn.Open();
        using (SqlCommand cmd = new SqlCommand("SELECT * FROM table", conn)) {
            using (SqlDataReader rd = cmd.ExecuteReader()) {
                throw new SomeException(); // Blow up. All 3 sql objects get disposed.
            }
        }
    }


  • About returns, I always try to have my functions return as soon as I can, especially if it is error condition. So, bunch of returns in the beginning, few in the middle, and maybe one at the end. This has additional benefit of forcing me to write functions that can exit at any point. RAII, streamlined logic and properly broken out code follows from here. Helps readability too.

    Having to use additional variable as a flag or return value carrier is a personal affront to me :).






  • @aquanight said:

    @KattMan said:

    You are falling into the trap I see so often in this argument.  You are thinking to narrowly.  Think about two variables being declared, the first passes and gets instantiated, the second fails and only cleans up itself, the first is left hanging and dispose is not called for it.  If we only ever worked with one object, your proposed method would always work. 

    Except you can nest using blocks, and at least for VB.NET (not sure on C#) you can also declare and init multiple variables with one using block. Correct me if I'm wrong, but with either method, all will be disposed properly.

    So you could do:

    using (SqlConnection conn = new SqlConnection("some connection string")) {
    conn.Open();
    using (SqlCommand cmd = new SqlCommand("SELECT * FROM table", conn)) {
    using (SqlDataReader rd = cmd.ExecuteReader()) {
    throw new SomeException(); // Blow up. All 3 sql objects get disposed.
    }
    }
    }

    Yes you can nest, but now think about readability.

    Go five variables, in the case of pulling data to retrieve an image from a database.  You start pushing your code off the edge of the screen.  You also make it easy to miss which variable is declared where.  Also if you look at your code, the exception generated by SomeException is thrown only if the third instantiation fails.  The other variables will throw the default if they fail.  In order to get SomeException to be called it needs to be thrown from the code within each using block.  If your SqlConnection object fails you will not get opened, you will not try to create the others, and you will not have SomeException() called.

    To account for all of this you would have to do the following, notice the nesting levels as they crawl deeper:

                using (SqlConnection conn =    new    SqlConnection("some    connection string")){
                    try
                    {
                        using (SqlCommand cmd =    new    SqlCommand("SELECT * FROM table", conn))
                        {
                            try
                            {
                                using (SqlDataReader rd    = cmd.ExecuteReader())
                                {
                                    try
                                    {
                                        //Some code
                                    }    
                                    catch
                                    {
                                        throw new SomeException(); // Blow up. All 3    sql    objects    get    disposed.          
                                    }
                                }
                            }
                            catch
                            {
                                throw new SomeException(); // Blow up. Top 2    sql    objects    get    disposed.          
                            }
                        }
                    }
                    catch
                    {
                        throw new SomeException(); // Blow up. Top 1    sql    objects    get    disposed.    
                    }
                }

     

    Could you format your code to account for all this, yes, and it is easily done.  In the end this comes down to what standards are you following.  I will not use Using because of the issues above. 

    Catching all this using my method provides for much more readable code as follows:

                SqlConnection conn;
                SqlCommand cmd;
                SqlDataReader rd;

                try
                {
                    conn =    new    SqlConnection("some    connection string");
                    cmd =    new    SqlCommand("SELECT * FROM table", conn);
                    rd    = cmd.ExecuteReader();
                    //Some code
                }
                catch
                {
                    if(rd != null)
                        rd.Dispose();
                    if(cmd != null)
                        cmd.Dispose();
                    if(conn != null)
                        conn.Dispose();
                    throw new SomeException();
                }

    Now seriously, which one would you prefer to read? 



  • I'm going to have to edit that, because the try doesn't work around a Using...

    using (SqlConnection conn =    new    SqlConnection("some    connection string")){
                        using (SqlCommand cmd =    new    SqlCommand("SELECT * FROM table", conn))
                        {
                                using (SqlDataReader rd    = cmd.ExecuteReader())
                                {
                                    try
                                    {
                                        //Some code
                                    }    
                                    catch
                                    {
                                        throw new SomeException(); // Blow up. All 3    sql    objects    get    disposed, but only because we are leaving the using blocks
                                    }
                            }
                        }
                }

     

    Now tell me how to raise an exception if you simply can't get a connection.  and what happens if the command errored within a transaction, did it rollback or not?  Using does not look at the ITransaction interface so it doesn't clean up well afterwards.

    Once again, my method still allows you to check the type of exception and is more readable.
     



  • I may be missing the point of the discussion here... (Fat chance, since I only looked at the code)

    But it seems to me that you guys are over complicating data access here...



  • @Ice^^Heat said:

    I may be missing the point of the discussion here...

    There isn't one any more.  It's descended into a religious war about coding-style issues.

     

    May as well finish it though:  my one's best.  There.  That's definitive.  You can all go home now.



  • @DaveK said:

    @Ice^^Heat said:

    I may be missing the point of the discussion here...

    There isn't one any more.  It's descended into a religious war about coding-style issues.

     

    May as well finish it though:  my one's best.  There.  That's definitive.  You can all go home now.

    Oh fudge, you win.  I concede. 


Log in to reply