Search

Categories

 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 

Send mail to the author(s) E-mail

# Tuesday, February 23, 2010

This is useful to get checking on the aspx page at compile time. 

Right click on the project, and select unload.

Edit the .csproj file

image

Set MvcBuildViews to true

Save, reload, compile.

Comments [0] | | # 
# Monday, February 22, 2010

The Hands on MVC set of videos:  www.learnvisualstudio.net

Routing

In global.asax

 routes.MapRoute(
"Default", // Route name
"{controller}/{action}/{id}", // URL with parameters
new {controller = "Home", action = "Index", id = ""} // Parameter defaults
);

This means eg http://localhost:3203/Home/About
will come to the controller called HomeController and run the method called About
 
public ActionResult About()
{
return View();
}

which in this case will call the view with the same name called About.aspx

What about http://localhost:3203/ which is the same as /Home and /Home/Index

In this case the defaults are being used eg Home, Index , null… as shown above in //Parameter defaults.

It even passes a message from the Home controller, Index method.

ViewData["Message"] = "Welcome to ASP.NET MVC!";
 
Hardcode /Home to mean something
// for hardcoded /home goto home/member
routes.MapRoute(
"MemberHome",
"home",
new {controller = "Home", action = "Member"}
);

Building UI Form

get is putting in query string

post is putting in the http request body

utility methods for not directly specifying URL


<%Html.BeginForm(); %>






<% Html.EndForm(); %>

Useful to see the Request coming back from /home/member into the HomeController, Member method.

image

asdf

image

Yuk!

Post redirect get pattern.

Basically stops the above resend message by getting the browser to do a redirect after the post page, to a nice get page.

[Authorize]
[AcceptVerbs(HttpVerbs.Post)]
public ActionResult Member(string status) // this will match up the name status in the forms collection
{
//return View();
return RedirectToAction("Member");
}

The Model using LinqToSQL

Added a simple table with a FK to the aspnet_users UserId column.

 

image


The UserId FK is a guid.

The Timestamp has a default value of GETUTCDATE() so that the db writes in the timestamp.

LinqToSQL has:

readonly – shouldn’t allow app to change it

auto Generated Value = true   - hmm to do with:

Auto_Sync – so it will auto get the timestamp property back into the model whenever it does an insert.

 

Can see the class generated that represents our StatusUpdate table:

image

The View

In the controller:

List updates = db.GetUserUpdates(User.Identity.Name);
return View(updates);

and in the view:

System.Web.Mvc.ViewPage>

we now get a strongly typed object

 

View Models

Have simply created another class called MemberViewModel in Models:

public class MemberViewModel
{
public List Updates { get; set; }
}
 
so can pass in more complex stuff than just a list into my view (can’t pass in multiple lists)
 
So the view now has:
 
System.Web.Mvc.ViewPage

and we can access data like:
 
foreach (var update in Model.Updates)

 

Lazy Loading and Eager Loading.

However we’ve hit an interesting issue:

image

this is because of in the view we’ve called:

Html.Encode(update.User.UserName)

And in the actual query, it returns only the StatusUpdate object and not the User object (even though it does a join).

var updates = from update in this.StatusUpdates
where update.User.UserName == userName
orderby update.Timestamp descending
select update;
 
in the controller after query is ran and the result put into the ViewModel, then passed to the View… the datacontext is destroyed.
 
using (var db = new TwixDataContext())
{
ViewData["Stuff"] = "Blah this is a generic object with no intellisense";

MemberViewModel model = new MemberViewModel
{
Updates = db.GetUserUpdates(User.Identity.Name)
};
//List updates = db.GetUserUpdates(User.Identity.Name);
return View(model);
}


So when the view tries to call the User.UserName.. LinqToSQL tries to go and get this data, as it hasn’t been needed yet.  This is lazy loading.

We want LinqToSQL to load the User object at the same time as loading the StatusUpdates.. this is Eager Loading.

CSS

Making things look good:

#timeline { list-style: none; padding: 0;}
#timeline li {border-top:dashed 1px #888; padding: 5px 0; }
#timeline li:hover { background-color: #eee; }
#timeline li div.message { font-size: 10pt; }
#timeline li div.message span { font-weight: bold; }
#timeline li div.time { font-size: 8pt; font-style: italic }

for:

    "timeline">
    <%foreach (var update in Model.Updates) {%>


  • class="message">
    <%= Html.Encode(update.User.UserName) %>:
    <%=Html.Encode(update.Message) %>

    class="time">
    <%=update.Timestamp.ToLocalTime().ToString() %>


  • <%} %>
Creating a Home/Profile

coppied the get from index, however instead of passing in the current logged in user to GetUserUpdates, am passing in the string ie it will be localhost/nameofperson or localhost/home/profile/nameofperson

Also created a new viewmodel for profile that has username in it (as we’ll need this).  Strongly typed as well.

public class ProfileViewModel
{
public string UserName { get; set; }
public List Updates { get; set; }
}

Partial View (#7)

create a viewmodel just for the timeline ie this partial view.

changing MemberViewModel and ProfileViewModel to have a TimeliveViewModel (abstracting it away to get rid of duplication)

public class MemberViewModel
{
public TimelineViewModel TimelineModel { get; set; }
}

public class TimelineViewModel
{
public List Updates { get; set; }

So we reference it in the controller like this:

ProfileViewModel model = new ProfileViewModel
{
UserName = userName,
TimelineModel = new TimelineViewModel{
Updates = db.GetUserUpdates(userName)
}
};
and then in the view:
<%foreach (var update in Model.TimelineModel.Updates) {%>

Hyperlinks, Anonymous Type - create new view as Timeline.ascx

<%Html.RenderPartial("Timeline", Model.TimelineModel); %>
 
<%Html.ActionLink(update.User.UserName, "profile", new {userName = update.User.UserName}) %>

First property is the text to be displayed, second is the action to be called when they click this link (method on this controller),

We’re not specifying the controller here, so it is going to user the current (home) controller.

The third is a generic object which represents the extra route values that we want.. as the method takes a parameter called userName, we need to pass it. We use an anonymous type

 

image

The crowd goes wild :-)

Seeing Other Peoples Tweets

A common technique when we need a Many to Many relationship. 

each user can follow many other users

each user can be followed by many other users

image

The primary key is both on the FollowerMappings table.

Then in the Mappings:

image

We have renamed the child and parent properties for clarity.

The first FK relationship is the FollowerId being held constant.  So the Child Property is the Followee (all the people the user is following)

The second FK relationship is the FoloweeId being held contact.  So the Child Property is the Follower (all people who are following the user).

**TODO refactor and come up with better names.

Adding Methods to The Model DataContext

GetAllUpdates – gets all status update records including those psoted by user that this user is following

IsFollowing – checks if a specific user name is following another user name… so we know whether to display a button

DeleteFollowerMapping

Adding Methods to the HomeController

Changed the get method on Home/Profile to GetUserUpdates.. and to do logic for displaying the Follow/Stop Following button.

Added a post method for Follow (which is called from Profile as a form post), and redirects back to Profile.

 

[Authorize]
[AcceptVerbs(HttpVerbs.Post)]
public ActionResult Follow(string userName)
{
if (!string.IsNullOrEmpty(userName) && !userName.Equals(User.Identity.Name, StringComparison.OrdinalIgnoreCase))
{
using (var db = new TwixDataContext())
{
if (!db.IsFollowing(User.Identity.Name, userName)) // double check they are not already being followed
{
var followerMapping = new FollowerMapping
{
FollowerId = db.GetUserIdForUserName(User.Identity.Name),
FolloweeId = db.GetUserIdForUserName(userName)
};

db.FollowerMappings.InsertOnSubmit(followerMapping);
db.SubmitChanges();
}
}
}
return RedirectToAction("Profile", new {userName = userName});
}

Added a post method for StopFollowing.. same as above.

Adding Properties to the ProfileViewModel

public class ProfileViewModel
{
public string FollowText { get; set; }
public string FollowAction { get; set; }
public bool IsSelf { get; set; }
public string UserName { get; set; }
public TimelineViewModel TimelineModel { get; set; }
}
So when we pass this object to the view, we’ve got lots of strongly typed goodness.

Using in the View

using (Html.BeginForm(Model.FollowAction, "home")) {
%>
<%=Html.Hidden("userName", Model.UserName) %>

<%
}

where the brace ends, is just the same as doing an Html.EndForm.

 

Best Practise

Use a form post instead of a link if doing database stuff to prevent hacking.

Summary so Far

Why do we have so much business logic in the controller?

DSCN1403

next video is #9

why is this line sometimes giving Http Exception in home controller.

var isSelf = userName.Equals(User.Identity.Name, StringComparison.OrdinalIgnoreCase);

#9 – is jQuery.. thats all folks or more stuff.

#10 – is account settings.. name, email, timezone.. form field validation… combo box (dropdown)… nice fade out jquery front end.

#11 – is upload photo

#12 – is search.. ?q=learnvisualstudio… not a post… stuff in his source code that isn’t in video. icon/link delete a post.

Comments [0] | | # 
# Friday, February 19, 2010

Pairing with Michael, we did our first spike from here:

http://ericdotnet.wordpress.com/2009/04/09/jquery-search-box-and-aspnet-mvc/

worked fine locally, but then going live getting some issues.  Looked like it was mateerit.co.nz/search

<script src=" <%=Url.Content("~/Scripts/jquery-1.3.2.js") %>" type="text/javascript"></script>

The above seemed to work, however the javascript seemed harder:

<script type="text/javascript">

$(document).ready(function() {
$("#searchTerm").autocomplete("/Home/getAjaxResult/");
});

</script>

so I cheated and went onto a subdomain:  jsearch.mateerit.co.nz

All works now with firebug too.

image

Comments [0] | | # 

I use vimperator, however sometimes I like to have the normal menus back.  The command I have to type is:  set guioption+=mT

Trick is to download autohotkey:

Run Autoscriptwriter (recorder)

save the file, and assign a hotkey.  In this case Windows Key, s

#s::  - set guioptions
Send, {SHIFTDOWN};{SHIFTUP}set{SPACE}guioptions{SHIFTDOWN}={SHIFTUP}=m{SHIFTDOWN}t{SHIFTUP}{ENTER}

#t:: – tv on

#8:: - 85hz

Then put the files in the startup script directory: C:\Documents and Settings\dave\Start Menu\Programs\Startup

Comments [0] | | # 
# Tuesday, February 16, 2010

http://www.mikesdotnetting.com/Article/128/Get-The-Drop-On-ASP.NET-MVC-DropDownLists

1) SelectListItem – Value and Text property assigned

var db = new northwindDataContext();
IEnumerable<SelectListItem> items = db.Categories.Select(c => new SelectListItem {Value = c.CategoryID.ToString(), Text = c.CategoryName});
ViewData["CategoryID"] = items;
 
in the view:
<%= Html.DropDownList("CategoryID") %>

which is the pretty much the same as:
 
<%= Html.DropDownList("CategoryID", (IEnumerable<SelectListItem>)ViewData["Categories"]) %>

2) SelectList is tidier, and gives a couple of overloads including selected (in the case below number 3):
 
var query = db.Categories.Select(c => new {c.CategoryID , c.CategoryName});
ViewData["Categories"] = new SelectList(query.AsEnumerable(), "CategoryID", "CategoryName",3);

image
 

PostBack

On the product view:

<% using (Html.BeginForm(null, null, FormMethod.Post, new { id = "TheForm" })) {%> 
<%=Html.DropDownList("CategoryID", (SelectList)ViewData["Categories"], new { onchange="this.form.submit();"})%>
<%}%>

and the product controller:

public ActionResult Index(int? categoryid)
{
var db = new northwindDataContext();

var query = db.Categories.Select(c => new {c.CategoryID, c.CategoryName});
ViewData["Categories"] = new SelectList(query.AsEnumerable(), "CategoryID", "CategoryName");

List<Product> products;
if (categoryid == null)
products = db.Products.ToList();
else
products = (from p in db.Products
where p.CategoryID == categoryid
select p).ToList();
return View(products);
}

which gives:
 
image 
Used LinqToSQL.
ASP.NET MVC version 1
 

Run Implementation

Found I had to sort a generic list
 
The crowd goes wild:
image
Comments [0] | | # 
# Monday, February 15, 2010

Both my asp.net web hosters advertised they are hosting on Server 2008, which meant IIS7.  This makes things much easier when publishing an MVC site (to do with routing apparently).  As I’d hosted my sites for years with these providers, they were still on Win2003 (IIS6) boxes.  I just had to ask for an upgrade and it was done.

Upload to IIS7 and it just works!

The first step was to see if routing worked on the default website.. it did!

The next step – to publish up the NerdDinner app I’ve been going through on Rob Conery, and get the database connected live.

image

The crowd goes wild  - this is live and connecting to a database :-)

Changing Namespace / Solution Name / Project Names

I had lots of namespace errors.. forgot to change default assembly namespace, which meant linq to sql was in the wrong space.  However it is possible to change everything very quickly – solution, project names, directly.  One gotcha was remembering to delete everything on the live server so there weren’t 2 dll’s with the same object names inside..

Changing Default Page to Activity / upload live trick

routes.MapRoute(
"Default", // Route name
"{controller}/{action}/{id}", // URL with parameters
new { controller = "Activity", action = "Index", id = "" } // Parameter defaults
);

Change controller = “Home” to controller = “Acitivity”

I also implemented the copyifnewer.bat trick for easily going from dev to live which is here: http://www.programgood.net/2010/01/28/WebConfigAutomatingDevAndLive.aspx

Run application working live with Create, Read, Update and Delete working.  Also validation, which comes for free in the standard templates.

image

DropDownList and Postbacks – covered in another post on this site.

http://www.mikesdotnetting.com/Article/128/Get-The-Drop-On-ASP.NET-MVC-DropDownLists

Summary Page

I created another link in the master page for menu item:

www.mateerit.co.nz/run

image

For the summary page I’m considering a ViewModel approach.

Lets see if a simple new Summary controller will suffice

Got to a stage.. was going to upload to github, but had db connection strings in there which didn’t want in the history.. so looked at rebase in git.. want to make sure before I play too much :-)

 

Creating a SQL View then Displaying it

Michael came up with an elegant solution which I’m trying.. so creating a SQL View:

SELECT     personid,    
DATEPART(week, date) AS Week,
DATEPART(year, date) AS Year,
SUM(kilometres) AS kilometres,
SUM(hours) AS hours
FROM dbo.Activity
GROUP BY personid, DATEPART(week, date), DATEPART(year, date)

which gives a good summary output:

image

The aim is to be able to see the summary of each week like this (this is the old webforms version)

image

linq to sql generated classes off the view.

Michael did it like this:

image

I’m on this:

image

Passing Complex Data – Lists within Lists

As each person has data associated to them, a list within a list is good.

image

public class WeekSummary
{
public int Personid;
public string Personname;
public List<WeekSummaryData> Persondata;
}

public class WeekSummaryData
{
public double? Hours;
public double? Kilometers;
public int? Week;
public int? Year;
}

Then in the repository:

public IQueryable<WeekSummary> GetWeeklySummary()
{
var weekSummaries = from p in db.Persons
orderby p.personname
let data = GetWeeklySummaryData(p.personid)
select new WeekSummary {Personid = p.personid,
Personname = p.personname,
Persondata = new List<WeekSummaryData>(data)};
return weekSummaries;
}

IQueryable<WeekSummaryData> GetWeeklySummaryData(int personid)
{
var summary = from w in db.weekly_summaries
where w.personid == personid
orderby w.Year , w.Week
select new WeekSummaryData {Hours = w.hours,
Kilometers = w.kilometres,
Week = w.Week,
Year = w.Year};
return summary;
}

** understand this linq

Then in the controller:

public ActionResult Index()
{
var summaries = _repository.GetWeeklySummary();
return View(summaries);
}

and the view:

** understand this rendering

<% foreach (WeekSummary weekSummary in Model) {
%>
<table>
<tr>
<th></th>
<% foreach (WeekSummaryData weekSummaryData in weekSummary.Persondata) { %>
<th><%=Html.Encode(weekSummaryData.Week) + "/" + Html.Encode(weekSummaryData.Year) %></th>
<%}%>
</tr>
<tr>
<td>
<%=Html.Encode(weekSummary.Personname) %>
</td>
<% foreach (WeekSummaryData week in weekSummary.Persondata) { %>
<td><%=Html.Encode(week.Hours) + "hrs" %><br /><%=Html.Encode(week.Kilometers) + "kms" %></td>
<%}%>
</tr>
</table>
<% } %>

 

refactor code with an interface for testing

unit test do

automock

jquery

tooling to help make form crud websites faster…

reproting tooling

mvc2

Comments [0] | | # 
# Friday, February 12, 2010

While pair programming with some great developers I asked the question:  How would you refactor this project (the running project website).

When both of the guys I paired with suggested that it would be smart to move to MVC and to use an ORM, I listened.

After downloading ASP.NET MVC version 1.  I’ve been working through the tutorial from Rob Conery:

http://www.asp.net/learn/mvc-videos/video-8143.aspx (about 80mins)

Creating a New MVC Project

Ctrl Shift N

Linq to SQL

image

going to use Membership for login / users.

In models, add new item: LINQ to SQL

drag on the two tables

image

Original table name was dinners.  The class is called dinner

Renamed NerddinnerDataContect to DB, and context namespace to Nerddinner

however Entity Namespace to Nerddinner.Model – which didn’t work!  I left it blank.

Adding a Controller

The default Index controller

var db = new DB();
var dinners = db.Dinners;
return View(dinners);

Adding a View

right click inside method in controller

Create strongly-typed view

View content – code gen nice.

Tooling figured out which is the primary key (as linq to sql)

 

ModelBinders - Add

When creating a new Dinner, we use ModelBinders.. mvc looks at the Object and sees if it can bind to the linqtosql.

Could have used the existing FormCollection, but this is easier!

If ModelState.IsValid   .. nice.

image

And by changing the Html.ValidationMessage

image

Editing Data

In the controller:

public ActionResult Edit(int id)
{
var db = new DB();
var dinner = db.Dinners.SingleOrDefault(x => x.DinnerID == id);
return View(dinner);
}

This is another way of expressing the above:

public ActionResult Edit(int id)
{
var db = new DB();
//var dinner = db.Dinners.SingleOrDefault(x => x.DinnerID == id);
var dinner = (from a in db.Dinners
where a.DinnerID == id
select a).SingleOrDefault();

return View(dinner);
}

<% gator tags

Other view engines – Spark or nHaml

image

So we’ve built scaffolding.

Routing

We want /dinners to the be root of the site.

Routing is the thing that listens for URL’s and passes them to a controller.

eg Dinner / Edit /5

eg { controller} / {action} / {id}

To change to be /dinners as the route in global.asax

static public void RegisterRoutes(RouteCollection routes)
{
routes.IgnoreRoute("{resource}.axd/{*pathInfo}");

routes.MapRoute(
"Default", // Route name
"{controller}/{action}/{id}", // URL with parameters
new {controller = "Dinner", action = "Index", id = ""} // Parameter defaults
);
}

We changed from Home to Dinner above.

Deleting

image

[AcceptVerbs(HttpVerbs.Post)]
public ActionResult Delete(int id)
{
var db = new DB();
var dinner = (from a in db.Dinners
where a.DinnerID == id
select a).SingleOrDefault();
db.Dinners.DeleteOnSubmit(dinner);
db.SubmitChanges();

return RedirectToAction("Index");
}

are wired up on the view:

<% using (Html.BeginForm("delete", "dinner")) {%>
<%=Html.Hidden("id",Model.DinnerID) %>
<input id="Submit1" type="submit" value="delete" />
<% } %>

We are not using any authentication to stop people doing this at the moment. For that we need:

[AcceptVerbs(HttpVerbs.Post)]
[Authorize(Roles="Administrator")]
public ActionResult Delete(int id)
{
var db = new DB();

Testing / Repository Pattern

Because the controller has database connection inside of it, this isn’t good from a testing point of view, so we’re going to refactor this out.

Speed would be a problem with lots of tests, as would not knowing what is in the database.

Created an IDinnerRepository interface:

public interface IDinnerRepository
{
IQueryable<Dinner> FindAllDinners();
Dinner GetDinner(int id);
void Add(Dinner dinner);
void Update(Dinner dinner);
void Delete(Dinner dinner);
void Save();
}

Then a SqlDinnerRepository that implements this, where the constructors calls the DB:

public class SqlDinnerRepository : IDinnerRepository
{
DB db;

public SqlDinnerRepository()
{
db = new DB();
}

public IQueryable<Dinner> FindAllDinners()
{
return db.Dinners;
}

public Dinner GetDinner(int id)
{
var dinner = (from a in db.Dinners
where a.DinnerID == id
select a).SingleOrDefault();

return dinner;
}

Then in the Dinner Controller:

public class DinnerController : Controller
{
IDinnerRepository _repository;

public DinnerController()
{
_repository = new SqlDinnerRepository();
}

// this will be the one that the test uses
public DinnerController(IDinnerRepository repository)
{
_repository = repository;
}

//
// GET: /Dinner/
public ActionResult Index()
{
//var db = new DB();
//var dinners = db.Dinners;
var dinners = _repository.FindAllDinners();
return View(dinners);
}

The plan is that we will make up something that implements IDinnerRepository when testing to make our life easier.

DSCN1379_dave

Made: FadinnerkeDinnerRepository

Made: DinnerControllerTests

Ctrl R T – Run tests

Found error in test code:

 //var data = result.ViewData.Model as IList<Dinner>; // this didn't work.. hmmmmmmm.
var data = result.ViewData.Model as IEnumerable<Dinner>;

TDD

Made a test that dinners should only return dates that are today or later:

[TestMethod]
public void Index_Should_Return_Dinners_For_Today_Or_Later() {
// Arrange
var controller = new DinnerController(new Fakes.FakeDinnerRepository());

// Act
//ViewResult result = (ViewResult) controller.Index();
var result = controller.Index() as ViewResult;

// Assert
var data = result.ViewData.Model as IEnumerable<Dinner>;
Assert.IsFalse(data.Where(x=>x.EventDate<DateTime.Now).Count()>0);
}

Failed the test, then wrote the code which was in the DinnerController:

var dinners = _repository.FindAllDinners().Where(x=>x.EventDate >= DateTime.Now);

 

Lambda expressions

var dinners = _repository.FindAllDinners().Where(x=>x.EventDate >= DateTime.Now);

 

Presentation Model Pattern or View Model

45min into Rob’s video.

Comments [0] | | # 

No error.  Just closes when using ASP.NET MVC

http://stackoverflow.com/questions/1361291/resharper-r-4-5-and-mvc-1-0-solutions-cause-visual-studio-2008-sp1-to-crash

then onto:

http://stackoverflow.com/questions/500696/why-does-visual-studio-crash-opening-aspx-with-mvc-rc1

Have tried the one above, which seems to work so far.

[Edit] it didn’t.. so now I’m trying this:

https://connect.microsoft.com/VisualStudio/Downloads/DownloadDetails.aspx?DownloadID=16827&wa=wsignin1.0

Something still weird as locking up when loading assemblies (looks like resharper)  am trying resharper5 beta2 to see if that helps (haven’t got correct license).  I can get around lockup by deleting dlls from bin and obj.

Comments [0] | | # 
# Thursday, February 11, 2010

Static classes are usually used as ‘utility’ classes

You don’t need an instance

 

class Program
{
static void Main()
{
double result = thing.daveSubtract(10);
Console.WriteLine(result);
}
}

static class thing
{
static public double daveSubtract(double number)
{
return (number - 2);
}
}

Extension Methods

From: http://msdn.microsoft.com/en-us/library/bb383977.aspx

Extension methods enable you to "add" methods to existing types without creating a new derived type,

Extension methods are a special kind of static method, but they are called as if they were instance methods on the extended type.

For client code, there is no apparent difference between calling an extension method and the methods that are actually defined in a type.

 

using System;

namespace ConsoleApplication1
{
class Program
{
static void Main(string[] args)
{
string s = "hello extension methods are good";
Console.WriteLine(s);
int i = s.WordCount();
Console.WriteLine("number of words is {0}", i.ToString());
}
}

public static class MyExtensions
{
public static int WordCount(this String str)
{
return str.Split(new char[] { ' ', '.', '?' }, StringSplitOptions.RemoveEmptyEntries).Length;
}
}
}

Comments [0] | | # 
# Monday, February 08, 2010

From Uncle Bobs book clean code example on refactoring:

    {
        static void Main()
        {
            int[] result = GeneratePrimes.generatePrimes(30);
            foreach (var i in result)
                Console.Write(i.ToString() + ", ");
        }
    }
 
    /// <summary>
    /// Given an array of integers starting at 2, cross out the multiples of 2.  Fine the next
    /// uncrossed integer, and cross out all of its multiples.
    /// Repeat until you have passed the square root of the maximum value
    /// </summary>
    public class GeneratePrimes
    {
class Program
        public static int[] generatePrimes(int maxValue)
        {
            if (maxValue >= 2) // the only valid case
            {
                // declarations
                int s = maxValue + 1; // size of the array
                bool[] f = new bool[s];
                int i;
 
                // initialize array to be true
                for (i = 0; i < s; i++)
                {
                    f[i] = true;
                }
 
                // get rid of non-primes
                f[0] = f[1] = false;
 
                //sieve
                int j;
                for (i = 2; i < Math.Sqrt(s) + 1; i++)
                {
                    if (f[i]) // if i is uncrossed, cross its multiples
                    {
                        for (j = 2 * i; j < s; j += i)
                            f[j] = false; // multiple is not a prime
                    }
                }
 
                // how many primes are there?
                int count = 0;
                for (i = 0; i < s; i++)
                {
                    if (f[i])
                        count++; // bump count
                }
 
                int[] primes = new int[count];
 
                //move the primes into the result
                for (i = 0, j=0;i<s ; i++)
                {
                    if (f[i])
                        primes[j++] = i;
                }
 
                return primes; // return the primes
            } else // maxvalue < 2
                return new int[0]; // return null array if bad input
        }
    }

This is messy code because:

  • Variable names are cryptic
  • Some comments are unnecessary

Here is Bob’s refactored version:

/// <summary>
    /// Generates prime number up to a user specified maximum
    /// The algorithm used is the Sieve of Eratosthenes.
    /// Given an array of integers starting at 2:
    /// Find the first uncrossed (eg 3 ) integer, and cross out all its multiples (eg 6,9,12,14)
    /// Repeat until there are no more multipes in the array
    /// </summary>
    class PrimeGenerator
    {
        static bool[] crossedOut;
        static int[] result;
 
        public static int[] generatePrimes(int maxValue)
        {
            if (maxValue < 2)
                return new int[0];
            else
            {
                uncrossIntegersUpTo(maxValue);
                crossOutMultiples();
                putUncrossedIntegersIntoResult();
                return result;
            }
        }
 
        static void uncrossIntegersUpTo(int maxValue)
        {
            crossedOut = new bool[maxValue + 1]; // as bool array starts at 0, and if maxvalue = 10, we need an array of length 11
            for (int i = 2; i < crossedOut.Length; i++) // less than as we don't want to reference crossedOut[4] which doesn't exist
                crossedOut[i] = false;
        }
 
        static void crossOutMultiples()
        {
            int limit = determineIterationLimit();
            for (int i = 2; i <= limit; i++)
                if (notCrossed(i))
                    crossOutMultiplesOf(i);
        }
 
        static int determineIterationLimit()
        {
            // Every multiple in the array has a prime factor that
            // is less than or equal to the square root of the array size, 
            // which is the largest number we are trying to find primes in.
            // So we don't have to cross out numbers
            // larger than that square root of the maxnumber, as they will have been crossed out already
            double iterationLimit = Math.Sqrt(crossedOut.Length);
            return (int) iterationLimit;
        }
 
        static void crossOutMultiplesOf(int i)
        {
            for (int multiple = 2 * i; multiple < crossedOut.Length; multiple += i)
                crossedOut[multiple] = true;
        }
 
        static bool notCrossed(int i)
        {
            return crossedOut[i] == false;
        }
 
        static void putUncrossedIntegersIntoResult()
        {
            result = new int[numberOfUncrossedIntegers()];
            for (int j = 0, i = 2; i < crossedOut.Length; i++)
                if (notCrossed(i))
                    result[j++] = i;
        }
 
        static int numberOfUncrossedIntegers()
        {
            int count = 0;
            for (int i = 2; i < crossedOut.Length; i++)
                if (notCrossed(i))
                    count++;
            return count;
        }
    }

I like Bobs version because it splits the code into simple manageable chunks eg:

    uncrossIntegersUpTo(maxValue);
    crossOutMultiples();
    putUncrossedIntegersIntoResult();

I paired with a friend over the weekend, and we thought this was better:

  • Initialised crossedOut in generatePrimes method instead of ‘child’ method
  • Pass in crossedOut as a parameter instead of a class scope variable
  • Took out (defactored), the notCrossed(i) method as !crossedOut[i] is very readable inline.
  • Make everything non static
  • Keep in /// summary method so intellisense works in the IDE – am trialling Ghostdoc.
/// <summary>
    /// Generates prime number up to a user specified maximum
    /// The algorithm used is the Sieve of Eratosthenes.
    /// Given an array of integers starting at 2:
    /// Find the first uncrossed (eg 3 ) integer, and cross out all its multiples (eg 6,9,12,14)
    /// Repeat until there are no more multipes in the array
    /// </summary>
    class PrimeGenerator
    {
        public int[] generatePrimes(int maxValue)
        {
            bool[] crossedOut;
 
            if (maxValue < 2)
                return new int[0];
            else
            {
                crossedOut = new bool[maxValue + 1];
                uncrossIntegersUpTo(crossedOut);
                crossOutMultiples(crossedOut);
                return putUncrossedIntegersIntoResult(crossedOut);
            }
        }
 
        void uncrossIntegersUpTo(bool[] crossedOut)
        {
            // as bool array starts at 0, and if maxvalue = 10, we need an array of length 11
            for (int i = 2; i < crossedOut.Length; i++) // less than as we don't want to reference crossedOut[4] which doesn't exist
                crossedOut[i] = false;
        }
 
        void crossOutMultiples(bool[] crossedOut)
        {
            int limit = determineIterationLimit(crossedOut);
            for (int i = 2; i <= limit; i++)
                if (!crossedOut[i])
                    crossOutMultiplesOf(crossedOut, i);
        }
 
        int determineIterationLimit(bool[] crossedOut)
        {
            // Every multiple in the array has a prime factor that
            // is less than or equal to the square root of the array size, 
            // which is the largest number we are trying to find primes in.
            // So we don't have to cross out numbers
            // larger than that square root of the maxnumber, as they will have been crossed out already
            double iterationLimit = Math.Sqrt(crossedOut.Length);
            return (int) iterationLimit;
        }
 
        void crossOutMultiplesOf(bool[] crossedOut, int i)
        {
            for (int multiple = 2*i; multiple < crossedOut.Length; multiple += i)
                crossedOut[multiple] = true;
        }
 
        int[] putUncrossedIntegersIntoResult(bool[] crossedOut)
        {
            int[] result = new int[numberOfUncrossedIntegers(crossedOut)];
            for (int j = 0, i = 2; i < crossedOut.Length; i++)
                if (!crossedOut[i])
                    result[j++] = i;
            return result;
        }
 
        int numberOfUncrossedIntegers(bool[] crossedOut)
        {
            int count = 0;
            for (int i = 2; i < crossedOut.Length; i++)
                if (!crossedOut[i])
                    count++;
            return count;
        }
    }
Comments [0] | | #