Ayende @ Rahien

Refunds available at head office

A public request for code review: Raccoon Blog

I really like this idea:

image

The code is available here: http://github.com/ayende/RaccoonBlog/

Comments

Dennis
05/17/2011 04:21 AM by
Dennis

normalize-crlf.ps1

I know it is not part of your main code, but still have to give you a review :P You touch every file, no matter if they need changing or not...

Use this in the inner loop to check (It is a bit overkill to read the whole file, but you are converting small files)

$fn = $_.FullName
if ([System.IO.File]::ReadAllText($fn).Contains(";`n"))
{
( Get-Content $fn -ReadCount 9999999 ) | Set-Content $fn
}

Rafal
05/17/2011 07:10 AM by
Rafal

And now your blog has became self-perpetuating: You blog about creating a blog.

firo
05/17/2011 08:17 AM by
firo

just a small thing:

    public static string ResolveTags(string[] tags)
    {
        string result = tags.Aggregate(string.Empty, (current, tag) => current + (TagsSeparator + tag));
        return result.Trim(TagsSeparator);
    }

Can be written as:

    public static string ResolveTags(string[] tags)
    {
        return string.Join(TagsSeparator.ToString(), tags).Trim(TagsSeparator);
    }

ToString is only needed, if the TagsSeparator is char and not string. Simple tests show, its 4 times faster. I know i doesnt really matter much, but i think join is easier to understand than an aggregate.

Remco
05/17/2011 10:30 AM by
Remco

I just came accros this piece of code in PostDetailsController (line 48/49):

        if (vm.Post.Slug != slug)
            return RedirectToActionPermanent("Details", new { id, vm.Post.Slug });

This can be moved to line 33. and written as:

        var postslug = SlugConverter.TitleToSlag(post.Title)
        if (postslug != slug)
            return RedirectToActionPermanent("Details", new { id, postslug });

this avoids the unneccesary loading of comments and creation/mapping of the viewmodel (which is not used because of the redirect).

also note the spelling mistake in TitleToSlag :-)

Itamar
05/17/2011 12:45 PM by
Itamar

There is also a double redirect, meaning unnecessary requests from the server. For example a request to:

http://ayende.com/Blog/archive/2009/03/08/designing-a-document-database.aspx

which is a search result in Google, will first respond with 301 to:

http://ayende.com/blog/archive/2009/03/08/designing-a-document-database.aspx

and a request to that page will respond with a 301 to the now real URL:

/blog/3897/designing-a-document-database

And now, writing this post, the post preview doesn't seem to handle URLs. Hopefully the posted comment will not look to awful...

Dan Plaskon
05/17/2011 01:12 PM by
Dan Plaskon

I'm thinking someone should review the future post code; as they don't seem to be automatically posting properly ;)

tobi
05/17/2011 01:38 PM by
tobi

"double redirect" this is what I meant when I was talking about SEO. redirect chains are counterintuitively bad for propagating the accumulated trust the old page had.

Ayende Rahien
05/17/2011 01:48 PM by
Ayende Rahien

Firo, Thanks, I implemented your suggestion. Please note that we don't actually need the Trim now.

Ayende Rahien
05/17/2011 01:49 PM by
Ayende Rahien

Dennis, I don't really care about touching all the files. Does it matter in any way?

Ayende Rahien
05/17/2011 01:52 PM by
Ayende Rahien

Remco, That is actually intentional, because is avoids breaking the creation of PostViewModel into several parts. We accept the loading of comments in that scenario as acceptable to keep the code looking better.

Ayende Rahien
05/17/2011 01:53 PM by
Ayende Rahien

Itamar, That is actually two different things happening here. The first is an SEO filter inside IIS that is converting all URLs to lower case. The second is the actual application redirect.

We'll probably do additional work on the comment preview as well, yes.

Ayende Rahien
05/17/2011 02:05 PM by
Ayende Rahien

Tobi, That is a good point, I'll take care of that.

Steves
05/17/2011 03:06 PM by
Steves

@Giedrius Banaitis

http://ayende.com/blog/4092/reviewing-nerddinner

https://github.com/ayende/RaccoonBlog/blob/master/src/RaccoonBlog.IntegrationTests/Views/Post/Details.cs

;-) let's face it, who wants to code a few tens of lines of code (I am counting code to define the test fixture, usings and all the boiler plate code) to test that a method with "return new Something()" really returns Something.

There should be somebody who has balls to admin this :-))

Ayende Rahien
05/17/2011 03:11 PM by
Ayende Rahien

Steves, Thanks, that is actually dead code, which will be removed soon.

Frank Quednau
05/17/2011 10:47 PM by
Frank Quednau

PostAdminController.CommentsAdmin is marked by NDepend as being fairly ugly. I have to say, I agree.

Since you do seem to like using readonly as well, in the AskimetService you can set the key to readonly, and get rid of the document session instance var.

There are a couple other findings, but all in all pretty undramatic.

Giedrius Banaitis
05/18/2011 05:54 AM by
Giedrius Banaitis

@Steves - well there are several things: a) first first class i've opened breaks the rantings posted on your link - I mean several classes in same file and one of them is controller? b) both commands have more than 10 lines (although there's no such rule - if there's more than 10 lines - test it), so i think they are quite complicated, first of all, because they would be not easy testable without refactoring (because you would struggle mocking smtp client or akismet service). All this ranting from my side is just because i see different approach in Ayendes code (talking about this blog engine only, haven't seen much code on other his projects) - he is not trying to make everything mockable and easy testable, question is if that is a good idea.

Remco
05/18/2011 02:25 PM by
Remco

@Ayende

I understand you want to keep things simple.

But look at PostAdminController:

In the Details(...) method this construct is also used (first map post to viewmodel. then check generated slug against slug parameter)

This is good in the sence that it hides the slug generation logic (it's deep hidden in a automapper profile, I don't like that either, but that's not my point right now).

Moving along to line 132 of the same controller (in the 'ugly' (not my words) CommentsAdmin method). SlugConverter is used directly!

So I don't really get why you don't want to use this SlugConverter directly in the other methods to gain some performance.

Remco

BFC
05/18/2011 03:14 PM by
BFC

@Ayende

I am checking the structure of the solution, and trying to understand how you organize your artifacts.

I am confused about the Common, Infrastructure and Helpers folder.

For example, - why you put the class LowercaseRoute in the Common folder instead of the Infrastrucure folder. - why you put the action filter AjaxOnlyAttribute in the Helpers\Attributes folder and the RavenActionFilter inside Infrastructure\Controllers

One more question: the Services folder contains both what you are exposing (MetaWeblog) and consuming (Askimet) right? I think the term Service is very ambiguous. For example, let's say that I have a LoggingService: would you put the class inside the Infrastructure folder or Services folder?

My concern about the solution structure is to have all team members aligned, and not have think too much where should I put some class? My team is checking RaccoonBlog trying to learn that it is possible to have things well done without an heavy architecure (repositories, dependency injection, too mucch tiers, etc.)

Ayende Rahien
05/19/2011 08:57 AM by
Ayende Rahien

Frank, Good suggestions for both, I refactored CommentsAdmin and AskimetService

Ayende Rahien
05/19/2011 08:59 AM by
Ayende Rahien

Giedrius, Regarding testing, you need to look at my posts about where testing have significant value, and where they don't

Ayende Rahien
05/19/2011 09:03 AM by
Ayende Rahien

BFC,

Good point, I've merged all three (Helpers, Common, Infrastructure) to a single location (Infrastructure).

Services, for me, are for interaction points with external stuff. Logging is internal, it would go in the infrastructure.

Comments have been closed on this topic.