Improving readability and logic on "re-indexing" method

Question

This is a method which takes an input of a "page" object, which includes SortOrder. It then fetches the existing page items from a database and attempts to re-index.

I don't think this is at all efficient and reads horribly and wondered if anybody could make some recommendations?

    public Boolean ReIndexOutputTemplatePages(OutputTemplatePage page)
    {
        try
        {

            int? requiredIndex = page.SortOrder;

            var items = from t1 in db.OutputTemplatePages
                        where (t1.OutputTemplateId == page.OutputTemplateId)
                        orderby t1.SortOrder
                        select t1;

            Boolean boolHasBeenAdded = false;
            int ctr = 0;
            if (items.Count() == 0)
            {
                Add(page);
                boolHasBeenAdded = true;
            }
            else
            {
                foreach (OutputTemplatePage template in items)
                {
                    if (requiredIndex == ctr)
                    {
                        Add(page);
                        ctr++;
                        boolHasBeenAdded = true;
                    }

                    OutputTemplateRepository repo = new OutputTemplateRepository();
                    OutputTemplatePage pg = repo.GetOutputTemplatePageById(template.Id);
                    pg.SortOrder = ctr;
                    repo.Save();

                    ctr++;

                }
            }
            if (!boolHasBeenAdded)
            {
                Add(page);
            }
            return true;
        }
        catch (Exception)
        {
            return false;
        }
    }

Answer 1

I have the following comments and recommendations:

• The try/catch is too general: if anything goes wrong anywhere, you return false. This may mask errors. Catch more specific exceptions and narrow the scope of your try/catch to where you are okay with things going wrong. (If you don't EXPECT anything to ever go wrong in certain parts of the code, you probably want to "fail fast" and catch that exception at a higher level. This could hide lots of kinds of errors during dev/test.)
• Name mismatch: requiredIndex does not match .SortOrder (I'm not saying they need to be same exact name, but these seem to describe different concepts to me)
• Does .SortOrder need to be optional? Consider enforcing that it be set by changing the orginal OutputTemplatePage class to require it. Is it an enum? If so, perhaps the default value could be Default (as in "use the default sort order")
• Name choice: Boolean boolHasBeenAdded might be better as Boolean pageHasBeenAdded (prefix notation has been out of vogue for years - since .NET, certainly). Also, I usually prefer language types (bool) to the .NET types, but that's just style/preference.

• This block could go away if you invert the next conditional (since this block happens anyway at the end):

      if (items.Count() == 0)
      {
          Add(page);
          boolHasBeenAdded = true;
      }
      else
  

... to ...

        if (items.Count() != 0)

• Consider moving the following code into a well-named helper method to make its intent more apparent:

              OutputTemplateRepository repo = new OutputTemplateRepository();
              OutputTemplatePage pg = repo.GetOutputTemplatePageById(template.Id);
              pg.SortOrder = ctr;
              repo.Save();
  

• It is unclear to me what repo.Save() does - seems like it might have side-effects. All the more reason for prior suggestion.

• Rename ctr to something more descriptive. Is it the sort index? Of course, setting SortOrder = ctr makes me wonder if SortOrder is named correctly. "Sort order" makes me thing of high level approach to sorting - like ascending vs. descending? Not clear. Makes it hard to grok.

[Edit] Actually, this largest problem might be that this code is not in the right class which might be the source of some awkwardness. Consider making ReIndexOutputTemplatePages a method on OutputTemplateRepository to clean up the code leading up to rep.Save.

Hope that helps!

Answer 2

Here's a little bit of how I'd reorganize. The key elements which could affect speed are: creating the repository only once, using the LINQ extension Any which has the possibility of optimizing the SQL generated and early return if there are no entries in the database.

    public bool ReIndexOutputTemplatePages(OutputTemplatePage page)
    {
        try
        {
            var items = from t1 in db.OutputTemplatePages
                where t1.OutputTemplateId == page.OutputTemplateId
                orderby t1.SortOrder
                select t1;

            if (!items.Any())
            {
                Add(page);
                return true;
            }

            var repo = new OutputTemplateRepository();
            var hasBeenAdded = false;
            var ctr = 0;

            foreach (var template in items)
            {
                if (page.SortOrder == ctr)
                {
                    Add(page);
                    hasBeenAdded = true;
                    ctr++;
                }

                var pg = repo.GetOutputTemplatePageById(template.Id);

                pg.SortOrder = ctr;
                ctr++;
                repo.Save();
            }

            if (!hasBeenAdded)
            {
                Add(page);
            }

            return true;
        }
        catch (Exception)
        {
            return false;
        }
    }

Answer 3

Here's my attempt to clean it up by LINQifying it. I think that I preserved all the logic but its hard to know for sure (I cannot emphasize how important it is to write tests).

 public void ReIndexOutputTemplatePages(OutputTemplatePage page) {
            //I'm on a personal vendetta to stamp out the sql-style of syntax, it makes thing way more confusing than need be
            var items = db.OutputTemplatePages
                          .Where(t => t.OutputTemplateId == page.OutputTemplateId)
                          .OrderBy(t=> t.SortOrder)
                          .ToArray(); //might as well load it all up, you're using each one it anyways

            OutputTemplateRepository repo = new OutputTemplateRepository();

            if(items.Length== 0 || (page.SortOrder != null && page.SortOrder > 0 && page.SortOrder < items.Length)) 
                Add(page);

            Enumerable.Range(items.Length)
            .Zip(items, (i, item)=> new {
                SortOrder = i,
                // This is a select n+1 bug waiting to happen and only ok if you are sure that items will always have a low count 
                // otherwise create a function repo.getTemplatePages(items.Select(p=>p.Id)) that is appropriately optimized
                // and get all of these in one shot
                // Also, since you're not using item directly, maybe you just want to fetch them out of 
                // the db with a join rather than having the intermediate step
                    Template = repo.GetOutputTemplatePageById(item.Id)
            }).ToList() // I usually write an ForEach(this IEnumerable<> extension so that you don't have to do this
           .ForEach(x=>x.Template.SortOrder = x.SortOrder);

            repo.Save();
}

As the others have said, don't try...catch unless you know exactly what exception you're looking for. You should only catch exceptions you can do something about, something like an OutOfMemoryException should be allowed to go right through as there are better ways to handle them.

I would also say it's a bit wierd to be newing objects up like this in a method. Newing an object up like this is theoretically no different from calling a static method and practically even a little worse. My understanding of proper OOP (and there is plenty of room for argument on this) is that dependencies should be passed in via the constructor or at least created as fields on the object.