Is this a good approach to dealing with tagging?

Question

Can this code be optimized or re-factored? Is this an optimal approach to tagging?

The following code is a callback in my posts model. It creates a record that associates a tag with a post in a QuestionsTags joiner table. When necessary, if a given tag does not already exist in the tags table, the function creates it, then uses its id to create the new record in the QuestionsTags table.

The difficulty with this approach is the QuestionsTags table depends on data in the tags table which may or may not exist.

The function assumes the following tables:

tags(id, tagName),
posts(tags) // Comma delimited list
questionsTags(postId, tagId)

The idea is to loop over a delimited list of tags submitted with a post and check to see if each tag already exists in the tags table

If tag exists:

Check to see if there's already a QuestionTag record for this post and this tag in the QuestionTags table.
If yes, do nothing (the association already exists)
If no, create a new QuestionTag record using the id of the existing tag and the postId

If tag does not already exist:

Create the new tag in the tags table
Use its id to create a new QuestionsTags record

Code

/**
* @hint Sets tags for a given question.
**/
private function setTags()
{
    // Loop over the comma and space delmited list of tags
    for (local.i = 1; local.i LTE ListLen(this.tags, ", "); local.i = (local.i + 1))
    {
        // Check if the tag already exists in the database
        local.tag = model("tag").findOneByTagName(local.i);
        // If the tag exists, look for an existing association between the tag and the question in the QuestionTag table
        if (IsObject(local.tag))
        {
            local.questionTag = model("questionTag").findOneByPostIdAndTagId(values="#this.postId#,#local.tag.id#");
            // If no assciatione exists, create a new QuestionTag record using the tagId and the postId
            if (! IsObject(local.questionTag))
            {
                local.newQuestionTag = model("questionTag").new(postId = this.id, tagId = local.tag.id);
                // Abort if the saving the new QuestionTag is unsuccesful
                if (! local.newQuestionTag.save())
                {
                    return false;
                }
            }
        }
        // If the tag does not exist create it
        else
        {
            local.newTag = model("tag").new(tagName = local.i, userId = this.ownerUserId);
            // Abort if the the new tag is not saved successfully
            if (! local.newTag.save())
            {
                return false;
            }

            // Otherwise create a new association in the QuestionTags table using the id of the newly created tag and the postId
            local.newQuestionTag = model("questionTag").new(postId = this.id, tagId = local.newTag.id);
            // Abort if the new QuestionTag does not save correctly
            if (! local.newQuestionTag.save())
            {
                return false;
            }
        }
    }
}

FYI: I'm using CFWheels in my application, which explains the ORM functions used.

score 2 · Accepted Answer · answered Jan 10 '11 at 15:48

2

That's pretty much how I would approach it. Curious why you're using ", " as the delimiter? What if the "user,didn't,leave,a,space"? I'd just use comma and trim() the list element.

answered Jan 10 '11 at 15:48

Todd Sharp

3,207
2
19
27

1

Also, you can use local.i++ in your for() loop. – Todd Sharp Jan 10 '11 at 15:49
Oh, and why are you touching the 'this' scope instead of passing arguments to the function and referencing 'arguments.tagList' or some such? Use of the 'this' scope is kinda frowned upon from a best practice standpoint. – Todd Sharp Jan 10 '11 at 15:51
1

I'm also confused by your referencing 'local.i' as the tag item in the list. Wouldn't you need to do listGetAt(thelist, i) to get the actual tag? local.i is just the iterator in this example, no? – Todd Sharp Jan 10 '11 at 15:53
1

The only other thing (I think) I'll mention is that you might consider first wiping out all existing associations before processing. I may have tagged something before but now I'm changing the tags so you don't want to leave any old associations on the object (or do you?). – Todd Sharp Jan 10 '11 at 15:54
@Todd, very good comments! The "this" scope is how CFWheels references object keys inside models--I don't know why, TBH; can you elaborate on why "this" is frowned upon? You're correct about the ListGetAt(); fixed. Is local.i++ a readability thing, or does it have other benefits? I intend to wipe existing associations by using a scheduled event. – Mohamad Jan 10 '11 at 16:05
1

Here is some good reading on the 'this' scope: http://www.briankotek.com/blog/index.cfm/2007/2/6/VarScoping-Private-and-Public-Data-in-CFCs and http://www.dougboude.com/blog/1/2007/10/Appropriate-Usage-of-the-THIS-Scope.cfm – Todd Sharp Jan 10 '11 at 16:16
local.i++ is just shorthand and is what is commonly used in other languages so I tend to stick to using it in cfscript too. – Todd Sharp Jan 10 '11 at 16:21
@Todd one more time: To answer your main post, I'm leaving the space there in case validation fails, I don't want the string to come back without spaces! My understanding is ", " treats comma and space as separate delimiters. – Mohamad Jan 10 '11 at 16:22
@Mel: Nope, that's unfortunately untrue. http://cfsilence.com/blog/client/index.cfm/2010/11/17/Using-Multi-Character-Delimiters-On-A-List – Todd Sharp Jan 10 '11 at 16:41
Wait - nevermind - I misunderstood. Yes, that would work for you. – Todd Sharp Jan 10 '11 at 16:52

Is this a good approach to dealing with tagging?

1 Answers1