Kobe – In the nuts & bolts and don’t really liking it
There is another ASP.Net MVC sample app, this time it is official, passed the proper review procedure, and is explicitly marketed as “intended to guide you with the planning, architecting, and implementing of Web 2.0 applications and services.”
I am saying all of that in order to distinguish it from Oxite, which was non of this things. There have been a couple of reviews of Kobe already. Frankly, I don’t really care for them, mostly because I think that they dealt too much with nitty gritty details of the app that doesn’t really matter much. I don’t much care for extra using or the use of System.Int32 vs. int, the naming convention used or even what sort of HTML formatting they used. I mostly care about the code and architecture. So I decided to take a look myself.
This post is going to go over the nuts & bolts, looking at low level coding details. I am going to have another that is going to look at the architecture and probably a few others that talk about some additional concerns that I have.
Let us start by looking at the code via some tools. Simian reports:
And that is when the similarity threshold is 6 lines, I usually run it with three, if we try that, we get:
Found 5138 duplicate lines in 873 blocks in 67 files
Next, the most important code metric to me in most cases, Cyclomatic Complexity. Two methods literally jump out of the solution and beg for mercy.
- HomeController.Discovery with CC index of 35(!)
- GroupController.Profile with CC index of 22
I am going to show them both in all their glory, and let you be the judge of them:
public ActionResult Discovery(string query) { query = query.Trim(); query = Server.HtmlEncode(query); List<PresentationSummary> presentationsAll = null; List<PresentationSummary> presentationsMostPopular = null; List<PresentationSummary> presentationsMostViewed = null; List<PresentationSummary> presentationsMostDownload = null; List<PresentationSummary> presentationsNew = null; List<UserSummary> activeusers = null; List<UserSummary> allusers = null; List<Group> activegroups = null; List<Group> allgroups = null; int iNewMemberPageCount = 0; int iNewGroupPageCount = 0; int ipresentationsAll = 0; int ipresentationsMostPopular = 0; int ipresentationsMostViewed = 0; int ipresentationsMostDownload = 0; int ipresentationsNew = 0; UserSummary user = _userService.GetUserByName(query); if (user != null) { presentationsAll = _userService.GetAuthoredPresentations(user.UserName); presentationsMostPopular = presentationsAll.OrderByDescending(p => p.Favorites).ToList(); presentationsMostViewed = presentationsAll.Where(p => p.Views > 0).OrderByDescending(p => p.Views).ToList(); presentationsMostDownload = presentationsAll.Where(p => p.Downloads > 0).OrderByDescending(p => p.Downloads).ToList(); presentationsNew = presentationsAll.OrderByDescending(p => p.InsertedDate).ToList(); ipresentationsAll = decimal.Divide(presentationsAll.Count, 10).ToInt(); ipresentationsMostPopular = decimal.Divide(presentationsMostPopular.Count, 10).ToInt(); ipresentationsMostViewed = decimal.Divide(presentationsMostViewed.Count, 10).ToInt(); ipresentationsMostDownload = decimal.Divide(presentationsMostDownload.Count, 10).ToInt(); ipresentationsNew = decimal.Divide(presentationsNew.Count, 10).ToInt(); activeusers = new List<UserSummary> { user }; allusers = new List<UserSummary> { user }; iNewMemberPageCount = decimal.Divide(allusers.Count, 8).ToInt(); allgroups = _userService.GetGroups(user.UserName); activegroups = allgroups.OrderByDescending(g => g.InsertedDate).ToList(); iNewGroupPageCount = decimal.Divide(allgroups.Count, 8).ToInt(); } else { Group group = _groupService.GetGroupByName(query); if (group != null) { presentationsAll = _groupService.GetPresentations(group.GroupName); presentationsMostPopular = presentationsAll.OrderByDescending(p => p.Favorites).ToList(); presentationsMostViewed = presentationsAll.Where(p => p.Views > 0).OrderByDescending(p => p.Views).ToList(); presentationsMostDownload = presentationsAll.Where(p => p.Downloads > 0).OrderByDescending(p => p.Downloads).ToList(); presentationsNew = presentationsAll.OrderByDescending(p => p.InsertedDate).ToList(); ipresentationsAll = decimal.Divide(presentationsAll.Count, 10).ToInt(); ipresentationsMostPopular = decimal.Divide(presentationsMostPopular.Count, 10).ToInt(); ipresentationsMostViewed = decimal.Divide(presentationsMostViewed.Count, 10).ToInt(); ipresentationsMostDownload = decimal.Divide(presentationsMostDownload.Count, 10).ToInt(); ipresentationsNew = decimal.Divide(presentationsNew.Count, 10).ToInt(); allusers = _groupService.GetMembers(group.GroupName); activeusers = allusers.OrderByDescending(u => u.DateOfJoining).ToList(); iNewMemberPageCount = decimal.Divide(allusers.Count, 8).ToInt(); allgroups = new List<Group> { group }; activegroups = new List<Group> { group }; iNewGroupPageCount = decimal.Divide(allgroups.Count, 8).ToInt(); } else { presentationsAll = _presentationService.GetAllPresentationsByKewordTimeLine(query, "Day", "0", 10, 1); presentationsMostPopular = _presentationService.GetMostPopularPresentationsByKewordTimeLine(query, "Day", "0", 10, 1); presentationsMostViewed = _presentationService.GetMostViewedPresentationsByKewordTimeLine(query, "Day", "0", 10, 1); presentationsMostDownload = _presentationService.GetMostDownloadedPresentationsByKewordTimeLine(query, "Day", "0", 10, 1); presentationsNew = _presentationService.GetNewPresentations(query, "Day", "0", 10, 1); ipresentationsAll = decimal.Divide(_presentationService.GetAllPresentationsByKewordTimeLine(query, "Day", "0").Count, 10).ToInt(); ipresentationsMostPopular = decimal.Divide(_presentationService.GetMostPopularPresentationsByKewordTimeLine(query, "Day", "0").Count, 10).ToInt(); ipresentationsMostViewed = decimal.Divide(_presentationService.GetMostViewedPresentationsByKewordTimeLine(query, "Day", "0").Count, 10).ToInt(); ipresentationsMostDownload = decimal.Divide(_presentationService.GetMostDownloadedPresentationsByKewordTimeLine(query, "Day", "0").Count, 10).ToInt(); ipresentationsNew = decimal.Divide(_presentationService.GetNewPresentations(query, "Day", "0").Count, 10).ToInt(); activeusers = _userService.GetMostActiveUsers(query, 8, 1); allusers = _userService.GetAllUsers(query, 8, 1); iNewMemberPageCount = decimal.Divide(_userService.GetMostActiveUsers(query).Count,8).ToInt(); activegroups = _groupService.GetMostActiveGroupByKeyword(query, 8, 1); allgroups = _groupService.GetAllGroupByKeyword(query, 8, 1); iNewGroupPageCount = decimal.Divide(_groupService.GetMostActiveGroupByKeyword(query).Count, 8).ToInt(); } } ViewData.Add("membersList-mostactive", activeusers); ViewData.Add("membersList-all", allusers); ViewData.Add("groupsList-mostactive", activegroups); ViewData.Add("groupsList-all", allgroups); ViewData.Add("presentations-all",presentationsAll); ViewData.Add("presentations-mostpopular",presentationsMostPopular); ViewData.Add("presentations-mostviewed",presentationsMostViewed); ViewData.Add("presentations-mostdownload",presentationsMostDownload); ViewData.Add("presentations-new",presentationsNew); ViewData.Add("Query", query); //ViewData.Add("Presentations", presentations); ViewData.Add("members-totalcount", iNewMemberPageCount); ViewData.Add("groups-totalcount", iNewGroupPageCount); ViewData.Add("presentations-alltotalcount", ipresentationsAll); ViewData.Add("presentations-mostpopulartotalcount", ipresentationsMostPopular); ViewData.Add("presentations-mostviewedtotalcount", ipresentationsMostViewed); ViewData.Add("presentations-mostdownloadtotalcount", ipresentationsMostDownload); ViewData.Add("presentations-newtotalcount", ipresentationsNew); return View(); }
This is… a very busy method, I must say. But in a way, the Profile method is much worse:
[AcceptVerbs(HttpVerbs.Post)] public ActionResult Profile(string gname, string type, string section, string subSection, string page) { if (string.IsNullOrEmpty(gname)) return new ContentResult { Content = "" }; if (type != "widget") return new ContentResult { Content = "" }; Group group = null; try { group = _groupService.GetGroupByName(gname); } catch (Exception) { return new ContentResult { Content = "" }; } if (group == null) { return new ContentResult { Content = "" }; } string groupName = group.GroupName; AddUserLevelToViewData(groupName); int pageNo = 1; Int32.TryParse(page, out pageNo); if (pageNo == 0) pageNo = 1; if (section == "div-GroupPresentations") { List<PresentationSummary> presentations = null; switch (subSection) { case "div-GroupPresentations-RecentltAdded": presentations = _groupService.GetRecentlyAddedPresentations(groupName, 5, pageNo); break; case "div-GroupPresentations-MostViewed": presentations = _groupService.GetMostViewedPresentations(groupName, 5, pageNo); break; case "div-GroupPresentations-MostDownloaded": presentations = _groupService.GetMostDownloadedPresentations(groupName, 5, pageNo); break; case "div-GroupPresentations-All": presentations = _groupService.GetPresentations(groupName, 5, pageNo); break; } return View("PresentationsList", presentations); } else if (section == "div-GroupWall") { switch (subSection) { case "div-GroupWall-Messages": ViewData["GroupMessages"] = _groupService.GetMessages(groupName, 5, pageNo); return View("GroupMessageList", ViewData["GroupMessages"]); case "div-GroupWall-MemberRequests": ViewData["GroupJoiningRequests"] = _groupService.GetGroupJoiningRequests(groupName, 5, pageNo); return View("GroupJoiningRequestList", ViewData["GroupJoiningRequests"]); } } else if (section == "div-GroupInfoExtended") { switch (subSection) { case "div-GroupInfoExtended-GroupMembers": ViewData["GroupMembers"] = _groupService.GetMembers(groupName, 4, pageNo); return View("MembersList", ViewData["GroupMembers"]); } } return new ContentResult { Content = "" }; }
Just look at the code. I thought that the whole point of MVC was to separate the logic from the view. Having the view strongly tied to the controller output is fine by me, but having the controller strongly tied to the HTML format of the page? That isn’t right.
Another thing that isn’t right is HomeController.Index():
public ActionResult Index() { GetFeaturedPresentations(); //*** Dummy call the the Database to activate the Connection. List<PresentationSummary> featured = _presentationService.GetFeaturedPresentations(); List<PresentationSummary> beingViewed = _presentationService.GetPresentationRecentlyViewed(); List<PresentationSummary> mostDownloaded = _presentationService.GetMostDownloadedPresentation(); PresentationSummary presentationOfDay = _presentationService.GetPresentationOfDay(); ViewData.Add("FeaturedPresentations", featured.ToArray()); ViewData.Add("RecentlyViewedPresentations", beingViewed.ToArray()); ViewData.Add("MostDownloadedPresentations", mostDownloaded.ToArray()); ViewData.Add("presentationsOfDay", presentationOfDay); ViewData["Tags"] = _presentationService.GetPopularTags(); return View(); }
Notice the first call?
private void GetFeaturedPresentations() { try { //*** Dummy call to the Presentation Service to get the Featured presentations, //*** this call is place because, an exception thrown from the Data layered on first hit to the DB (failed to open DB) //*** and the second hit to the DB gets success. _presentationService.GetFeaturedPresentations(); } catch (Exception) { /*do nothing with this exception*/ } }
I really like it when you work around a bug instead of actually fix it.
Moving on, let us look at the service layer. Most of it looks like this:
public ADs GetAdById(string adId) { try { string key = "Ads-" + adId; ADs data = cacheService.Get<ADs>(key); if (data == null) { data = provider.GetAdById(adId.ToGuid()); if (data != null && data.Image != null) { cacheService.Add(key, data as Object, null, DateTime.MaxValue, new TimeSpan(0, 10, 0), System.Web.Caching.CacheItemPriority.Normal, null); } } return data; } catch (Exception ex) { bool rethrow = ExceptionPolicy.HandleException(ex, "Service Policy"); if (rethrow && WebOperationContext.Current == null) { throw; } return null; } }
I am not joking about all of them looking alike, by the way, it is obviously has been cut & paste a lot.
Nitpick, “data as Object” – that is not something you often see. But this is even better (from CacheService):
I like how we have a cacheService but we coupled its interface with System.Web.Caching, or the fact that most of this code is just a very long winded way of calling GetAdById.
But wait, I spared you the method documentation, which is a real masterpiece:
/// <summary> /// Returns Advertisment. /// </summary> /// <param name="adId"> GUID of an Advertisment</param> /// <returns>Ads</returns> public ADs GetAdById(string adId)
Yes, the adId is a string which is a guid. We are so lucky to work with VARIANT again, right?
Let us take another method, just to see what is going on in a typical method inside the project. I intentionally avoid the ones that we already looked at. I took a peek at CommunityController and found the Index method:
[AcceptVerbs(HttpVerbs.Post)] public ActionResult Index(string type, string section, string subSection, string page) { if (type != "widget") return new ContentResult { Content = "" }; int pageNo = 1; Int32.TryParse(page, out pageNo); if (pageNo == 0) pageNo = 1; if (section == "members") { List<UserSummary> users = null; switch (subSection) { case "members-new": users = _communityService.GetNewUsers(8, pageNo); break; case "members-mostactive": users = _communityService.GetMostActiveUsers(8, pageNo); break; case "members-all": users = _communityService.GetAllUsers(8, pageNo); break; default: users = _communityService.GetAllUsers(8, pageNo); break; } return View("MembersList", users); } else if (section == "groups") { List<Group> groups = null; switch (subSection) { case "groups-new": groups = _communityService.GetNewGroups(8, pageNo); break; case "groups-mostactive": groups = _communityService.GetMostActiveGroups(8, pageNo); break; case "groups-all": groups = _communityService.GetAllGroups(8, pageNo); break; default: groups = _communityService.GetAllGroups(8, pageNo); break; } return View("GroupsList", groups); } else if (section == "favourites") { List<PresentationSummary> favouritePresentation = _communityService.GetCommunityFavorite(10, pageNo); return View("PresentationsView", favouritePresentation.ToArray()); } return new ContentResult { Content = "" }; }
Let me see how many things I can find in a cursory examination:
- Hard coding galore
- Long method
- Complex method
- Controller method return several different views
And note that I am still not even trying for the architectural concepts or code quality metrics. That I’m going to leave to another post.
Frankly, I am seeing way too much bad things in the code to overview all of them. I am going to stop with a goodie, though.
Let us explore GroupRepository.GetGroup, shall we?
private Group GetGroup(Guid groupId, KobeEntities Context) { try { Group group = Context.GroupSet .Where(g => g.GroupId == groupId) .FirstOrDefault(); if (group == null) throw new DataException("Invalid Group Id", null); return group; } catch (Exception ex) { bool rethrow = ExceptionPolicy.HandleException(ex, "Data Policy"); if (rethrow) { throw; } return null; } }
On the face of it, except for the repeated stupid error handling, there doesn’t seems to be something wrong here, right?
Take note for the different parameter casing on the GetGroup, though. Why is KobeEntities PascalCase? Well, that is because there is also a property called Context on the GroupRepository that you might use by accident. So, what is this Context parameter all about? GetGroup is a private method, who is calling it?
Here is one such callsite:
public void AddGroupInterests(Guid groupId, string[] interests, Guid userId) { try { KobeEntities _context = Context; Group group = GetGroup(groupId, _context); User user = GetUser(userId, _context);
So, we take the Context property, put it in a _context local variable. Then we pass it to GetGroup, which uses it.
I must say that I am at a loss to understand what was going on in the mind of whoever wrote it. Was he trying to optimize the number of time we access the property?
As I said, I am currently writing a few other posts about Kobe, this was just to get to see the code itself, so we have an impression about its quality.
I am… not impressed.
Comments
After reading this long post, I must say that I actually see a lot of MVC code that just doesn't grasp the MVC way of coding. They like the url rewriting, but they don't want to write all those controllers and action methods that they should. A group is not a user, so they should have a GroupController and a UserController. Each subsection case should be an action method.
As I assume that they use Linq to Sql I don't really understand why GetUser and GetGroup aren't part of the context.
I find it very interesting to see that if a group (and probably a user) doesn't exist, they throw a DataException. Why not just return the null value? The group is just passed on to the view and inside the view group is examined. If group is null they display a message that the group doesn't exists or it present some group data. What happens when they need to resolve a list of groups. The first group that doesn't exists really screws up your application. GetGroup is strongly coupled to the controller that's using the method. Can't wait for part 2..
In the video that comes on the Kobe page, it appears that one of the Kobe contributor is the "Lead for the Plateform Architecture Team". Poor code to be produced by inexperienced developers is probably unavoidable, but when the owner is a supposedly high-ranked engineer, it's hard not to think WTF?
Thanks for taking the time to write this - I'm going to meet with the team this coming week and I'd love to use this feedback. It's a very complete writeup and believe me - we're on it :).
@Joannes the issue isn't coding in general - it's that people in our org are still trying to grasp the ins and outs of MVC. Please keep in mind that it's new and most folks internally are only used to WebForms. It's not an excuse - and it's only related to the MVC stuff. The other things Oren raises are valid questions to be sure.
On the plus side it makes me feel much better about my own work!
But, Rob, unused to webforms or not, some of this is just bad programming. And to be honest, really, how hard is it to grasp MVC?
If this is the way they program in webforms, then that's not very smart either. Those really long methods whether they are action methods or not, are generally not good.
I guess MS doesn't use the brightest people for sample apps. But at least they should be given Refactoring by Martin Fowler, as a mandatory read.
@Rob - MVC isn't new, it has been around for decades.
Thanks again to Oren for helping me avoid wasting my time.
Are there any good ASP.NET MVC apps out there whose code is available to browse?
All I can say is WTF? Comparing my MonoRail apps to this code, I feel like a genius! Maybe I can get a job at Microsoft?
@Rik
MVC StoreFront by Rob Conery is a good and solid MVC sample
S#arp architecture is good - possibly not in a style I would choose personally
Suteki Shop isn't bad
Apart from that - Cuyahoga on Monorail is a good MVC sample app - much applies to ASP.NET MVC too
@robconery: MVC is new? So you think MVC wasn't around until Microsoft "invented" it?
@Barry,
I too feel the same. If this is the code coming from MS ;)
I think the issue is the overloading of vocabulary.
"MVC", the design pattern, started with Smalltalk and was gradually refined.
"MVC", the Web application architecture (?), probably started with---and was definitely popularized by---Ruby on Rails.
"MVC", the abbreviation for "ASP.NET MVC", is basically "Rails.NET", and from what I understand (not having taken too close a look at it) it's basically a direct clone of MonoRail (which itself draws heavily from Rails, hence the name).
So, MVC isn't new, and MVC isn't new, and MVC is sorta new, but only in areas where Microsoft broke it. Got it? (There'll be a short quiz on this next period.)
I'm repeating myself a lot, which probably means that I'm wrong but...
The nitty gritty does matter. Google and Java list consistent use of coding styles as a key factor in readable and maintainable systems. Apple has made a business out of consistent UI. I think dismissing the least-consistent coding-style I've ever seen in order to go after more fundamental issues is a wrong. There's no reason not to be equally harsh on both.
Also, I've been there and done that with Oxite. I've detailed some of the more fundamental problems. Kobe has those same problems, I just listed them off. We touched on the same issues, horrible controllers, lack of business logic, poor exception handling and stupid data access patterns.
I think you are being harsh on my review :P
@Ben I think @robconery meant that MVC is new to a lot of folks internal to MS. Remember, the MVC concept was new to you once, too.
We should keep in mind that what folks like @robconery, @shanselman, & @scottgu are trying to do is change the culture of a very large organization. Change is hard, and they are doing an admirable job of turning the direction of a VERY large ship. It takes time, and there will be mis-steps along the way. Rather than belittle them, we should help them, as Oren is doing with his thoughtful series of critiques.
@Ben
I think Rob´s comment was clear, people inside his organization was trying to get a better grasp on MVC and MVC in this particular incarnation. It is not like they invented it. No need to pick on a simple comment.
@Joannes
Probably someone put his name too fast on a work executed by others? Wouldn´t be the first time.
Karl,
Consistency _matters_, no two ways about it.
But I feel that focusing on that when you have a code base that looks like Kobe is doing injustice.
It is putting a plaster instead of putting tourniquet.
@Benjamin Geiger
I think it goes further back than Ruby on Rails, see the Java frameworks Struts and Spring.
(I had a long apology written, until I noticed that the people talking to "Ben" weren't talking to me. If anyone took offense at my comment, I apologize anyway.)
@James: I see. Rails did more to popularize the paradigm than other frameworks, even though it didn't originate there.
MVC or Webforms, Kobe is really horrible. Coming from Java to ASP.NET I still can't believe how coding inconsistencies thrive in the .NET world.
I would really like to know any project that this same "kobe" team has worked on.
I couldn't even finish this.. that code gave me a headache. Too much going on in a controller.
Having some experience with offshore outsourcing, this is exactly the kind of [dare I say] code I'v come to expect - always requiring a great deal of refactoring. Please, don't tell me this was from someone in the mothership...
Ouch, that's awful! I found it hard to read your blog, I don't think I could read any more so I wont bother downloading the demo :-)
Look, we've all written a lot of crappy code; we've all read a lot of crappy code. I'm sure MS engineers also read and write a lot of crappy code.
But the critical point here is that MS SHOULD NOT deliver crappy code in a sample application. You need your BEST engineers working on sample apps--let your junior devs work on the marketing sites. The sample apps are used for teaching and learning best practices.
Its disturbing to think that a lot of young developers will turn to this app assuming that they'll be seeing the correct and best way to develop an MVC app on the MS platform.
@Rob Conery
Rob, the problem is your team should have met with them BEFORE they pushed this out there. The damage is done now. There should be a rule of rules that if someone in some random team is going to put out guidance and a kit like this that it should go through a series of things:
Call the guys who wrote it who's job it is to preach about it (You, Phil, ScottHa, etc)
Put out feedback to members of the community
Rinse and repeat.
Cheers,
-Keith
It is unfortunate that code goes out that isn't "EXACTLY" the way you/me/or someone else would have written it. Because we all know that there is but one way in which to code things - that way, is "correctly".
But some of the arguments and whining that occurs is somewhat pathetic. Software developers need to take a deep breath and move on with their existence! Sheesh! I don't know of another industry where the practitioners are so bloody pissy about anything that isn't "theirs".
By arguments made in the comments above - everyone should just grasp MVC without issue, as it's not been invented by Microsoft and has existed for many, many years. And just because you may have grown through classic ASP, and ASP.Net 1.1 webforms, you should immediately drop all knowledge and simply code "Correctly", which, isn't documented anywhere - people are just supposed to GRASP it.
Give it up people! Coding correctly is in direct relation to the time management allows for research/learning and project deadlines.
Differential Mathematics is arguably one of the most difficult practices to grasp - to some almighty "developers", it would appear that if one can't grasp the concept, then they should just completely stop doing math OR Learn it! Well - one doesn't have to learn it to get through life...
Guess what people - just because a method has more than "five" lines of code, doesn't make it evil. The arbitrary numbers from Simbian are meant to be something to look into by a subjective PERSON. taking statistics as in the above post is bad form, IMO.
Nevertheless, the super-guru-absolutely-never-wrote-a-bad-line-of-code-in-my-life devs above point out valid facts regarding the code - I just think attitude towards teaching others is better than reprimanding and being a git about it.
Also - amazingly, there are a ton of devs here that claim to have perfected development (all over the web, actually) - I've yet to see an actual, usable, LOB application that takes into account every best practice possible. Perhaps taking the time to write a tutorial would better suit your time than traversing the net and posting to others examples?
Just a thought.
And yes - I suck. I don't do TDD (Although I am researching it and will probably do it all wrong), I LOVE WCSF and WCF and ADO.Net Entities, but probably code it "ALL WRONG". I believe ASP.Net MVC isn't even close to handling the LOB apps that I write, but I get paid, and my products are capable of running thousands of concurrent users through them without failing. Sue me. Next year - my code won't look anything like it does today (except true standards/guidelines), as the next ORM, Framework, whatever will be out and about as well as VS 2010 and .Net 4.0, and ASP.Net MVC V 3.221.9.
You may say "Hope I don't run into 'Your' Code"... but the fact is - you probably already have... and you survived just fine.
Ayende, looks like your new best friend at codeproject was the project lead on this.
Tim,
The topics that are discussed here are NOT things that weren't done "exactly" the way I want them.
They are deep, fundamentals, BASIC programming errors.
Love it when people gather in flocks - and you can feel free to STEP RIGHT OFF MY BACK on this one.
No - MVC isn't NEW. ASP.NET MVC is. Internally, that means MVC for the web is NEW because they're used to web forms. I think I deserve a bit more credit then being framed like an MS dolt.
Back to your regularly scheduled rants :).
For those of you looking for a sample, try:
http://code.google.com/p/sharp-architecture/
Sharp Architecture by Billy McCafferty, et. al.
data as object, null as T
I have not laughed that hard in a very long time. Thanks, I needed that.
There are some good samples out there and it's going to depend on your style which one you're going to like best. Take a look at:
CodeCampServer
atomsite
kigg
@rob OK there was more than a slight tone of sarcasm in my comment :)
My point is that whilst OK... asp.net MVC is a new technology, it's by no means a new concept. So any web developer worth their salt should have at least a passing understanding. Especially web developers residing at Microsoft. Or so you would think.
In my (not-so-humble) opinion, the problem isn't that the code is horrid. If it were something intended to be used, then horrid code would be almost excusable (with time pressure, etc). The problem is that Microsoft seems to be pointing at this and explicitly saying "this is how it should be done". When code this bad is held up as the standard, it speaks volumes about the technology itself. (That's why my coworkers refuse to use ASP.NET MVC; the sample code they've seen has been almost as bad.)
The people who designed the technology should have been the ones to write a sample app. I concur with Arthur: the best developers should be writing the sample code, so the lesser developers (such as myself) have a good resource to learn from.
Ayende,
It was not really you I was in a tirade about, actually. Mostly it's the comments that follow; however, I still believe it would be better to not just rip code because of what you beleive to be basic.
There are many out and about that are looking for good examples/best practices. It does NO ONE any good to take a piece of code and say "That Sucks!" and not explain why or show a better way to do it. Better yet - show best practices on the offending method!
Otherwise, you really are just pointing fingers and not really helping the people that really need it. Right? If everyone that read your blog already new how to do it properly, then... why read your blog?
I supposed the offending line to me was:
"I am going to show them both in all their glory, and let you be the judge of them:"
what if I can't be the judge, and they look fine to me?
BTW - it's GREAT to see that they took back the sample (or so I saw when I went to download it) to revamp it.
I don't follow blogs much at all, but I certainly recognize coming to this one often - so you are obviously one of my favorites and I don't mean to offend!
-Tim
Tim,
Some things are basic.
I am not going to repeat advice about STRUCTURED PROGRAMMING that can be traced back to the 60s!
Avoid long methods?
Reduce method complexity?
Avoid magic values?
Don't have methods that does umpteen things?
Guess I just never ran into a full LOB application written by you or any of the other glass-house developers that demand absolutes and follow theories and ideas from the 60s.
I simply feel it's better to teach than to deride - but to each his or her own.
Subjectively, I would argue that the first method isn't "very busy", albeit busy. I've seen huge methods that would dwarf that. Would I take that method and refactor it if it was a production system that had been running for a year and I didn't need to modify anything in it? Nope. Wouldn't bother me a bit to leave it as is...
For some, the finished painting is more important than the brush strokes - and others the brush strokes are the art itself... so again, to each his or her own.
Tim,
Glass house?
Absolutes?
Theories?
By this time, I believe that I have about a substantial amount of code in the open.
Several hundreds of thousand of lines of code, as a matter of fact.
I think that I can safely say that what I am practicing what I am doing.
As for the rest, to be perfectly frank, I am not going to start laying the foundation of OO, structured programming or basic code structure organization in this blog.
It is assumed that you are familiar with them if you read what I write.
As for painting, Art is subjective, but we aren't talking about anything subjective here, you realize, those are objective metrics that the code base fail to measure up against
@Tim, as many have pointed out, we all have written awful codes, or done horrible things. Problem is when they are explicitely published as the official guidance of the correct way we should follow.
Nice Blog...
Comment preview