5
\$\begingroup\$

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>();
        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<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))
            {
                using (SQLiteDataReader rdr = cmd.ExecuteReader())
                {
                    while (rdr.Read())
                    {
                        listOfSlides.Add(new SlideModel
                        {
                            Id = rdr["ID"].ToString()
                        });
                    }

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

            con.Close();
        }

        return SlideList;
    }
}

Any input is greatly appreciated!

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

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?


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

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.

\$\endgroup\$
1
\$\begingroup\$
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))
        {
            using (SQLiteDataReader rdr = cmd.ExecuteReader())
            {
                while (rdr.Read())
                {
                    listOfSlides.Add(new SlideModel
                    {
                        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))
        using (SQLiteDataReader rdr = cmd.ExecuteReader())
        {
            while (rdr.Read())
            {
                listOfSlides.Add(new SlideModel
                {
                    Id = rdr["ID"].ToString()
                });
            }

            SlideList = listOfSlides;
        }
    }

    return SlideList;
}  
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.