Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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);
    }
}
share|improve this question
add comment

2 Answers

Your code looks mostly good to me. My notes:

  1. The recursions you mentioned are entirely appropriate, I think. Especially the one in ToString(). The one in ChangeLastPredecessor() could be simplified using while 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.

  2. 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.

  3. 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.

  4. 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.

  5. 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,.

share|improve this answer
add comment

Here are few comment:

  1. it seems like ChangeLastPredecessor is an implementation of linked list.

  2. instead of returning a new object in every function , return by modifying the same object.

  3. you may use Builder pattern (this is under investigation).

  4. use of extension method can be exploited.

let me know in case of any concern.

share|improve this answer
    
Why would you want to use extension methods here? I don't see any reason to use them in this case. –  svick Dec 19 '12 at 9:17
    
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 –  paritosh Dec 19 '12 at 9:40
1  
But the OP can freely change the class itself, because they wrote it. –  svick Dec 19 '12 at 9:47
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.