# ASP MVC Controller Unit Test

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<ISlideRepository>();
var controller = new SlideController(repositoryMock);
var expectedSlides = new List<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<SlideModel> GetSlides(string id);
}

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

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

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

using (SQLiteCommand cmd = new SQLiteCommand(stm, con))
{
{
{
{
Id = rdr["ID"].ToString()
});
}

rdr.Close();
SlideList = listOfSlides;
}
}

con.Close();
}

return SlideList;
}
}


Any input is greatly appreciated!

• I like seeing code review questions about unit tests! It's an all-too-often ignored part of the code review process. – Greg Burghardt Oct 8 '15 at 12:57

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;


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?

• This was exactly what I was looking for! I'll make appropriate changes right away! – Khaine775 Oct 8 '15 at 13:24

Remarks on Unit Testing ,

1. I used nsubstitute and fluent assertion instead of Rhino mocks , It will make your test code more readable here is an example.

    ISlideRepository repository =Substitute.For<ISlideRepository>();
repository.GetSlides("Test").Returns("TestSlides");

var controller= new SlideController(repository);

var result= controller.Slide("Test") as ViewResult;

//Assert
result.Should().NotBeNull();
result.ViewName.Should().Be("Slide");


Remarks on Coding styles

1. Introduce a DAL helper layer which will be responsible of opening and closing connection and executing commands. There are lots of DAL helper layer is available
2. Write an extension method to get values from the data reader. Here is one dummy code

 listOfSlides.Add(new SlideModel
{
Id = rdr["ID"].ToString() //Not preferable
Id= rdr.Get<string>("ID") //Try doing this
});


If you dont find any extension method , let me know I can post it here.

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

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

using (SQLiteCommand cmd = new SQLiteCommand(stm, con))
{
{
{
{
Id = rdr["ID"].ToString()
});
}

rdr.Close();
SlideList = listOfSlides;
}
}

con.Close();
}

return SlideList;
}


Some minor remarks

• By enclosing the IDisposable's SQLiteConnection, SQLiteCommand and SQLiteDataReader neither the reader nor the connection needs to be closed explicitly. You can safely remove the calls to Close().

• By stacking the IDisposable's you save one level of horizontal spacing.

• there is no need to introduce the 2 variables cs and slideid.

• abbrevations of variable names should not be done. Using e.g reader instead of rdr would increase the readability of the code.

Setting aside the sql parameters, which had been mentioned by @BCdotWEB the method could look like so

public List<SlideModel> GetSlides(string id)
{

using (SQLiteConnection connection = new SQLiteConnection(dbPath))
{
var listOfSlides = new List<SlideModel>();
string stm = "SELECT * FROM Slide WHERE ID = " + id; //don't do that
connection.Open();

using (SQLiteCommand cmd = new SQLiteCommand(stm, con))
{
{