patterncsharpMinor
ASP MVC Controller Unit Test
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:
Here's the controller under test:
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
{
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
Same for
Don't name something
Even worse:
Your variable names don't tell me what they contain:
Also, why bother assigning
Same for assigning
"Danger, Will Robinson!" You code is now vulnerable to SQL injection:
Use the Parameters collection instead.
Also: looping through a
"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.