CuriOddities, part 3 [Kit George]

Time for another interesting snippet, in which the code itself doesn’t appear bad at first glance, but again: the coder seems to be making an assumption about the way they have constructed the code, which is incorrect. And in fact, it’s not an appropriate design. And yes, this gets the fxcop nod, but spot the problem before trying that out.


Class UnexpectedBehaviors

   Public Shared ReadOnly ValidSeasons As String() = {“Spring”, _

               “Summer”, “Autumn”, “Winter”}

   Public Shared Sub PrintSeasons()

      For Each season As String In ValidSeasons



   End Sub



The question with this one is: what’s the assumption, and what’s bad about it?

Comments (6)

  1. Atif Aziz says:

    There are a number of problems here. First of all, the class should have a private constructor to disallow instances since it has only shared members. ValidSeasons is public, but there is no need for it to be. This leads to the final issue, which is the poor assumption that the array contents cannot be modified. The ReadOnly modifier only renders the ValidSeasons reference read-only, but not the contents of the array it points to. So no one can change the particular array instance ValidSeasons points to, but they can party on its elements. The quickest way to fix this would be to do:

    Public Shared ReadOnly ValidSeasons As String() = ArrayList.ReadOnly(New String() = {…})

  2. Don Newman says:

    I have to confess, I stole your word. I actually used the term Curioddity in an email at work today.

  3. eden li says:

    Obviously it’s very easy for someone to make the assumption that ReadOnly extends into the array contents, but it’s not clear if this is something a compiler should worry about catching. It’s obviously very bad for API writers, but it seems like there could be a requirement for ReadOnly members whose internal contents should not be ReadOnly. I can’t think of any examples off the top of my head, but it seems like it would be better to err on the side of usability. Leaving things like this to FXCop and other tools seems like the best option.

    (BTW, Thanks for the link Kit 🙂

  4. Kit George [MSFT] says:

    Don, more than happy for you to steal the word, I hope it went down well ;-).

  5. Kit George [MSFT] says:

    Atif gave a good breakdown of the issues. The key focus is the problem with the assumption that ReadOnly is being assumed to affect each specific element in the array, and yet, that is not a contract it can enforce.

    If you find yourself in this situation, one suggested technique for solving is to include a Get method, that returns a new instance of the internal readonly array. Atif’s proposed solution is similar in nature.

    private static readonly char[] chars;

    public static char[] GetInvalidPathChars()


    return (char[]) chars.Clone();