Tigraine

Daniel Hoelbling-Inzko talks about programming

A short look at the Big>Days 2009 Demo

Posted by Daniel Hölbling on March 23, 2009

I couldn’t help and look at the Big>Days2009 Rent-A-Worker code that Max Knor recently put up on his website.

I really didn’t spend too much time to look at the whole code, most notably it’s not the complete code (the WPF desktop client and the Silverlight client are missing from the repository).

Usually when confronted with a new code I immediately try to look at the tests to see what the code is about (since I don’t want to build the database on this machine).

Finding the tests is easy, the solution is rather well structured and split into multiple projects to separate concerns.
Unfortunately that’s the only good thing about the tests. Since there are only 2 classes with tests, both which I find tragically funny:

[TestMethod]
public void CustomersGetAll()
{
    TestContext.WriteLine("Retrieving customers...");

    Cust.Customer[] Customers = CustomerMgmt.GetCustomers(string.Empty);     TestContext.WriteLine("Called successfully!");     foreach (var c in Customers)     {         TestContext.WriteLine("Getting details for {0}...", c.CustomerID);         Cust.Customer cd = Customer.GetCustomer(c.CustomerID);         TestContext.WriteLine("Details retrieved: {0} {1}!", cd.Name, cd.MembershipID);

        TestContext.WriteLine("Getting by membership ID...");         int cdId = Customer.GetCustomerForUser(             cd.MembershipID == null ? Guid.Empty : cd.MembershipID.Value);         TestContext.WriteLine("Customer ID by Membership retrieved: {0}!", cdId);

        TestContext.WriteLine("-----");     } }

This should be a test for the Proxy class, but there are no asserts in there. I mean, if you test something, at least make sure you test that what you did worked. Not getting an exception from your code isn’t really a test at all (wait for the guy who mucks all exceptions with try/catch!).

Same thing goes for tests like this one:

[TestMethod()]
public void DeleteResourceTest()
{
    ResourceDataAccess target = new ResourceDataAccess();
}

or half done tests like this one:

[TestMethod()]
public void GetResourcesTest()
{
    ResourceDataAccess target = new ResourceDataAccess(); // TODO: Initialize to an appropriate value
    IEnumerable<RentResource> expected = null; // TODO: Initialize to an appropriate value
    IEnumerable<RentResource> actual;
    actual = target.GetResources();
    Assert.AreEqual(expected, actual);
    Assert.Inconclusive("Verify the correctness of this test method.");
}

From what I can judge (and I’m surely in no position to do that since my latest code was quite untested too), there isn’t one test in two different test projects that actually does something (besides Assert.Inconclusive calls at the end, or no asserts) and so I wonder why someone bothered creating those projects at all.

Also, most code in there uses a static Factory classes that I would abandon in favor of dependency-injection to facilitate testing.


It’s rather painful to see production code like this:

public static ICustomerAccess GetCustomerAccess()
{
    if (UseDummy)
    {
        return new DataAccessDummy();
    }
    else
    {
        return new CustomerDataAccess();
    }
}

(You could spare yourself some pain if you’d have two implementations of the Factory class instead of doing the Dummy branch in every method)

Now, this is hard I know. Most other code I looked at in there is quite nice, the DataAccessLayer seperation is quite nice, and also the strict DTO declaration is really cool, and now hitting on the tests and the factory is quite bad. Also the project structure is a really pleasant sight (although I keep missing projects :)).

But I’m a test and deendency-inection nut, so what matters most to me is what I’ll pick on first. It takes time to come up with good code, and with some refactorings this codebase can really shine (it’s well done after all).

Filed under net, programmierung
comments powered by Disqus

My Photography business

Projects

dynamic css for .NET

Archives

more