UnityWeb
Posted by birdchest on 2008-11-26 01:21
Hey so I downloaded the project, but I couldn't get it to build. There seems to be conflicts between the versions of Unity you are using. You probably should clean that up. I did look through the code and saw some things that could be done better.
1.) Your presenters probably should implement Interfaces and those interfaces are the dependencies in your web page.
2.) // Get the controls in the page's control tree excluding the page itself
private static IEnumerable<Control> GetControlTree(Control root)
{
foreach (Control child in root.Controls)
{
yield return child;
foreach (var c in GetControlTree(child))
{
yield return c;
}
}
}
This method is completely wrong (sorry no other way to say that). You only should use yield when creating your own enumerator. It doesn't make sense in this context. I could see if you wanted to make a LazyList, but that doesn't make much sense either.
3.) I changed <%@ Import Namespace="Business" %> to <%@ Import Namespace="Domain.Business" %> in Default.aspx (error), I didn't check it in, so you should probably do it.
4.) what is this "<table cellspacing="0" cellpadding="5">", oh no you went to the dark side.
5.) Other than that, It looked pretty good, I would just try to group the actual MVP core together without your logic code. Create an assembly for just your MVP stuff.
Modules.Base.View.Presenter doesn't really make sense.
A good start though.
4.)
1.) Your presenters probably should implement Interfaces and those interfaces are the dependencies in your web page.
2.) // Get the controls in the page's control tree excluding the page itself
private static IEnumerable<Control> GetControlTree(Control root)
{
foreach (Control child in root.Controls)
{
yield return child;
foreach (var c in GetControlTree(child))
{
yield return c;
}
}
}
This method is completely wrong (sorry no other way to say that). You only should use yield when creating your own enumerator. It doesn't make sense in this context. I could see if you wanted to make a LazyList, but that doesn't make much sense either.
3.) I changed <%@ Import Namespace="Business" %> to <%@ Import Namespace="Domain.Business" %> in Default.aspx (error), I didn't check it in, so you should probably do it.
4.) what is this "<table cellspacing="0" cellpadding="5">", oh no you went to the dark side.
5.) Other than that, It looked pretty good, I would just try to group the actual MVP core together without your logic code. Create an assembly for just your MVP stuff.
Modules.Base.View.Presenter doesn't really make sense.
A good start though.
4.)
Home / Developer API / Tour / Get a Project - Solutions for Bug & Issue Tracking, Collaboration Tools, Subversion Hosting, Git Hosting
Marisic.net is powered by Assembla.
0 Comments