1
\$\begingroup\$

Edit

The rest of my code utilises the Command/Query seperation pattern. I'll probably move this out into a command and a query but for now I've written this directly in my MVC controller for brevity.

Original

I'm quite new to Fluent NHibernate and its various workings. This code is designed to receive an array of tag names, get those tags from the database and then create any tags that were in the array but not already in the database.

Here's the code I've just written:

private IEnumerable<Tag> GetOrCreateTags(IEnumerable<string> tagNames)
        {

            var tagCriteria = _nHibernateSession.CreateCriteria<Tag>();

            var or = Restrictions.Disjunction();

            foreach (var tagName in tagNames)
            {
                or.Add(Restrictions.Eq("Name", tagName));
            }
            tagCriteria.Add(or);

            var tagsFromTagNames = tagCriteria.List<Tag>();

            foreach (var newTagNeverSeen in tagNames.Except(tagsFromTagNames
                                                           .Select(x => x.Name)))
            {
                using (var transaction = _nHibernateSession.BeginTransaction())
                {
                    var newTag = new Tag
                                     {
                                         Name = newTagNeverSeen,
                                         Enabled = true,
                                         DateCreated = DateTime.Now
                                     };
                    _nHibernateSession.SaveOrUpdate(newTag);
                    tagsFromTagNames.Add(newTag);
                    transaction.Commit();
                }
            }
            return tagsFromTagNames;
        }

How would you improve this? I feel like I'm doing too much here already so I'll likely refactor this to split it out.

\$\endgroup\$
2
\$\begingroup\$

Admittedly I am using a really old version of nhibernate, but I think this is still there (to replace a few lines):

Restrictions.InG("Name", tagNames.ToList())

I think I would also bring the using statement outside of the loop:

private IEnumerable<Tag> GetOrCreateTags(IEnumerable<string> tagNames)
    {
        var tagList = tagNames.ToList(); //or make the parameter above an ICollection<string>

        var tagCriteria = _nHibernateSession.CreateCriteria<Tag>();
        tagCriteria.Add(Restrictions.InG("Name", tagList));

        var tagsFromTagNames = tagCriteria.List<Tag>();

        //naming this for readability
        var existingTagNames = tagsFromTagNames.Select(x => x.Name);

        var newTags = tagList
            .Except(existingTagNames)
            .Select(t => new Tag
                           {
                               Name = newTagNeverSeen,
                               Enabled = true,
                               DateCreated = DateTime.Now
                           }); //moving this outside the loop makes the loop cleaner

        if (newTags.Any())
        {
            using (var transaction = _nHibernateSession.BeginTransaction())
            {
                foreach (var tag in newTags)
                {
                    _nHibernateSession.SaveOrUpdate(tag);
                    tagsFromTagNames.Add(tag);
                }
                transaction.Commit();
            }
        }
        return tagsFromTagNames;
    }
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Thanks Bill, just what I was looking for. The InG method works a treat and moving the using statement out of the loop seems obvious now you mention it. Cheers again. \$\endgroup\$ – Jamie Dixon May 30 '12 at 17:06
  • \$\begingroup\$ The other parts are really helpfull too. The code is much more readable now. To note, the InG restriction can take the IEnumerable<string> tagNames as an input so no need for the conversion. \$\endgroup\$ – Jamie Dixon May 30 '12 at 17:11
  • \$\begingroup\$ Good to know, my version doesn't take IEnumerable. Yet another reason to upgrade. \$\endgroup\$ – Bill Barry May 30 '12 at 18:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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