Ayende @ Rahien

It's a girl

Reviewing Xenta and wishing I hadn’t

Xenta Framework is the extensible enterprise n-tier application framework with multilayered architecture.

I was asked by the coordinator for the project to review it.

This isn’t going to take long. I looked at the code, and I got this:

image

I am sure you remember the last time when I run into something like this, except that the number of projects at that solution was a quarter of what we had here, and I already had to hold my nose.

Looking a bit deeper:

image

Here is a hint, you are allowed to have more than one class per project.

Having that many project is a nightmare in trying to manage them. Finding things, actually making use of how things work. Not to mention that the level of abstractness required to support that is giving me a headache.

This is still without looking at the code, mind.

Now, let us look at the actual code. I like to start the controllers. The following code is from ForumController:

[HttpPost, ValidateInput(false)]
public ActionResult Update(int forumID, FormCollection form)
{
    ForumModel m = new ForumModel()
    {
        ForumID = forumID
    };

    if(!m.Load())
    {
        return HttpNotFound();
    }

    if(TryUpdateModel<ForumModel>(m, "Model", form))
    {
        try
        {
            m.Update();
        }
        catch(Exception ex)
        {
            ModelState.AddModelError("API", ex);
        }
    }

    if(!ModelState.IsValid)
    {
        TempData.OperationStatus("Failed");
        TempData.PersistObject("Model", m);
        TempData.PersistModelState(ModelState);
    }
    else
    {
        TempData.OperationStatus("Success");
    }

    return RedirectToAction("Edit", new
    {
        ForumID = m.ForumID
    });
}

Seriously, I haven’t seen this style of architecture in a while. Let us dig deeper:

public bool Update()
{
    using(ForumApiClient api = new ForumApiClient())
    {
        var dto = api.UpdateForum(ForumID, ParentForumID, DisplayOrder, Flags);

        return Load(dto);
    }
}

public bool Load()
{
    using(ForumApiClient api = new ForumApiClient())
    {
        var dto = api.GetForum(ForumID);

        return Load(dto);
    }
}

In case you are wondering, those are on the ForumModel class.

This piece of code is from the ForumPostModel class, it may look familiar:

public bool Load()
{
    using(ForumApiClient api = new ForumApiClient())
    {
        var dto = api.GetPost(PostID);

        return Load(dto);
    }
}

This ForumApiClient is actually a WCF Proxy class, which leads us to this interface:

image

I won’t comment on this any further, but will go directly to ForumApiService, where we actually update a forum using the following code:

public ForumDto UpdateForum(int forumID,
    int parentForumID,
    int displayOrder,
    ForumFlags flags)
{
    ForumService forumService = Infrastructure.Component<ForumService>();

    if(forumService == null)
    {
        throw new FaultException(new FaultReason("forum service"), new FaultCode(ErrorCode.Infrastructure.ToString()));
    }

    try
    {
        ForumEntity entity = forumService.UpdateForum(forumID, parentForumID, displayOrder, flags, DateTime.UtcNow);

        return Map(entity);
    }
    catch(XentaException ex)
    {
         throw new FaultException(new FaultReason(ex.Message), new FaultCode(ex.Code.ToString()));
    }
}

Infrastructure.Component represent a home grown service locator. Note that we have manual exception handling for absolutely no reason whatsoever (you can do this in a behavior once, and since this is repeated for each and every one of the methods…).

I apologize in advance, but here is the full UpdateForum method:

public ForumEntity UpdateForum(int forumID,
    int parentForumID,
    int displayOrder,
    ForumFlags flags, 
    DateTime updatedOn)
{
    ForumEntity oldForum = GetForum(forumID);
            
    if(oldForum == null)
    {
        throw new XentaException(ErrorCode.NotFound, "forum");
    }

    ForumEntity parentForum = null;

    if(parentForumID != 0)
    {
        parentForum = GetForum(parentForumID);

        if(parentForum == null)
        {
            throw new XentaException(ErrorCode.NotFound, "forum");
        }

        ForumEntity tmp = parentForum;

    HierarchyCheck:

        if(tmp.ForumID == forumID)
        {
            throw new XentaException(ErrorCode.InvalidArgument, "parentForumID");
        }
        if(tmp.ParentForumID != 0)
        {
            tmp = tmp.Parent;

            goto HierarchyCheck;
        }
    }

    ForumEntity newForum = null;
    bool res = Provider.UpdateForum(forumID,
        parentForumID,
        oldForum.LastTopicID,
        oldForum.TopicCount,
        oldForum.PostCount,
        displayOrder,
        flags,
        oldForum.CreatedOn,
        updatedOn);

    if(res)
    {
        if(Cache != null)
        {
            Cache.Remove("ForumService_GetForum_{0}", oldForum.ForumID);
        }

        newForum = GetForum(forumID);

        if(EventBroker != null)
        {
            EventBroker.Publish<PostEntityUpdate<ForumEntity>>(this, new PostEntityUpdate<ForumEntity>(newForum, oldForum));
        }
    }
    return newForum;
}

Yes, it is a goto there, for the life of me I can’t figure out why.

Note that there is also this Provider in here, which is an IForumProvider, which is implemented by… ForumProvider, which looks like this:

public bool UpdateForum(int forumID,
    int parentForumID,
    int lastTopicID,
    int topicCount,
    int postCount,
    int displayOrder,
    ForumFlags flags,
    DateTime createdOn,
    DateTime updatedOn)
{
    bool res = false;

    using(DbCommand cmd = DataSource.GetStoredProcCommand("fwk_Forums_Update"))
    {
        SqlServerHelper.SetInt32(DataSource, cmd, "ForumID", forumID);
        SqlServerHelper.SetInt32(DataSource, cmd, "ParentForumID", parentForumID);
        SqlServerHelper.SetInt32(DataSource, cmd, "LastTopicID", lastTopicID);
        SqlServerHelper.SetInt32(DataSource, cmd, "TopicCount", topicCount);
        SqlServerHelper.SetInt32(DataSource, cmd, "PostCount", postCount);
        SqlServerHelper.SetInt32(DataSource, cmd, "DisplayOrder", displayOrder);
        SqlServerHelper.SetInt32(DataSource, cmd, "Flags", (int)flags);
        SqlServerHelper.SetDateTime(DataSource, cmd, "CreatedOn", createdOn);
        SqlServerHelper.SetDateTime(DataSource, cmd, "UpdatedOn", updatedOn);

        res = DataSource.ExecuteNonQuery(cmd) > 0;
    }
    return res;
}

And… this is about it guys.

I checked the source control, and the source control history for this goes back only to the beginning of February 2012. I assume that this is older than this, because the codebase is pretty large.

But to summarize, what we actually have is a highly abstracted project, a lot of abstraction. A lot of really bad code even if you ignore the abstractions, seemingly modern codebase that still uses direct ADO.Net for pretty much everything, putting a WCF service in the middle of the application just for the fun of it.

Tons of code dedicated to error handling, caching, etc. All of which can be handled as cross cutting concerns.

This is the kind of application that I would expect to see circa 2002, not a decade later.

And please note, I actually got the review request exactly 34 minutes ago. I didn’t review the entire application (nor do I intend to). I merely took a reprehensive* vertical slide of the app and followed up on that.

*Yes, this is intentional.

Tags:

Posted By: Ayende Rahien

Published at

Originally posted at

Comments

Scooletz
05/10/2012 09:14 AM by
Scooletz

Some people just love coding and following cargo cult like following rules even when it requires of writting the same over and over again. But hey! They've written a lot of code! Isn't it great?

Serge
05/10/2012 09:24 AM by
Serge

You should write a book about all mistakes and how to avoid them.

Andrew Rimmer
05/10/2012 09:34 AM by
Andrew Rimmer

Wow, I don't remember ever seeing a goto used in c#

Yuriy
05/10/2012 09:45 AM by
Yuriy

Pure boilerplate - maybe this is an output from some DSL?

Matt
05/10/2012 09:47 AM by
Matt

I think I've only ever used a goto once in anger, and that was many years ago for some numerical code in c. It gave a performance benefit that reduced the time to run a model by about 10%. I can't even remember the details now. gotos do have their place, but it's a very small niche and I doubt I'll ever use one again.

tobi
05/10/2012 09:50 AM by
tobi

This really is about 2002. Funny how much best practices have evolved over the years. Today, this code is rightfully regarded as junk.

Thomas Levesque
05/10/2012 09:51 AM by
Thomas Levesque

Note to self: never ask Ayende to review your code if you want to keep a bit of self esteem ;-)

Bjorn Coltof
05/10/2012 09:53 AM by
Bjorn Coltof

Wow, a goto that isn't the result of a decompiled optimized assembly... Cheer!!!

Yuriy
05/10/2012 09:53 AM by
Yuriy

Goto's are probably only there to simplify decompilation in some cases :)

configurator
05/10/2012 10:15 AM by
configurator

Loops? We don't need no stinkin' loops! We have goto!

Rippo
05/10/2012 10:32 AM by
Rippo

88 projects, really! Resharper is a must for this project. Don't think I have used a GOTO in C# ever, VBA now that's a different story

Rippo
05/10/2012 10:32 AM by
Rippo

88 projects, really! Resharper is a must for this project. Don't think I have used a GOTO in C# ever, VBA now that's a different story

Khalid Abuhakmeh
05/10/2012 11:05 AM by
Khalid Abuhakmeh

I like on the codeplex site for the project it says:

"Easy to use and customize"

Probably, you just need to add another project followed by some goto statements. :P

Wayne M
05/10/2012 12:41 PM by
Wayne M

Wow. Reminds me of my own codebase at work (which I did not write) that uses that wacky bool Load type stuff everywhere. I don't get it either. Is this VB6 all over again?

Ian Nelson
05/10/2012 02:42 PM by
Ian Nelson

Wow. Just... wow.

Ollie Riches
05/10/2012 03:25 PM by
Ollie Riches

But what does it do? I can't understand from codeplex what this framework is for...

Gene Hughson
05/10/2012 03:29 PM by
Gene Hughson

You'd think with all the other plug-ins they'd provide an option for direct-connect to the back end.

@Ollie, it looks like it's supposed to provide some common functionality around sales, etc. Not a good sign for a "framework" when there's zero documentation.

Ali Hmer
05/10/2012 03:37 PM by
Ali Hmer

The framework has no developers. Only contributors. No wonder. And now no one would hire dnovikov, http://www.codeplex.com/site/users/view/dnovikov. FYI, dnovikov is the sole contributor to the project.

Karep
05/10/2012 06:19 PM by
Karep

What do you mean by "you can do this in a behavior once, and since this is repeated for each and every one of the methods…" What behaviour?

Bjorn Coltof
05/10/2012 06:24 PM by
Bjorn Coltof

@karep WCF has a behavior concept that allows you to do common stuff once and then apply it to all/lots of services

Kim
05/10/2012 07:19 PM by
Kim

if itlooks like shit it most likely is shit. I would not touch that code under any circumstances :)

David
05/10/2012 10:53 PM by
David

"what does it do" - http://demo.xenta.net/

João P. Bragança
05/11/2012 03:23 AM by
João P. Bragança

You'd think Ayende would've learned his lesson by now...

firefly
05/11/2012 04:15 AM by
firefly

This look like an April fool post :)

magellings
05/11/2012 03:18 PM by
magellings

I think I just pulled my hair out...

Alessandro Riolo
05/11/2012 08:49 PM by
Alessandro Riolo

I shall own up, I must have coded in a very similar way to this the 1st time I started to deal with .Net, circa 2002. Probably even worst. I must say just reminding of Visual Studio .NET gives me goose bumps :) Perhaps the guy has only got 6 months experience with .Net, he'll probably learn how to code better in future, hopefully this review will speed him up ...

Rick D
05/12/2012 07:56 PM by
Rick D

Oh, come on, it's a joke right? Right?!?

DC
05/12/2012 10:29 PM by
DC

Seriously, no self reflection? 88 project, that should smell ....

Lucas Ontivero
05/13/2012 03:15 PM by
Lucas Ontivero

The goto statements is not the worst part because it can be removed easily, the woprst part for me is the old idea of having WCF services behind web applications to 'improve' the security. It´s the evil for me.

Jon Rowett
05/14/2012 09:01 AM by
Jon Rowett

unmaintainable code written in an obsolete architectural style, to be sure, and i sincerely hope i never have to work with this or anything like it (although i probably will)....

BUT you guys shouldn't be mean to the developer responsible. comments like "now no-one will hire " are completely out of order.

it's clearly a labour of love written by someone who enjoys coding. he just needs to get out of his intellectual vacuum.

Bil Simser
05/17/2012 12:40 AM by
Bil Simser

You know people criticize you for giving bad reviews (or maybe non-constructive ones) on code bases. This one makes my eyes bleed. There is nothing good that can be said about it. I'm about to write a blog post explaining why coding to interfaces is good even if it does create "more code". I can't believe this is 2012.

Wayne M
05/17/2012 08:38 PM by
Wayne M

I can certainly believe this code. I see similar code every day at work that I sometimes forget it's 2012. No interfaces, 100% reliance on stored procedures for everything, no understanding of unit tests, a few gotos here and there, the argument that, and I quote, "Writing tests makes code take longer" as an argument against any sort of testing beyond poking around an app.

It's real, believe me. And the people who write code like that probably have no idea why it's bad, no concept of evolution. Just churning out the same garbage all the time because "it works for me".

Comments have been closed on this topic.