Posted by: Terry Nederveld | July 29, 2011

MvcContrib DataList fix

While working with the MvcContrib DataList the other day I noticed a problem. Here is how I found the issue. I had a collection that contained 3 items and everything was hunky-dory. I had my view setup with the following code for the DataList.


@Html.DataList( Model.GroupBy( g => g.CategoryTitle ) ).NumberOfColumns( 3 ).CellTemplate( x =>
{ Write( new MvcHtmlString( "<ul class="links"><p>" + x.Key + "</p>" + x.Each( @<li><a href="@(item.Item.LinkResourceType.ToLower().Equals( "report" ) ? Url.Action( "Viewer", new { Id = item.Item.LinkId } ) : item.Item.LinkUrl)" @(item.Item.OpenInNewWindow ? "target="_blank"" : "")>@Html.Raw( item.Item.LinkContent )</a></li> ) + "</ul>" ) ); } ).CellAttributes( valign => "top", width => "33%" )

Here are a few finer details about of the code above.

  1. The DataList control uses the table Html element to lay its self out.
  2. It is set to 3 columns.
  3. Since the repeat direction is not set, it will use vertical repeating (the default).
  4. Since we don’t want the default table alignment of middle, each cell in the table will have the “valign=”top” width=”33%”” attributes appended to each <td>.
  5. .Each() is a HelperResult extension. It was detailed by Phil Haack on his blog at http://haacked.com/archive/2011/04/14/a-better-razor-foreach-loop.aspx

Life was good and my items were showing up fine and dandy, until I added a fourth item to my collection. And I received:

Index was out of range. Must be non-negative and less than the size of the collection.

I added another item and the message went away. Then it came back again this time it was when I added the seventh item and again on the tenth item. I was seeing a pattern, so forked the CodePlex project and attempted to clone the repository locally in order to fix the issue. Turns out I could load the software to access the projects but when it came to actually using it to connect to the repository our proxy, firewall or something was blocking me from accessing the fork. So I ended up just downloading the entire source as a ZIP from my fork and proceeded to debug it.

I quickly came to the section that’s used when the repeat direction is vertical. Below is what the code originally looked like.

 private void RenderVertical(int repeatColumns, IList <T> items)
 {
 	int rows = CalculateAmountOfRows(items.Count, repeatColumns);
 	int columns = repeatColumns;

 	int i = 0;

 	for (int row = 0; row < rows; row++)
 	{
 		Write("<tr>" );
 		for (int column = 0; column < columns; column++)
 		{
 			if (i + 1 <= items.Count)
 			{
 				if (column == 0)
 					RenderCell(items[row]);
 				else
 					RenderCell(items[((column * rows) + row)]);
 			}
 			else
 				RenderNoItemCell();
 			i++;
 		}
 		Write("</tr>" );
 	}
 }

Ok, so first up it calls the CalculateAmountOfRows method to get the number of rows it needs. Which for a collection that contains 4 items and number of columns set to 3, it will return 2 rows. Trust me it works properly. From there it starts building the output. Can you see the problems? I’ll give you a hint 🙂

Still having a hard time seeing what the issue is? Let’s walk through the loops and do the math. Remember we have a collection that has 4 items in it so we will need two rows. Below is a visual representation of what the DataList method above will output. Did you see the problems? Look closely at Row 0, Column 2. Is the “i+1” code less than or equal the items.Count? Yes it is, so it tries to render the cell. When it renders the cell it tries to do so with items[4], Does the collection have an item at an index of 4? You are correct, no it doesn’t. In a zero-based collection the highest index for our collection would be 3. And this is where the code throws the lovely message we saw above. So we throw a quick fix in that checks to make sure that the index we are looking for is not higher than the count of items in our collection. Tada! It works or so it seems. If you run the new code you would see the following cells rendered (0,0), (0,1) and (1,0). Where is the fourth item? Well if we step back and forgo the obvious fix earlier you will notice that even if the code wouldn’t throw the out-of-range error, it wouldn’t ever show the forth item because the “i + 1” at (1,1) would be 5 and the test against the item.Count would fail and it would run the RenderNoItemCell code. So now that we found the issues, how do we fix it? Well that was actually pretty easy. I just needed to look at the index to make sure it was within the bounds of the collection before attempting to render the cell. And when actually laying out how it works I realized that we didn’t need to +1 the “i” and check against the items.Count. The desired behavior only needs the “i” checked. Here is what I changed the method above to in order to fix the issue. 

 private void RenderVertical(int repeatColumns, IList <T> items)
 {
 	int rows = CalculateAmountOfRows(items.Count, repeatColumns);
 	int columns = repeatColumns;

 	int i = 0;

 	for (int row = 0; row < rows; row++)
 	{
 		Write("<tr>" );
 		for (int column = 0; column < columns; column++)
 		{
             if (i <= items.Count)
             {
                 int index = (column == 0 ? row : ((column * rows) + row));
                 if (index < items.Count)
                     RenderCell(items[index]);
                 else
                     RenderNoItemCell();
             }
             else
                 RenderNoItemCell();
 			i++;
 		}
 		Write("</tr>" );
 	}
 }

I have updated my forked version of MvcContrib and will submit a pull request for them to bring in the updated code.

Thanks, until next time… Happy Coding.

Advertisements

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

Categories

%d bloggers like this: