Why you shouldn't mix your View logic with your Controller logic.

I was called up to say the admin pages for the supplier gallery had stopped working, all new images wouldn't upload. After searching through the javascript (which usually causes issues here) I discovered this juicy morsel in the code behind....


If PostID = 0 And btnUpload.Text.Trim.Equals("Upload Photo") Then
  AddPostToMediaGallery()
  Response.Redirect("MediaGallery.aspx?msg=add")
Else
  UpdateMediaGalleryPost()
  Response.Redirect("MediaGallery.aspx?msg=update")
End If

That reads "If the post has no ID, and the button to upload the photo was named 'Upload Photo' then Add the photo to the media gallery, otherwise update the existing media gallery photo."

Firstly, that's too much information to read in a few lines of code, so let's refactor it a little...



Public Enum ImageActionType
  AddImage
  UpdateImage
End Enum

Public Readonly Property ActionType As ImageActionType
  Get
    If PostID = 0 And btnUpload.Text.Trim.Equals("Upload Photo") Then
      Return AddImage
    Else
      Return UpdateImage
    End If
  End Get
End Property

...


If ActionType = ImageActionType.AddImage Then
  AddPostToMediaGallery()
  Response.Redirect("MediaGallery.aspx?msg=add")
Else
  UpdateMediaGalleryPost()
  Response.Redirect("MediaGallery.aspx?msg=update")
End If

There, we've moved the nasty bit of code into a much easier to read property, but the problem remains. So next I check both the properties that are in the page. PostID is 0, that's as expected... this is a new Post, so we don't have an ID. I'm confused why "0" is representing the absence of an integer. What if there was a valid post with the ID of 0? We should make PostID nullable.


Private _postID As Nullable(Of Integer)
Public Property PostID() As Nullable(Of Integer)
  Get
    Return _postID
  End Get
  Set(ByVal value As Nullable(Of Integer))
    _postID = value
  End Set
End Property

Great, things are looking better already, but we haven't fixed the cause of the issue. Let me check the value of the buttons' text....

Upload your Photo

Here it is! Some designer has come in and changed the text of the button to "Upload your photo". That should be allowed. Why would anyone expect that changing the text of a button changes the behaviour too? I can see in the code that if the user is performing an update, it changes the label to another wording. This is the view, but the controller relies on the contents of the view to function. This is really bad. I checked all the code stubs and this is simply over-zealous programming. On no account is PostID ever set to anything but 0 for a new photo. Even if we did rely on a parameter in the page, we definitely shouldn't be storing it on the view.

So i've gone into the code and removed that condition, now it looks much nicer.


    Public ReadOnly Property ActionType() As ImageActionType
        Get
            If Not PostID.HasValue Then
                Return ImageActionType.AddImage
            Else
                Return ImageActionType.UpdateImage
            End If
        End Get
    End Property