Ayende @ Rahien

Refunds available at head office

Review: Microsoft N Layer App Sample, part VIII–CRUD is so 90s

Continuing my review of http://microsoftnlayerapp.codeplex.com/, an Official Guidance (shows up at: http://msdn.microsoft.com/es-es/architecture/en) which people are expected to read and follow.

Take a look at the following:

image

I was curious about that, I didn’t really understand what is going on here, so I tracked it down a bit…

image

Skipping over the part where there is a cast of null, this seems to indicate that this is a call to modify the entity to simulate concurrency, digging deeper:

image

Seems like I was right, it is intended to force the save of this item to the database. That seemed very strange, so I checked upward, where this is used, and I found:

image

So, ChangeBankAccount is just another way of saying Save(), it seems, in a very round about way.

Tags:

Posted By: Ayende Rahien

Published at

Originally posted at

Comments

iwayneo
07/13/2011 09:12 AM by
iwayneo

ceremony, ceremony ceremony... why make patterns easy to work with? I mean, if you're MS - if your customers are learning patterns that are used in other languages - that might mean they have transferable knowledge.

This is what this is all about.

Take the EntLib pile of old shit

policy injection anyone?!

and the EntLib Data Access Application Block!?!? A lump of code that actually makes the task at hand 10000% harder than it would have been without it - while also completely screwing with the whole layout of your system....

NICE!

Honestly - what the hell were they thinking?! http://msdn.microsoft.com/en-us/magazine/cc163766.aspx

Dave
07/13/2011 09:29 AM by
Dave

Better questions are why LockAccount is always negating the lock status, allowing LOCKaccount also to unlock the account if it was already locked.

And why is a entity only marked 'modified' if changeTracker is set? Does this mean that if account.StartTrackingAll() isn't called that entity is never marked modified, but it is actually saved to the database??

And most importantly. If you lock an account, why on earth would you want to redirect the user to a transfer money page?

Peter Morlion
07/13/2011 11:36 AM by
Peter Morlion

With all respect to the people who coded this, but it looks like it was coded by an intern. Very sloppy stuff.

Scooletz
07/13/2011 12:16 PM by
Scooletz

Want to learn all the presented internals, how they should be implemented, in terms of locking, tracking, states? Read NHibernate code and learn. The wrap around wrap around ORM. Gosh, are they paid for each line of code (additional bonus for ctrl+c, ctrl+v?

Jeff Sternal
07/13/2011 12:42 PM by
Jeff Sternal

If I'm reading this right, the LockAccount method also unlock locked accounts.

Frank Quednau
07/13/2011 01:01 PM by
Frank Quednau

Sigh, and then customers frown because the argumentation and opinions expressed to them are so detached from Microsoft.

The null-cast alone shows blatant ignorance to what the developer is actually doing.

I have been wondering lately, did you find anything remotely positive about the whole "official guidance"?

Jay
07/13/2011 01:04 PM by
Jay

I don't quite understand the casting of null part??

Alex Simkin
07/13/2011 01:42 PM by
Alex Simkin

@Jay

This is what Ayende ment as casting of null:

if (bankAccount == (BankAccount)null) {

Nick
07/13/2011 01:51 PM by
Nick

Haha this is just embarrassing.

David
07/13/2011 02:21 PM by
David

Well, a cast of null can be done in order to remind you what kind of type class you are dealing with. It does not heart. It can be a matter of readability. For instance, I saw similar 'null casting' in PEX & MOLES code.

In any case, I beleive that most of that code is outdated as they are working on V2.0 code. That code review is part of V1.0 Sample.

tawani
07/13/2011 04:22 PM by
tawani

Who every wrote/reviewed/verified this code should not be allowed next to an IDE

Kamran
07/13/2011 08:01 PM by
Kamran

Is this really how you should be performing this operation in an "N-Layer" application? Why not just encapsulate that saving logic (i.e. business logic!) into your service itself. What if someone provided an invalid account to the method (etc etc)?:

public void LockAccount(string acctNum) {
    ServiceResult result = BankingService.LockAccount(acctNum);

    if (result.WasSuccessful) {
        // do something (display message, redirect, etc.)

        return RedirectToAction("Index");
    } else {
        // set errors/warnings/alerts/whatever
        // return view model or some alert mechanism
        base.Alert(result.Errors, "Sorry, we could not lock the account due to some errors:");

        return View();
    }
}

Or something to that effect (i.e. encapsulating this saving stuff into the service itself) so that consumers of your service layer don't need to manually save entities? All of that code smells wrong to me.

Alex Simkin
07/13/2011 08:04 PM by
Alex Simkin

Casting null may be used in two cases:

  1. For generic type inference when compiler cannot figure out type, and
  2. When trying to use null as a result for the ternary operator to avoid "no implicit conversion" error.

However, this is so 90's. Cool guys should use default(...) in such cases.

Željko
07/13/2011 09:09 PM by
Željko

As I see it, one reason to cast a null to some object would be if we had a function that expected a certain object type as a parameter. The compiler would then need the cast to decide for the correct function. But in the Microsoft example, I really don't understand it.

Duncan Smart
07/14/2011 03:07 AM by
Duncan Smart

For in thing, I suspect the developer's first language isn't English (nor their 2nd or 3rd perhaps... :) ). It seems that "Change" is a typo for "Charge"... but still.

Duncan Smart
07/14/2011 03:08 AM by
Duncan Smart

Should have said "For one thing..."

Comments have been closed on this topic.