A question of style: inline delegates
-
Inspired by Ennis Ray's question below. Which of these do you prefer... and why? A:
protected string FormatText(string text)
{
...
text = Regex.Replace(text, @"\b(?:TTP|TT|Bug)[\s:#]*(?<id>\d+)\b",
new MatchEvaluator(LinkMatchedBug), RegexOptions.IgnoreCase);
...
return text;
}
private string LinkMatchedBug(Match m)
{
return string.Format("<a href='?id={0}'>{1}</a>",
m.Groups["id"].Value, m.Value);
}B:
protected string FormatText(string text)
{
...
text = Regex.Replace(text, @"\b(?:TTP|TT|Bug)[\s:#]*(?<id>\d+)\b",
new MatchEvaluator(delegate(Match m)
{
return string.Format("<a href='?id={0}'>{1}</a>",
m.Groups["id"].Value, m.Value);
}), RegexOptions.IgnoreCase);
...
return text;
}----
It appears that everybody is under the impression that I approve of the documentation. You probably also blame Ken Burns for supporting slavery.
--Raymond Chen on MSDN
-
Inspired by Ennis Ray's question below. Which of these do you prefer... and why? A:
protected string FormatText(string text)
{
...
text = Regex.Replace(text, @"\b(?:TTP|TT|Bug)[\s:#]*(?<id>\d+)\b",
new MatchEvaluator(LinkMatchedBug), RegexOptions.IgnoreCase);
...
return text;
}
private string LinkMatchedBug(Match m)
{
return string.Format("<a href='?id={0}'>{1}</a>",
m.Groups["id"].Value, m.Value);
}B:
protected string FormatText(string text)
{
...
text = Regex.Replace(text, @"\b(?:TTP|TT|Bug)[\s:#]*(?<id>\d+)\b",
new MatchEvaluator(delegate(Match m)
{
return string.Format("<a href='?id={0}'>{1}</a>",
m.Groups["id"].Value, m.Value);
}), RegexOptions.IgnoreCase);
...
return text;
}----
It appears that everybody is under the impression that I approve of the documentation. You probably also blame Ken Burns for supporting slavery.
--Raymond Chen on MSDN
Assuming that I don't need the MatchEvaluator anywhere else, I prefer:
protected string FormatText(string text)
{
...
text = Regex.Replace(text, @"\b(?:TTP|TT|Bug)[\s:#]*(?\d+)\b", (m) =>
string.Format("{1}", m.Groups["id"].Value, m.Value), RegexOptions.IgnoreCase);
...
return text;
}I would probably use anonymous delegate too, but the syntax is a bit ugly and VS supports generating method for delegate, so writing the first version is as easy (or even easier) than the second.
Homepage: TomasP.net | Photo of the month: Calendar | C# and LINQ, F#, Phalanger: My Blog
Latest article: Phalanger, PHP for .NET: Introduction for .NET developers -
Inspired by Ennis Ray's question below. Which of these do you prefer... and why? A:
protected string FormatText(string text)
{
...
text = Regex.Replace(text, @"\b(?:TTP|TT|Bug)[\s:#]*(?<id>\d+)\b",
new MatchEvaluator(LinkMatchedBug), RegexOptions.IgnoreCase);
...
return text;
}
private string LinkMatchedBug(Match m)
{
return string.Format("<a href='?id={0}'>{1}</a>",
m.Groups["id"].Value, m.Value);
}B:
protected string FormatText(string text)
{
...
text = Regex.Replace(text, @"\b(?:TTP|TT|Bug)[\s:#]*(?<id>\d+)\b",
new MatchEvaluator(delegate(Match m)
{
return string.Format("<a href='?id={0}'>{1}</a>",
m.Groups["id"].Value, m.Value);
}), RegexOptions.IgnoreCase);
...
return text;
}----
It appears that everybody is under the impression that I approve of the documentation. You probably also blame Ken Burns for supporting slavery.
--Raymond Chen on MSDN
When its short I prefer B. It does a great job of keeping the code located where it is used. While the above example uses elipses to bring LinkMatchedBug's definition close to its use, real programs will often have a hundred lines or more between them (as part of a big switch or if/elseif block). "A" tends to force the reader to skip back and forth through the code to try to make sense of it. Two caveats: first off, the actual function needs to be decently self explanatory. Addding a comment may suffice, but if you're doing something tricky its usually best to use A, and make sure the function name is very descriptive. Second, if you are going to use a pattern more than once... maybe more than twice... A is vastly prefered over code duplication. In this specific example, I prefer A because "LinkMatchedBug" seems more descriptive than the actual string.Format() comand. I might change my mind if I saw the rest of the code and decided that the Format string spoke for itself, given the context.
-
Assuming that I don't need the MatchEvaluator anywhere else, I prefer:
protected string FormatText(string text)
{
...
text = Regex.Replace(text, @"\b(?:TTP|TT|Bug)[\s:#]*(?\d+)\b", (m) =>
string.Format("{1}", m.Groups["id"].Value, m.Value), RegexOptions.IgnoreCase);
...
return text;
}I would probably use anonymous delegate too, but the syntax is a bit ugly and VS supports generating method for delegate, so writing the first version is as easy (or even easier) than the second.
Homepage: TomasP.net | Photo of the month: Calendar | C# and LINQ, F#, Phalanger: My Blog
Latest article: Phalanger, PHP for .NET: Introduction for .NET developersLambdas only work in C# 3.0 though...I think Shog was probably referring to 2.0 code. In which case, anonymous delegates would be the way to go.
We are certainly uncertain at least I'm pretty sure I am...
-
Lambdas only work in C# 3.0 though...I think Shog was probably referring to 2.0 code. In which case, anonymous delegates would be the way to go.
We are certainly uncertain at least I'm pretty sure I am...
David Stone wrote:
I think Shog was probably referring to 2.0 code.
Correct. But Tomas' solution was still cool to see, and certainly fits in well with the theme of brevity vs. readability (especially since, IMHO, it was both the most concise and most readable).
----
It appears that everybody is under the impression that I approve of the documentation. You probably also blame Ken Burns for supporting slavery.
--Raymond Chen on MSDN
-
When its short I prefer B. It does a great job of keeping the code located where it is used. While the above example uses elipses to bring LinkMatchedBug's definition close to its use, real programs will often have a hundred lines or more between them (as part of a big switch or if/elseif block). "A" tends to force the reader to skip back and forth through the code to try to make sense of it. Two caveats: first off, the actual function needs to be decently self explanatory. Addding a comment may suffice, but if you're doing something tricky its usually best to use A, and make sure the function name is very descriptive. Second, if you are going to use a pattern more than once... maybe more than twice... A is vastly prefered over code duplication. In this specific example, I prefer A because "LinkMatchedBug" seems more descriptive than the actual string.Format() comand. I might change my mind if I saw the rest of the code and decided that the Format string spoke for itself, given the context.
cmdrrickhunter wrote:
I might change my mind if I saw the rest of the code and decided that the Format string spoke for itself, given the context.
This was ripped out of, and trimmed down from, a big wad of regexps and replacement loops that are tasked with turning five years worth of free-form text bug reports into something navigable and half-way presentable. I've half a mind to just make it look as ugly as possible, to discourage anyone from ever touching it... but i won't, 'cause i'm just such an optimist (and it wouldn't work anyway). Of course, LinkMatchedBug is about five times longer than it appears - if it was really as simple as presented, i could have stopped with a replacement pattern rather than resorting to a MatchEvaluator. Still, i think the example is sound, as it's very much a one-off task - and i'm more than a little leery about separating a routine that depends on named groups from the regexp that names them...
----
It appears that everybody is under the impression that I approve of the documentation. You probably also blame Ken Burns for supporting slavery.
--Raymond Chen on MSDN
-
Lambdas only work in C# 3.0 though...I think Shog was probably referring to 2.0 code. In which case, anonymous delegates would be the way to go.
We are certainly uncertain at least I'm pretty sure I am...
Yes, I knew he was :-), but lambdas in C# 3 are much nicer when compared to anonymous delegates so I had to answer in C# 3.
Homepage: TomasP.net | Photo of the month: Calendar | C# and LINQ, F#, Phalanger: My Blog
Latest article: Phalanger, PHP for .NET: Introduction for .NET developers -
Inspired by Ennis Ray's question below. Which of these do you prefer... and why? A:
protected string FormatText(string text)
{
...
text = Regex.Replace(text, @"\b(?:TTP|TT|Bug)[\s:#]*(?<id>\d+)\b",
new MatchEvaluator(LinkMatchedBug), RegexOptions.IgnoreCase);
...
return text;
}
private string LinkMatchedBug(Match m)
{
return string.Format("<a href='?id={0}'>{1}</a>",
m.Groups["id"].Value, m.Value);
}B:
protected string FormatText(string text)
{
...
text = Regex.Replace(text, @"\b(?:TTP|TT|Bug)[\s:#]*(?<id>\d+)\b",
new MatchEvaluator(delegate(Match m)
{
return string.Format("<a href='?id={0}'>{1}</a>",
m.Groups["id"].Value, m.Value);
}), RegexOptions.IgnoreCase);
...
return text;
}----
It appears that everybody is under the impression that I approve of the documentation. You probably also blame Ken Burns for supporting slavery.
--Raymond Chen on MSDN
A. I detest code that seeks to do as much as it can in as few lines as possible. The time taken by a developer 6 months down the track to work out what's going on is worth more than the filespace saved. Keep it simple, keep it neat, keep it documented. Most of all make sure it's easy to debug.
cheers, Chris Maunder
CodeProject.com : C++ MVP
-
Yes, I knew he was :-), but lambdas in C# 3 are much nicer when compared to anonymous delegates so I had to answer in C# 3.
Homepage: TomasP.net | Photo of the month: Calendar | C# and LINQ, F#, Phalanger: My Blog
Latest article: Phalanger, PHP for .NET: Introduction for .NET developersOh. I agree. I :love: lambdas. :cool:
We are certainly uncertain at least I'm pretty sure I am...
-
A. I detest code that seeks to do as much as it can in as few lines as possible. The time taken by a developer 6 months down the track to work out what's going on is worth more than the filespace saved. Keep it simple, keep it neat, keep it documented. Most of all make sure it's easy to debug.
cheers, Chris Maunder
CodeProject.com : C++ MVP
As Kent Beck puts is, the explicit code is intention revealing., To Be Explicit, Martin Folwer ... an oldy but a goody.
Regards Ray "Je Suis Mort De Rire" Blogging @ Keratoconus Watch
-
A. I detest code that seeks to do as much as it can in as few lines as possible. The time taken by a developer 6 months down the track to work out what's going on is worth more than the filespace saved. Keep it simple, keep it neat, keep it documented. Most of all make sure it's easy to debug.
cheers, Chris Maunder
CodeProject.com : C++ MVP
Chris Maunder wrote:
The time taken by a developer 6 months down the track to work out what's going on is worth more than the filespace saved.
You don't think separating the regexp from the code that consumes its results will cost you?
----
It appears that everybody is under the impression that I approve of the documentation. You probably also blame Ken Burns for supporting slavery.
--Raymond Chen on MSDN
-
Chris Maunder wrote:
The time taken by a developer 6 months down the track to work out what's going on is worth more than the filespace saved.
You don't think separating the regexp from the code that consumes its results will cost you?
----
It appears that everybody is under the impression that I approve of the documentation. You probably also blame Ken Burns for supporting slavery.
--Raymond Chen on MSDN
You mean in a code-execution-time type situation? If testing showed it did, and if this was something where execution time saved was important then I'd go with whatever was fastest but just make sure it was commented to within an inch of its life.
cheers, Chris Maunder
CodeProject.com : C++ MVP
-
You mean in a code-execution-time type situation? If testing showed it did, and if this was something where execution time saved was important then I'd go with whatever was fastest but just make sure it was commented to within an inch of its life.
cheers, Chris Maunder
CodeProject.com : C++ MVP
Chris Maunder wrote:
You mean in a code-execution-time type situation?
No. I mean in terms of, i'm reading through this method start to finish and having to jump to another method to find out what happens is annoying in that "reading a choose-your-own-adventure book and running out of fingers" way. Personally, i could take it either way (which is why i started this thread...): moving the replacement into a separate method gives me a chance to name it and greatly reduces clutter, allowing me to scan through the method faster. Leaving it inline reduces the temptation to throw in a comment duplicating information already in code (
// create an <a> element with href pointing to URL derived from item Id), and avoids the problem of having to flip back and forth between two methods when looking for the source of a match (the choose-your-own-adventure thing... i hate that). In the end, i moved the whole replacement call into a separate method, with inline delegate - it's still messy looking, but the operation is kept logically and textually atomic, so i can read through at a higher level (LinkBugs(); LinkURLs(); LinkEmails();...) without ever worrying about the madness that lies beneath (and read through at a lower-level without ever having to lose context). This works well for me, and since i don't expect anyone else to ever read it that's enough... but i'm always curious about how others view such things.----
It appears that everybody is under the impression that I approve of the documentation. You probably also blame Ken Burns for supporting slavery.
--Raymond Chen on MSDN