Today had a pair programming session with a fellow developer in my company.

He made a function like this:

private string _CreateHtmlTableRowForField(string fieldName, string fieldValue) {

string output;

output += “
“;
output += “ “;
output += fieldName;
output += “

“;
output += “ “;
output += fieldValue;
output += “

“;
output += “

“;

return output;
}

Would you refactor this? Idea is that the structure of this HTML will probably never change (it’s a simple implementation of something for a small business problem).

So previously we had a function like this:

protected void FinalizeImportButton_Click(object sender, EventArgs e) {

if (_IsNotImportStatus(ResponseImport.ImportStatusId, ResponseImportStatusEnum.Imported)) return;

ResponseImportServiceExtended responseImportService = new ResponseImportServiceExtended();
responseImportService.ChangeImportStatus(ImportId, (long)ResponseImportStatusEnum.Finalized);

Navigation.RedirectToUrl(SiteMap.ResponseImports.ImportsList, false);

}

Maybe it could be written a little bit clearer? Today when I see what I wrote, I definetly think so. Let’s see:

protected void FinalizeImportButton_Click(object sender, EventArgs e) {

if (!_IsImportInImportedStatus(ResponseImport)) return;

ServiceLocator<ResponseImportServiceExtended>.ChangeStatusToFinalized(ResponseImport);

Navigation.RedirectToUrl(SiteMap.ResponseImports.ImportsList);

}

What do you think? Would you understand this a little bit easier and quicker?

Ofcourse this approach requires:
a) More customized functions
b) Service Locator

Which in return requires more complicated code, but for the sake of clarity and maintainability (e.g. Instantiating service would be done all in one place…) this would be a smarter approach I think.

Cheers!

Just caught myself (code which I wrote) reading something that is not clear as I though when I’ve written it a few months back:

protected void FinalizeImportButton_Click(object sender, EventArgs e) {

if (_IsNotImportStatus(ResponseImport.ImportStatusId, ResponseImportStatusEnum.Imported)) return;

ResponseImportServiceExtended responseImportService = new ResponseImportServiceExtended();
responseImportService.ChangeImportStatus(ImportId, (long)ResponseImportStatusEnum.Finalized);

Navigation.RedirectToUrl(SiteMap.ResponseImports.ImportsList, false);

}

While I didn’t think of of it when I written it… today I see it’s not as clear as I thought it would be. What do you think? What does this code do?

I’ll write a followup post tommorow explaining how I would write it today