In improve this we take a look at a reader submitted test, user interface, story or block of code and we try and improve it, without context, explaining what we did as we went.

In this issue we have with two tiny partials of code. It’s some ASPX/HTML madness

<div class="group-legend"><%=ViewData["Title"]%></div>
<p>
<label for="StreetAddress">Address:</label>&nbsp;
<textarea name="HomeAddress.StreetAddress" id="HomeAddress.StreetAddress" cols="20" rows="3" <%=!isEditable ? "readonly='readonly'":"" %>>
<%= Model.StreetAddress %>
</textarea>
</p>
<%if (isCurrentUser || isAdminUser) {%>
<p>
<label for="AltenateEmail">Personal Email:</label>
<span>
<textarea id="AltenateEmail" name="AltenateEmail" cols="20" rows="1" <%=!isEditable ? "readonly='readonly'":"" %>>
<%=Model.AltenateEmail%>
</textarea>
</span>
</p>
<%} %>
view raw 1.html hosted with ❤ by GitHub

The first thing we should do is try and remove the conditional logic and the editable properties out. This behaviour should be in the controller. The controller logic should be ‘If the user has insufficient privileges to edit but can see the record’, redirect the user to the view-only resource.

<%if (isCurrentUser || isAdminUser) {%>
...
<%=Model.AltenateEmail%>
...
<%} %>
view raw i4g2.aspx hosted with ❤ by GitHub

The goal of the conditional (isCurrentUser || isAdminUser) is to not show the personal email address unless a user is looking at their own account or an admin is looking. There are couple of ways we can handle this.

Keep the view logic.

If you want to keep the view logic, then the next step would be to collapse the conditional down to one branch.

//view
<%if (personalEmailIsVisible) {%>
...
<%=Model.AltenateEmail%>
...
<%} %>
view raw i4g3.aspx hosted with ❤ by GitHub

The personalEmailIsVisible property is set on the controller.

//controller
personalEmailIsVisible = false;
if (isCurrentUser || isAdminUser) {
personalEmailIsVisible = true;
}
view raw i4g4.aspx hosted with ❤ by GitHub

With fewer code paths our testing becomes simpler.

Remove the view logic and mask the property

Here change the view to always render the personal email address:

//view
...
<%=Model.AltenateEmail%>
...
view raw i4g5.aspx hosted with ❤ by GitHub

What we do in the controller is mask the value for users who should not see that data.

//controller
if (!isCurrentUser && !isAdminUser) {
Model.AltenateEmail = "*******";
}
view raw i4g6.aspx hosted with ❤ by GitHub

Remove the view logic and make a new domain object

I would trend towards making new domain objects as the view fractures with conditionals. Let’s call it a PublicPerson. There is no personal email address on this model so the conditional logic is no longer required. I would use an adapter to turn the PrivatePerson object into a PublicPerson. The challenge with object oriented programming is more logic you have, the muddier your domain model becomes.

Here is our view and controller

//view
...
<%=Model.AltenateEmail%>
...
//controller
publicPerson = personAdapter.adapt(privateModel);
view raw i4g7.aspx hosted with ❤ by GitHub

Minor Model Changes and some CSS

I would rename the model from Model to something that was more meaningful like Person. This changes Model.StreetAddress to Person.HomeAddress. It’s a bit mixed in places as there is HomeAddress.StreetAddress, StreetAddress, Model.StreetAddress. As there is only one line for the street and there appears to be only one address (home) all this relates to a person. so Person.HomeAddress is a good name.

I’d also make the AlternateEmail and PersonalEmail consistent. It should be one or the other, using both terms makes it feel like the domain model is not fully understood. I’d also remove the cols and rows properties and move them into CSS as height and width. The span elements in the second paragraph are probably there for formatting. Remove them and style them using CSS.

<div class="group-legend"><%=ViewData["Title"]%></div>
<p>
<label for="HomeAddress">Home Address:</label>&nbsp;
<textarea name="Person.HomeAddress" id="Person.HomeAddress">
<%= Person.HomeAddress %>
</textarea>
</p>
<p>
<label for="PersonalEmail">Personal Email:</label>
<textarea id="PersonalEmail" name="PersonalEmail">
<%=Person.PersonalEmail%>
</textarea>
</p>
view raw i4g8.aspx hosted with ❤ by GitHub

The Sucker Punch

The final improvement is easy to miss and by the longest margin: the most important. It’s the reason why this code snippet was put up for review. Neither the address or the email address have had the values encoded. This meant that arbitrary javascript was inserted into the fields and executed as the page is rendered. The fix is quite simple.

...
<%= Html.Encode(Person.HomeAddress) %>
...
<%= Html.Encode(Person.PersonalEmail) %>
...
view raw i4g9.aspx hosted with ❤ by GitHub

Do you have something you want improved? Send it to p2@thoughtworks.com.