Thursday, April 16, 2009 10:53 PM
kazimanzurrashid
Kobe – MS New Web 2.0 Resource Kit in ASP.NET MVC and My thoughts
[Edit 2: Kobe is now pulled off from the ASP.NET front page as well as from the news section, the msdn page also has a red notice – congrats MS for this step.]
[Edit: There are few people who mentioned that MS is not claiming it as a Best Practice, but I think they are wrong as the moment I am typing this the www.asp.net still has this tag
“The resources are intended to guide you with the planning, architecting, and implementing of Web 2.0 applications and services”
and I do have the objection when it is labeled as guide]
If you recently visited the home page of www.asp.net you will find that Microsoft has released a new Resource Kit for developing Web 2.0 Applications. The resource kit contains a sample reference application that is developed in ASP.NET MVC framework. Since both of these are my area of interest, I downloaded the codes and give a quick walkthrough. Prior getting to the main discussion, I just want to remind you that it is the second reference application (after Oxite) that is named under Microsoft, so you are suppose to get some quality architecture/design, codes etc etc. But unfortunately, it does not even qualify as an average application that is written by joe developer, it is filled with serious flaws in both architectural design and coding and I would consider it as a worst practise of developing ASP.NET MVC application. Let me list the serious issues that I found:
Full of Magic Values in Controller
The controllers contains interesting magic values and left me wondering what's wrong with Enum. Consider the following Action of Home Controller:
[AcceptVerbs(HttpVerbs.Post)]
public ActionResult Discovery(string query, string type, string section, string subSection, string page)
{
query = Server.HtmlEncode(query);
//Paging Section
if (subSection != string.Empty)
{
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-mostactive":
users = _userService.GetMostActiveUsers(query, 8, pageNo);
break;
case "members-all":
users = _userService.GetAllUsers(query, 8, pageNo);
break;
default:
users = _userService.GetAllUsers(query, 8, pageNo);
break;
}
return View("MembersList", users);
}
else if (section == "groups")
{
List<Group> groups = null;
switch (subSection)
{
case "groups-mostactive":
groups = _groupService.GetMostActiveGroupByKeyword(query, 8, pageNo);
break;
case "groups-all":
groups = _groupService.GetAllGroupByKeyword(query, 8, pageNo);
break;
default:
groups = _groupService.GetAllGroupByKeyword(query, 8, pageNo);
break;
}
return View("GroupsList", groups);
}
else if (section == "presentations")
{
List<PresentationSummary> presentations = null;
switch (subSection)
{
case "presentations-all":
presentations = _presentationService.GetAllPresentationsByKewordTimeLine(query, "Day", "0", 10, pageNo);
break;
case "presentations-mostpopular":
presentations = _presentationService.GetMostPopularPresentationsByKewordTimeLine(query, "Day", "0", 10, pageNo);
break;
case "presentations-mostviewed":
presentations = _presentationService.GetMostViewedPresentationsByKewordTimeLine(query, "Day", "0", 10, pageNo);
break;
case "presentations-mostdownload":
presentations = _presentationService.GetMostDownloadedPresentationsByKewordTimeLine(query, "Day", "0", 10, pageNo);
break;
case "presentations-new":
presentations = _presentationService.GetNewPresentations(query, "Day", "0", 10, pageNo);
break;
default:
presentations = _presentationService.GetAllPresentationsByKewordTimeLine(query, "Day", "0", 10, pageNo);
break;
}
return View("PresentationsList", presentations);
}
// return new ContentResult { Content = "" };
}
return View();
}Can you count the number of Magic string and integers in the above method?
Lack of Knowledge of ActionResult
Whenever the Action methods requires to return an empty result (which I think is also a design flaw) instead of EmptyResult it is returning new ContentResult { Content = "" }. Also when downloading a file, instead of using those nice FileResult, FilePathResult etc the people behind this application decided to modify the Response object directly (Check the Download method of Presentation Controller) which clearly indicates the lack of knowledge of ASP.NET MVC Framework.
Action Methods are Populating Too Many View Data
Rather than populating the common view data with action filters or dividing the parts by Controller.RenderAction, the action methods are resposible to populate too many view data, consider the following action method:
[AcceptVerbs(HttpVerbs.Get)]
public ActionResult Index()
{
UserSummary memberOfDay = _communityService.GetUserOfDay();
PresentationSummary presentationOfDay = _presentationService.GetPresentationOfDay();
Group groupOfDay = _communityService.GetGroupOfDay();
List<PresentationSummary> favouritePresentation = _communityService.GetCommunityFavorite(10,1);
List<PresentationSummary> userPresentationCount = _userService.GetAuthoredPresentations(memberOfDay.UserName);
List<UserSummary> newUsers = _communityService.GetNewUsers(8,1);
List<UserSummary> mostActiveUsers = _communityService.GetMostActiveUsers(8, 1);
List<UserSummary> allUsers = _communityService.GetAllUsers(8, 1);
int iNewMemberPageCount = decimal.Divide(_communityService.GetAllUsers().Count,8).ToInt();
List<Group> newGroups = _communityService.GetNewGroups(8, 1);
List<Group> mostActiveGroups = _communityService.GetMostActiveGroups(8, 1);
List<Group> allGroups = _communityService.GetAllGroups(8, 1);
int iNewGroupPageCount = decimal.Divide(_communityService.GetAllGroups().Count, 8).ToInt();
int ifavouritesPageCount = 3;
List<Group> groupsCommunity = _communityService.GetNewGroups(8,1);
ViewData.Add("favouritePresentations", favouritePresentation.ToArray());
ViewData.Add("UserOfDay", memberOfDay);
ViewData.Add("GroupOfDay", groupOfDay);
ViewData.Add("UserPresentationCount", userPresentationCount.Count);
ViewData.Add("presentationsOfDay", presentationOfDay);
ViewData.Add("IsCommunityPage", true);
ViewData.Add("members-new", newUsers);
ViewData.Add("members-mostactive", mostActiveUsers);
ViewData.Add("members-all", allUsers);
ViewData.Add("members-totalcount", iNewMemberPageCount);
ViewData.Add("groups-new", newGroups);
ViewData.Add("groups-mostactive", mostActiveGroups);
ViewData.Add("groups-all", allGroups);
ViewData.Add("groups-totalcount", iNewGroupPageCount);
ViewData.Add("favourites-totalcount", ifavouritesPageCount);
ViewData.Add("GroupsListCommunity", groupsCommunity);
return View();
}Same Action Method Returns Multiple Irrelevant View
Consider the following action:
[AcceptVerbs(HttpVerbs.Post)]
public ActionResult Profile(string userId, string type, string section, string subSection, string page)
{
string argUserName = userId;
UserSummary _user = null;
_user = _userService.GetUserByName(argUserName);
if (argUserName == HttpContext.User.Identity.Name)
ViewData["UserLogged"] = "Yes";
else
ViewData["UserLogged"] = "No";
ViewData["User"] = _user;
//Paging Section
if (subSection != string.Empty)
{
if (type != "widget")
return new ContentResult { Content = "" };
int pageNo = 1;
Int32.TryParse(page, out pageNo);
if (pageNo == 0)
pageNo = 1;
switch (section)
{
case "profileContacts":
switch (subSection)
{
case "profile-contacts":
AddContactsToViewData(_user, pageNo);
return View("UserContactList");
case "profile-groups":
AddGroupsToViewData(_user, pageNo);
return View("UserGroupList");
case "profile-authors":
AddAuthorsToViewData(_user, pageNo);
return View("UserAuthorList");
}
break;
case "messages":
switch (subSection)
{
case "wall-messages":
AddUserMessagesToViewData(_user, pageNo);
return View("Messages", ViewData["wallMessage"]);
case "wall-userinvites":
AddUserInvitationToViewData(_user, pageNo);
return View("UserInvites", ViewData["ContactInvites"]);
case "wall-groupinvites":
AddGroupInviteToViewData(_user, pageNo);
return View("GroupInvites", ViewData["GroupInvites"]);
}
break;
case "profile":
switch (subSection)
{
case "profile-recommendedForYou":
AddSystemRecommendationToViewData(_user, pageNo);
return View("SystemRecommended", ViewData["profile-recommendedForYou"]);
case "profile-contactRecommendations":
AddContactRecommendationToViewData(_user, pageNo);
return View("ContactsRecommended", ViewData["profile-contactRecommendations"]);
case "profile-myFavorites":
AddFavoriteToViewData(_user, pageNo);
return View("FavoritePresentations", ViewData["profile-myFavorites"]);
case "profile-myPresentations":
AddPresentationAuthoredToViewData(_user, pageNo);
return View("AuthoredPresentations", ViewData["profile-myPresentations"]);
}
break;
}
}
return null;
}The same action is responsible for returning 11 different view!!!
VB6 Style variable naming
I thought we have already passed those days, but it proves me wrong:
string regStatus = _userService.RegisterUser(username, passwordRegistration, firstname, lastname, email, arrInterests);
if (regStatus == "Success")
{
UserSummary user = _userService.GetUserByName(username);
string callBackURL = Request.Url.ToString().Replace("Registration", "EmailConfirmation") + "/" + user.UserId.ToString();
string strMessageBody = GetRegistrationConfirmationMailBody(firstname, callBackURL);
string strMailSubject = "PlanetPPT - confirm you email";
this.SendMail("admin@planetppt.com", user.Email, strMessageBody, strMailSubject);
//More Codes
}Check that the UserService is returning string to indicate success/failure and the controller itself is responsible for formatting and sending the email.
Still Provider Model
The Data Access is designed with the similar style of Oxite v1.0, which the community has rejected a long time ago.
Zero Unit Test
The application does not contain a single line of unit test and the worst thing is that the way it has been architect there is zero chance that you will be able to write it without refactoring the architecture.
This is so far I think it worth to investigate and I can assure you that you will be able to find more flaws if you have the time and patience.
At last, my urge to Microsoft, please take down this application from both www.asp.net and MSDN or put a notice that the application is not up to the standard and do not follow it anyway as it is full of bad practices. Do introduce a review board and get the approval before releasing this kind of application. Remember this is not the first time that this kind of controversy was born, so please ensure the quality and standard.
Filed under: Asp.net, MVC, ASPNETMVC, ASP.NET MVC