CuriOddities, part 3 [Kit George]

CuriOddities, part 3 [Kit George]

  • Comments 6

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.

Public Class UnexpectedBehaviors

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

               "Summer", "Autumn", "Winter"}

   Public Shared Sub PrintSeasons()

      For Each season As String In ValidSeasons

         Console.WriteLine(season)

      Next

   End Sub

End Class

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

  • 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() = {...})
  • I have to confess, I stole your word. I actually used the term Curioddity in an email at work today.
  • 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 :)
  • Don, more than happy for you to steal the word, I hope it went down well ;-).
  • 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();
    }
  • PingBack from http://quickdietsite.info/story.php?id=13353

Page 1 of 1 (6 items)