Recursion in C# Properties



  • Yesterday I have discovered a whole new world of WTFs in my predecessors code... Recursion in C# Properties!

    Basically what this piece of code (edited by me for the sake of brevity) does, it converts a 1-based integer containing the column number in an Excel spreasheet to the appropriate column name (Excel columns have names like A or AB, basically they are base-26 numbers using letters as digits)

     

    <link href="http://www.thecomplex.plus.com/styles/SyntaxHighlighter.css" mce_href="http://www.thecomplex.plus.com/styles/SyntaxHighlighter.css" rel="stylesheet" type="text/css">
    1. using System;  
    2.   
    3. public class ExcelCell  
    4. {  
    5.     private int columnNumber;  
    6.     private const string ABC = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";  
    7.   
    8.     public string ColumnName  
    9.     {  
    10.         get  
    11.         {  
    12.             if (columnNumber / 26 == 0)  
    13.             {  
    14.                 return ABC[columnNumber - 1].ToString();  
    15.             }  
    16.             else  
    17.             {  
    18.                 int x = columnNumber;  
    19.                 columnNumber = columnNumber / 26;  
    20.                 string rest_result = ColumnName;  
    21.                 columnNumber = x % 26;  
    22.                 string this_result = ColumnName;  
    23.                 columnNumber = x;  
    24.                 return this_result + rest_result;  
    25.             }  
    26.         }  
    27.     }  
    28. }  

    Now using recursion for this task might or might not be a good idea, but this is definitely not something you do in a C# property.

    For people who don't know C# (from MSDN):

    Properties are members that provide a flexible mechanism to read, write, or compute the values of private fields. Properties can be used as if they are public data members, but they are actually special methods called accessors. This enables data to be accessed easily and still helps promote the safety and flexibility of methods.



  •  Actually, this reminds me of an infuriating feature of Visual Studio and the C# compiler. If I write a property like this before creating the local variable:

     public String ColumnName

    {

        get { return columnName; } 

     }

    Visual Studio helpfully turns that into:

    get { return ColumnName; }

    If I don't notice this and then create my local variable and run the application, all of a sudden I start seeing StackOverflowExceptions and have to step through with a debugger to find the problem. I always thought that the compiler should not allow a property to call itself (I never thought that anyone would want to create a recursive property) but perhaps even a warning would be useful here. 

     



  • A couple of things:

    1. It does not correctly handle the 1-based indexing and will bork on any column numbers that are divisible by 26 with an IndexOutOfRangeException.
    2. It uses string concatenation.

    I don't see anything from the MSDN spec that disallows recursive properties. Keep in mind that properties are actually methods, get_ColumnName() in this case.

    The best way to write it would be:

    public class ExcelCell
    {
        private int columnNumber;
        private const string ABC = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";

        public string ColumnName
        {
            get
            {
                StringBuilder sb = new StringBuilder();
                MakeName(sb, columnNumber - 1);
                return sb.ToString();
            }
        }
        private static void MakeName(StringBuilder name, int number)
        {
            int d = number / ABC.Length;
            int m = number % ABC.Length;

            if (d > 0)
                MakeName(name, d);

            name.Append(ABC[m]);
        }
    }



  • @Phill said:

    If I don't notice this and then create my local variable and run the application, all of a sudden I start seeing StackOverflowExceptions and have to step through with a debugger to find the problem.
    I once wrote two different property getters (in Java: methods like any other, except their names begin in 'get') that called each other. It was a fun mistake to make, once.



  • @SlyEcho said:

    It does not correctly handle the 1-based indexing and will bork on any column numbers that are divisible by 26 with an IndexOutOfRangeException.

    Good point.

    @SlyEcho said:

    It uses string concatenation.

    You're right, but I think that's a relatively minor WTF here. StringBuilder FTW.

    @SlyEcho said:

    I don't see anything from the MSDN spec that disallows recursive properties. Keep in mind that properties are actually methods, get_ColumnName() in this case.

    No, it does not disallow it. But I think you agree with me that recursion in a property is a Bad Idea™. Just think about it: you cannot pass parameters to properties (unless it is an indexer, but that's not a property anyway). This basically forces you to use global variables to hold data. Now this is a bad thing for countless reasons, thread safety during an inherently read opeartion is just a one example. Maintainability and readability is an other one.



  • @DrJokepu said:

    No, it does not disallow it. But I think you agree with me that recursion in a property is a Bad Idea™. Just think about it: you cannot pass parameters to properties (unless it is an indexer, but that's not a property anyway). This basically forces you to use global variables to hold data. Now this is a bad thing for countless reasons, thread safety during an inherently read opeartion is just a one example. Maintainability and readability is an other one.

    Yeah, but for simple recursions (no parameters), it would be OK. Like:

    public class Tree
    {
        List<TREE> Children;

        public int ChildCount { get { return Children != null ? Children.Count : 0; } }

        public int TotalChildCount
        {
            get
            {
                int sum = 0;

                if (ChildCount > 0)
                {
                    for (int i = 0; i < ChildCount; i++) // don't use foreach in recursion.
                    {
                        sum += Children[i].TotalChildCount; // recursion here
                    }
                    return sum;
                }

                return sum + ChildCount;
            }
            
        }
    }

    (I didn't test this code, it could have bugs in it.)



  • @DrJokepu said:

  •                 int x = columnNumber;  
  •                 columnNumber = columnNumber / 26;  
  •                 string rest_result = ColumnName;  
  •                 columnNumber = x % 26;  
  •                 string this_result = ColumnName;  
  •                 columnNumber = x;  
  •                 return this_result + rest_result;  
  • What the fuck. Parameter passing by means of overwriting and restoring a global variable? That's insane. Are getters automatically protected with mutexes and made thread safe, or will this blow up big time when the property is used by two different threads?




  • Well, I may imagine a situation where a property could be represented using recursion...
    For example:

    class MultipleFileSearcher
    {
    
    //......
    
       public SearchResult Result
      {
        get
        {
           if( IsResultInCurrentFile() )
           {
              return GetResultInCurrentFile();
           }
           else
           {
              AdvanceToNextFile();
              return Result;
           }
        }
      }
    }
    

    However, I would certainly extract the logic into a private method, just to avoid confusion.

    As for the original code, it's a huge WTF indeed. Still, the coder knows recursion; it should be possible to teach him to use it sparingly. The character lookup table is also a good sign - it's not a 'switch' statement after all. So I wouldn't call the guy hopeless.



  • @SlyEcho said:

    The best way to write it would be:

    public class ExcelCell
    {
        private int columnNumber;
        private const string ABC = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";

        public string ColumnName
        {
            get
            {
                StringBuilder sb = new StringBuilder();
                MakeName(sb, columnNumber - 1);
                return sb.ToString();
            }
        }
        private static void MakeName(StringBuilder name, int number)
        {
            int d = number / ABC.Length;
            int m = number % ABC.Length;

            if (d > 0)
                MakeName(name, d);

            name.Append(ABC[m]);
        }
    }

    The best way to write it would be to not use fucking recursion to handle base conversion.  Seriously, I think 95% of TDWTF readers are probably WTF factories themselves. 



  • @morbiuswilters said:

    The best way to write it would be to not use fucking recursion to handle base conversion. Seriously, I think 95% of TDWTF readers are probably WTF factories themselves.

    OK, how about making a better WTF than the original one? Here's my almost-one-liner that should be taken out and shot:

    public string ColumnName
    {
      get
      {
        return ( (number / ABC.Length > 0 ) ? 
                         ABC[number / ABC.Length - 1] : '') 
               + ABC[number % ABC.Length - 1];
      }
    }
    

    All off-by-one and compilation errors are intended for the reader's amusement.



  • @Phill said:

     Actually, this reminds me of an infuriating feature of Visual Studio and the C# compiler. If I write a property like this before creating the local variable:

     public String ColumnName

    {

        get { return columnName; } 

     }

    Visual Studio helpfully turns that into:

    get { return ColumnName; }

    If I don't notice this and then create my local variable and run the application, all of a sudden I start seeing StackOverflowExceptions and have to step through with a debugger to find the problem. I always thought that the compiler should not allow a property to call itself (I never thought that anyone would want to create a recursive property) but perhaps even a warning would be useful here. 

     

     

     

    TRWTF is your naming convention. How would you code in a case-insensitive language?



  • @Huf Lungdung said:

    TRWTF is your naming convention. How would you code in a case-insensitive language?
     

    Are you suggesting your C# naming conventions should be different because some languages are case insensitive??



  • @SlyEcho said:

    The best way to write it would be:

    public class ExcelCell
    {
        private int columnNumber;
        private const string ABC = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";

        public string ColumnName
        {
            get
            {
                StringBuilder sb = new StringBuilder();
                MakeName(sb, columnNumber - 1);
                return sb.ToString();
            }
        }
        private static void MakeName(StringBuilder name, int number)
        {
            int d = number / ABC.Length;
            int m = number % ABC.Length;

            if (d > 0)
                MakeName(name, d);

            name.Append(ABC[m]);
        }
    }

    27 should turn into AA, but your code turns it into BA.

    This ought to work:

    public class ExcelCell
    {
        private int columnNumber;
        private const string ABC = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";

        public string ColumnName
        {
            get
            {
                StringBuilder sb = new StringBuilder();
                MakeName(sb, columnNumber);
                return sb.ToString();
            }
        }
        private static void MakeName(StringBuilder name, int number)
        {
            if (--number >= 0) {
                int d = number / ABC.Length;
                int m = number % ABC.Length;

                MakeName(name, d);
                name.Append(ABC[m]);
            }
        }
    }



  • @MasterPlanSoftware said:

    @Huf Lungdung said:

    TRWTF is your naming convention. How would you code in a case-insensitive language?
     

    Are you suggesting your C# naming conventions should be different because some languages are case insensitive??


      Microsoft suggests it.  The .NET Framework CLS does not dictate that languages must be case-sensitive, and for this reason any public members of a type must be named in a manner that allows a case-sensitive language to use them:

     http://msdn.microsoft.com/en-us/library/04xykw57(VS.71).aspx

    Do not use names that require case sensitivity. Components must be
    fully usable from both case-sensitive and case-insensitive languages.
    Case-insensitive languages cannot distinguish between two names within
    the same context that differ only by case. Therefore, you must avoid
    this situation in the components or classes that you create.

    You can do what you want with your private members, but at least for public members you have to play as if your language isn't case-sensitive to remain CLS-compliant.

     That said, the thought of using recursive code from properties is a horrible WTF.  Property access should never throw a property or incur a delay; recursion can cause stack overflows or take some time to run.  This is a situation that calls for Get/Set methods, rather than "clever" use of recursion.


     



  • @yme said:

    @SlyEcho said:

    The best way to write it would be:

    27 should turn into AA, but your code turns it into BA.

    I realized that later, after I opened Excel. It still has the "problem" of being recursive. I have written an improved version of the conversion function:

    private const string ABC = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
    private static readonly int AbcMax = (int)Math.Ceiling(Math.Log(int.MaxValue, ABC.Length));
    

    public static string MakeName(int num)
    {

    char&#91;&#93; buffer = new char[AbcMax];
    int i;
    
    for (i = buffer.Length - 1; i &gt;= 0; i--)
    {
        buffer[i] = ABC[num % ABC.Length];
        if ((num = num / ABC.Length - 1) &lt; 0) break;
    }
    
    return new string(buffer, i, buffer.Length - i);
    

    }



  • @DrJokepu said:

    (...)What the fuck. Parameter passing by means of overwriting and restoring a global variable? That's insane. Are getters automatically protected with mutexes and made thread safe, or will this blow up big time when the property is used by two different threads?


    They're not thread safe by default.



  • @Sitten Spynne said:

     That said, the thought of using recursive code from properties is a horrible WTF.  Property access should never throw a property or incur a delay; recursion can cause stack overflows or take some time to run.  This is a situation that calls for Get/Set methods, rather than "clever" use of recursion.

     

    Another good reason to avoid the above is that microsoft's .net debugger will evalutate and display properties in the 'locals' grid whenever you look at it.  If you stack overflow a property get, it stalls the debugger and stuffs up your ability to see values or use immediate evaluation.



  • @Sitten Spynne said:

    Microsoft suggests it. 
     

    C# standards:

    http://www.irritatedvowel.com/Programming/Standards.aspx

    I agree you shouldn't make something differ by case only, but in the code I was replying to, PhillS is talking about a private member with a public property to access it. There is nothing wrong with this.

    If it were a public member and a public property I could see your argument.

     

    I agree I wouldn't name things like this either, but there is nothing truly 'wrong' with what PhillS did.



  • [quote user="Renan "C#" Sousa"]

    @DrJokepu said:

    (...)What the fuck. Parameter passing by means of overwriting and restoring a global variable? That's insane. Are getters automatically protected with mutexes and made thread safe, or will this blow up big time when the property is used by two different threads?


    They're not thread safe by default.

    [/quote] 

    You credited Nandrius's post to me. The lines above weren't written by me. I had catholic education and I would never use the 'f' word.



  • @DrJokepu said:

    [quote user="Renan "C#" Sousa"]

    @DrJokepu said:

    (...)What the fuck. Parameter passing by means of overwriting and restoring a global variable? That's insane. Are getters automatically protected with mutexes and made thread safe, or will this blow up big time when the property is used by two different threads?


    They're not thread safe by default.

     

    You credited Nandrius's post to me. The lines above weren't written by me. I had catholic education and I would never use the 'f' word.

    [/quote]

    Sorry =( I commited a mistake while removing a quote block.

    I was talking to a friend of mine about recursion inside a get block. I came to the conclusion that having someone catch a StackOverflowException while trying to serialize an object is one of those few things that can make a tough man cry.



  • @SlyEcho said:


    private static readonly int AbcMax = (int)Math.Ceiling(Math.Log(int.MaxValue, ABC.Length));
    ...
    if ((num = num / ABC.Length - 1) < 0) break;

    My eyes!


    I mean, it's probably correct (if you took the time to actually test this thing; otherwise it's certainly not), but the task was to generate a TWO-LETTER combination, not a generic one.


    And is changing the value of function input (bonus points for making it in the head of the 'if' operator) considered harmful only by me?


    Sorry if I missed the <irony> tags around the words 'improved version'.



  • @IMil said:

    @SlyEcho said:
    private static readonly int AbcMax = (int)Math.Ceiling(Math.Log(int.MaxValue, ABC.Length));
    ...
    if ((num = num / ABC.Length - 1) 
    My eyes!

    I mean, it's probably correct (if you took the time to actually test this thing; otherwise it's certainly not), but the task was to generate a TWO-LETTER combination, not a generic one.

    Of course it's overkill, but if you're a programmer, you need to look further ahead and find a more generic solution (you don't always have time for this though). The original problem was finding Excel column names, and Excel 2007 has at least three letter columns (It took some time to scroll that far).

    @IMil said:

    And is changing the value of function input (bonus points for making it in the head of the 'if' operator) considered harmful only by me?

    It may seem a little unnatural, but perfectly valid since it's not a reference, only a copy on the stack.

    @IMil said:

    Sorry if I missed the <irony> tags around the words 'improved version'.

    The "improvement" here was to make it not recursive. Lots of people seem to be afraid on stack overflows, but in reality that only happens when your recursions don't terminate properly. The recursive function I wrote originally would only need a maximum of 7 recursions for any positive Int32, for smaller values, even less.



  • @SlyEcho said:

    Of course it's overkill, but if you're a programmer, you need to look further ahead and find a more generic solution (you don't always have time for this though). The original problem was finding Excel column names, and Excel 2007 has at least three letter columns (It took some time to scroll that far).

    Sorry, my mistake. I didn't work with Excel that much and thought ZZ was the last one. Then recursion IS the way to go, just not in the crazy way of the OP.

    @SlyEcho said:

    @IMil said:

    And is changing the value of function input (bonus points for making it in the head of the 'if' operator) considered harmful only by me?

    It may seem a little unnatural, but perfectly valid since it's not a reference, only a copy on the stack.

    It's valid, just confusing. For example, when I enter the debugger, I usually may hover the cursor over the function parameters and see their values. But in your case the initial value would be lost; I'd have step back and examine the caller and that's not always possible.

    @SlyEcho said:

    The "improvement" here was to make it not recursive. Lots of people seem to be afraid on stack overflows, but in reality that only happens when your recursions don't terminate properly. The recursive function I wrote originally would only need a maximum of 7 recursions for any positive Int32, for smaller values, even less.

    Well, that's what I meant by <irony>. A stack overflow is the last thing to worry about with a strictly limited recursion depth. If there's an error that leads to an infinite recursion, the same error in the iterative code would lead to an infinite loop. So I think the recursive version was more readable and thus better (were it correct). I'd even remove the whole StringBuilder thing and just concatenate the strings - for a small number of strings this will be unimportant.


Log in to reply