I am writing an object in C# to mimic jQuery's various functions. That is, calling the constructor is akin to calling the $()
function of jQuery. For example,
var a = new JQuery("div").eq(0);
Assert.AreEqual("$(\"div\").eq(0)", a.ToString());
I also have an Append function which concatenates two or more existing JQuery objects into one, changing the $ of all but the first into a find. For example,
var a = new JQuery("body").find("ul").eq(0);
var b = new JQuery("li").filter("div").eq(0);
var c = new JQuery("span").filter("a");
var d = JQuery.Append(a, b, c);
Assert.AreEqual("$(\"body\").find(\"ul\").eq(0).find(\"li\").filter(\"div\").eq(0).find(\"span\").filter(\"a\")", d.ToString());
The code is below. There is a bit of recursion in the ChangeLastPredecessor
and in the ToString
methods. How do you suggest I clean up those two methods?
internal struct Parameter
{
private readonly object @object;
internal Parameter(object parameter)
{
@object = parameter;
}
public override string ToString()
{
if (@object == null)
{
return string.Empty;
}
// convert string to quoted and escaped string
var objString = @object as string;
return objString == null
? Convert.ToString(@object, CultureInfo.InvariantCulture)
: string.Concat("\"", objString.Replace("\"", "\\\""), "\"");
}
}
public class JQuery
{
public JQuery(string selector)
{
Function = "$";
Parameters = new List<Parameter> { new Parameter(selector) };
Predecessor = null;
}
public JQuery(JQuery obj)
{
Function = "$";
Parameters = new List<Parameter> { new Parameter(obj) };
Predecessor = null;
}
private JQuery(JQuery predecessor, string function, params object[] parameters)
{
Function = function;
Parameters = parameters.Select(x => new Parameter(x));
Predecessor = predecessor;
}
// the current function in the chain
private string Function { get; set; }
// the parameters to the current function
private IEnumerable<Parameter> Parameters { get; set; }
// the previous function in the chain
private JQuery Predecessor { get; set; }
public static JQuery Append(JQuery appendee, params JQuery[] appendices)
{
if (appendices == null)
{
throw new ArgumentNullException("appendices");
}
if (appendices.Length == 0)
{
return appendee;
}
for (var i = 0; i < appendices.Length - 1; ++i)
{
ChangeLastPredecessor(appendices[i], appendices[i + 1]);
}
ChangeLastPredecessor(appendee, appendices[appendices.Length - 1]);
return appendices[appendices.Length - 1];
}
private static void ChangeLastPredecessor(JQuery appendee, JQuery changee)
{
if (changee.Predecessor == null)
{
changee.Function = "find";
changee.Predecessor = appendee;
return;
}
var predecessor = changee.Predecessor;
ChangeLastPredecessor(appendee, predecessor);
changee.Predecessor = predecessor;
}
public override string ToString()
{
return (Predecessor == null)
? string.Format(CultureInfo.InvariantCulture, "{0}({1})", Function, string.Join(",", Parameters.Select(x => x.ToString())))
: string.Format(CultureInfo.InvariantCulture, "{0}.{1}({2})", Predecessor, Function, string.Join(",", Parameters.Select(x => x.ToString())));
}
public JQuery eq(int index)
{
return new JQuery(this, "eq", index);
}
public JQuery find(string selector)
{
return new JQuery(this, "find", selector);
}
public JQuery find(JQuery obj)
{
return new JQuery(this, "find", obj);
}
public JQuery filter(string selector)
{
return new JQuery(this, "filter", selector);
}
public JQuery filter(JQuery obj)
{
return new JQuery(this, "filter", obj);
}
}
2 Answers 2
Your code looks mostly good to me. My notes:
The recursions you mentioned are entirely appropriate, I think. Especially the one in
ToString()
. The one inChangeLastPredecessor()
could be simplified usingwhile
to:private static void ChangeLastPredecessor(JQuery appendee, JQuery changee) { JQuery current = changee; while (current.Predecessor != null) current = current.Predecessor; current.Predecessor = appendee; current.Function = "find"; }
Probably the biggest problem with recursion is the possibility of stack overflow. But that doesn't seem to be a possibility here, since you're not going to have one query containing hundreds of subqueries.
If you're going to keep recursive
ChangeLastPredecessor()
, then you can remove the last line (changee.Predecessor = predecessor;
). That's because all it does is to set the property to a value it already has.I think that in cases like this, it's useful to use immutable objects. This way, you could reuse one
JQuery
object in multiple queries. Doing this would effectively turn your structure from a normal singly-linked list to functional immutable linked list.In
ToString()
, you shouldn't repeat the part that joins parameters. Instead, you should extract a variable out of that and use that in both branches.You should try to avoid using identifiers that are the same as keywords. I think that in your case, using
obj
instead of@object
wouldn't reduce clarity,.
Here are few comment:
it seems like ChangeLastPredecessor is an implementation of linked list.
instead of returning a new object in every function , return by modifying the same object.
you may use Builder pattern (this is under investigation).
use of extension method can be exploited.
let me know in case of any concern.
-
\$\begingroup\$ Why would you want to use extension methods here? I don't see any reason to use them in this case. \$\endgroup\$svick– svick2012年12月19日 09:17:26 +00:00Commented Dec 19, 2012 at 9:17
-
\$\begingroup\$ extension method should be used when you want to add methods to a class even it is not allowing you to change it, by the help of extension method this library can be more useful \$\endgroup\$Paritosh– Paritosh2012年12月19日 09:40:41 +00:00Commented Dec 19, 2012 at 9:40
-
1\$\begingroup\$ But the OP can freely change the class itself, because they wrote it. \$\endgroup\$svick– svick2012年12月19日 09:47:32 +00:00Commented Dec 19, 2012 at 9:47