HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

ASP MVC Controller Unit Test

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
mvccontrollertestaspunit

Problem

So I've been writing a unit test using Rhino Mocks to a controller in my ASP MVC application. The test passes, but I'm interested in your opinion and if there's something I should be doing differently or what it could be.

Here's my test method:

[TestMethod]
    public void TestSlideView()
    {
        // Arrange
        var repositoryMock = MockRepository.GenerateMock();
        var controller = new SlideController(repositoryMock);
        var expectedSlides = new List();
        expectedSlides.Add(new SlideModel
        {
            Id = "id"
        });
        repositoryMock.Stub(x => x.GetSlides("id")).Return(expectedSlides);

        // Act
        var actualView = controller.Slide("id") as ViewResult;
        var actualData = actualView.Model;

        // Assert
        Assert.IsNotNull(actualView);
        Assert.IsNotNull(actualData);
        Assert.AreEqual("Slide", actualView.ViewName);
    }


Here's the controller under test:

public ActionResult Slide(string slideid)
    {
        var slides = slideRepository.GetSlides(slideid);

        return View("Slide", slides);
    }


The repository the controller uses is here:

```
public interface ISlideRepository
{
List GetSlides(string id);
}

public class SlideRepository : ISlideRepository
{
public SlideModel Slides { get; set; }
public List SlideList { get; set; }

public List GetSlides(string id)
{
string cs = dbPath;
string slideid = id;

using (SQLiteConnection con = new SQLiteConnection(cs))
{
var listOfSlides = new List();
string stm = "SELECT * FROM Slide WHERE ID = " + slideid;
con.Open();

using (SQLiteCommand cmd = new SQLiteCommand(stm, con))
{
using (SQLiteDataReader rdr = cmd.ExecuteReader())
{
while (rdr.Read())
{
listOfSlides.Add(new SlideModel
{

Solution

The string "id" is used three times in TestSlideView(): make it a const string instead.

Same for "Slide", as used in TestSlideView() and Slide(string slideid).

Don't name something SlideList, name it Slides. And yes, I know you named it SlideList because you already have a property called Slides, but Slides isn't an appropriate name for a single SlideModel anyway.

Even worse: listOfSlides.

Your variable names don't tell me what they contain: cs, stm, etc. I can figure out that "cs" means "connectionString", but I shouldn't be wasting effort on such things.

Also, why bother assigning dbPath to cs? Why not simply use dbPath (and perhaps rename that to connectionString)?

Same for assigning id to slideid -- which should be named slideId since it is a compound word. Matter of fact, why isn't the parameter named slideId in the first place?

"Danger, Will Robinson!" You code is now vulnerable to SQL injection:

"SELECT * FROM Slide WHERE ID = " + slideid;


Use the Parameters collection instead.

Also: looping through a SQLiteDataReader seems quaint to me. Why not store the result in a DataTable and work with that? Or even better: why not work with an ORM like NHibernate or Entity Framework?

Code Snippets

"SELECT * FROM Slide WHERE ID = " + slideid;

Context

StackExchange Code Review Q#106937, answer score: 3

Revisions (0)

No revisions yet.