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've created an algorithm which weighs the relevance of a list of articles against two lists of keywords that correlate to attributes of the article.

It works great and is super efficient... but it's a mess. It's not terribly readable, so it's difficult to discern what's going.

The operation in pseudo code goes something like this:

  • Loop through every article in a list called articles(List<Article>)
  • For every article, loop through every role in a list of roles (List<string>)
  • Check to see if the current article has any roles (Article.Roles = List<string>)
  • If yes, then loop through each role in the article and try to match a role in the article to the role in the current loop
  • If a match is found, add weight to the article. If the index of the role on the article and the role in the roles list are both index 0 (in primary position) add extra weight for two matching primaries
  • Repeat for topics, but with no bonus for primary matches

What would be a better way to write the following code? I can't use foreach except in one or two places, because I need to match indexes to know what value to add on a match.

private static List<Article> WeighArticles(List<Article> articles, List<string> roles, List<string> topics, List<string> industries)
{
    var returnList = new List<Article>();
    for (int currentArticle = 0; currentArticle < articles.Count; currentArticle++)
    {
        for (int currentRole = 0; currentRole < roles.Count; currentRole++)
        {
            if (articles[currentArticle].Roles != null && articles[currentArticle].Roles.Count > 0)
            {
                for (int currentArticleRole = 0; currentArticleRole < articles[currentArticle].Roles.Count; currentArticleRole++)
                {
                    if (articles[currentArticle].Roles[currentArticleRole].ToLower() == roles[currentRole].ToLower())
                    {
                        if (currentArticleRole == 0 && currentRole == 0)
                            articles[currentArticle].Weight += 3;
                        else
                            articles[currentArticle].Weight += 1;
                    }
                }
            }
        }
        for (int currentTopic = 0; currentTopic < topics.Count; currentTopic++)
        {
            if (articles[currentArticle].Topics != null && articles[currentArticle].Topics.Count > 0)
            {
                for (int currentArticleTopic = 0; currentArticleTopic < articles[currentArticle].Topics.Count; currentArticleTopic++)
                {
                    if (articles[currentArticle].Topics[currentArticleTopic].ToLower() == topics[currentTopic].ToLower())
                    {
                        articles[currentArticle].Weight += 0.8;
                    }
                }
            }
        }
        returnList.Add(articles[currentArticle]);
    }

    return returnList;
}

//Article Class stub (unused properties left out)
public class Article
{
    public List<string> Roles { get; set; }
    public List<string> Topics { get; set; }
    public double Weight { get; set; }
}
share|improve this question
1  
I know this is preference, but it really would make a big difference in readability to use the brace style where there is no new line before the first one (e.g. while(true) {\n}, where \n is a new line). –  Paul Draper Aug 23 at 5:41

3 Answers 3

up vote 6 down vote accepted

Note: last refactoring with rich domain model approach is at the bottom. But you can see my 3 refactoring steps which lead to nice object-oriented design.

You can use Linq to make your code more readable. Also you can give names for intermediate results:

var articlesWithRoles = from a in articles
                        where a.Roles != null && a.Roles.Any()
                        select a;

var primaryRole = roles.First().ToLower();

var highWeightArticles = from a in articlesWithRoles
                         where a.Roles.First().ToLower() == primaryRole
                         select a;

foreach(var article in highWeightArticles)
    article.Weight += 3;

var lowWeightArticles = from a in articlesWithRoles.Except(highWeightArticles)
                        from ar in a.Roles
                        join r in roles on ar.ToLower() equals r.ToLower()
                        select a;

foreach(var article in lowWeightArticles)
    article.Weight += 1;

var articlesWithTopics = from a in articles
                         where a.Topics != null
                         from at in a.Topics
                         join t in topics on at.ToLower() equals t.ToLower()
                         select a;

foreach(var article in articlesWithTopics)
    article.Weight += 0.8;

I also would recommend to use named constants instead of magic numbers 3, 1, 0.8. Also maybe you don't need to create new list, because it will share same articles as list yo passed to method.


Further improving readability could involve splitting this method into three methods:

 AddRolesWeights(articles, roles);
 AddPrimaryRoleWeights(articles, roles.First());
 AddTopicsWeights(articles, topics);

Which could look this way:

void AddRolesWeights(IEnumerable<Article> articles, IEnumerable<Role> roles)
{
    const double RoleWeight = 1;

    var nonPrimaryArticles = 
        from a in articles
        where a.Roles != null && a.Roles.Any() &&
        from ar in a.Roles
        join r in roles on ar.ToLower() equals r.ToLower()
        select a;

    foreach(var article in primaryArticles)
        article.Weight += RoleWeight;
}

Yes, its less efficient. But you always should chose between readability and performance:

void AddPrimaryRoleWeights(IEnumerable<Article> articles, string primaryRole)
{
    const double PrimaryArticleWeight = 2;
    var primaryArticles = 
       from a in articlesWithRoles
       where a.Roles != null && a.Roles.Any() &&
           String.Equals(a.Roles[0], primaryRole, StringComparison.CurrentCultureIgnoreCase)
       select a;

    foreach(var article in primaryArticles)
        article.Weight +=  PrimaryArticleWeight;
}

Same way you can update articles with selected topics.


Final solution

Further refactoring can include moving weights calculation logic to article class, thus making it reach instead of anemic, and increasing cohesion:

 // add this to Article class
 public double Weight { get; private set; } // probably you don't need setter

 public void AddWeights(IEnumerable<Role> roles)
 {
     const double RoleWeight = 1;
     const double PrimaryRoleWeight = 3;

     if (!roles.Any())
        return;

     if (Roles == null || !Roles.Any())
         return;

     var pirmaryRole = roles.First();
     if (String.Equals(Roles[0], primaryRole, StringComparison.CurrentCultureIgnoreCase))
     {
         Weight += PrimaryRoleWeight;
         return;
     }

     foreach(var role in roles)         
        if (Roles.Contains(role, StringComparer.CurrentCultureIgnoreCase))
            Weight += RoleWeight;
 }

Adding topics weights:

 public void AddWeights(IEnumerable<Topic> topics)
 {
     const double TopicWeight = 0.8;

     if (Topics == null || !Topics.Any() || !topics.Any())
        return;

     foreach(var topic in topics)         
        if (Topics.Contains(topic, StringComparer.CurrentCultureIgnoreCase))
            Weight += TopicWeight;
 }

And your original code will look like:

 foreach(var article in articles)
 {
     article.AddWeights(roles);
     article.AddWeights(topics);
 }

I think it's a little more readable now :)

share|improve this answer
    
I may be reading this wrong, but it's the ROLE in first position ON the article that is high weight, not the article itself. Each article (may have) many roles, and each role match needs to be given a score. It looks to me like you are creating two separate lists of article, one high weight and one low weight, and only weighing the first match on the high weight articles. Is that correct? –  Wesley Aug 22 at 22:12
    
I'd probably lose the .ToLower()s (on mostImportantRole and highWeightArticles declarations/assignments) and replace the comparison with string.Equals(firstString, secondString, StringComparison.CurrentCultureIgnoreCase) –  Jesse C. Slicer Aug 22 at 22:13
    
@Wesley a.Roles.First().ToLower() is a first role on the article, not article itself –  Sergey Berezovskiy Aug 22 at 22:14
    
Correct, but you have highWeightArticles and lowWeightArticles as two separate lists of article. highWeightArticles are each getting +3, but are then Except() from lowWeightArticles, meaning it looks like they are left out of any action other than their initial +3. Testing this right now with a large sample of articles and roles. –  Wesley Aug 22 at 22:16
    
@Wesley sorry, was in rush or further refactorings :) I suggest you to test last, object-oriented approach. It also should have better performance –  Sergey Berezovskiy Aug 22 at 22:57

Yes, I think there may be improvements.

  1. You refer to articles[currentArticle] alot in the code. You should be able to remove this by replacing your first loop with a foreach(var article in articles).
  2. The if statements such as articles[currentArticle].Roles != null && articles[currentArticle].Roles.Count > 0 refer only in the context of your outermost loop and never change in the inner loops. Hence you are needlessly checking this condition on each inner iteration. Consider moving one layer up to avoid additional checks.
  3. I think you can get rid of the first index check in your article loop and hence improve things slightly by only doing an increment on each condition.
  4. I would consider using constants for the weighting factors. At the very least at a method level, if not class level.

I can't guarantee but this code should be slightly faster and a bit more readable. So a first crack at a refactored solution might look like.

private static List<Article> WeighArticles(List<Article> articles, List<string> roles, List<string> topics, List<string> industries)
{
    var returnList = new List<Article>();
    const int RoleWeighting = 3;
    const double TopicWeighting = 0.8;

    foreach(var article in articles)
    {
        if (article.Roles != null && article.Roles.Any())
        {
            if(roles.Any()) {
                // So roles and articles have at least 1 element so we can assume a + weight of at least 3
                article.Weight += RoleWeighting;                    
            }

            // Skipping the first element as that was covered in our +3 above
            for (var currentRole = 1; currentRole < roles.Count; currentRole++)
            {
                for (var currentArticleRole = 1; currentArticleRole < article.Roles.Count; currentArticleRole++)
                {
                    if (article.Roles[currentArticleRole].ToLower() == roles[currentRole].ToLower())
                    {
                            article.Weight ++;
                    }
                }
            }
        }

        if (article.Topics != null && article.Topics.Any())
        {
            for (int currentTopic = 0; currentTopic < topics.Count; currentTopic++)
            {
                for (int currentArticleTopic = 0; currentArticleTopic < article.Topics.Count; currentArticleTopic++)
                {
                    if (article.Topics[currentArticleTopic].ToLower() == topics[currentTopic].ToLower())
                    {
                            article.Weight += TopicWeighting;
                    }
                }
            }
        }

        returnList.Add(article);
    }

    return returnList;
}

Now, I'm sure those statements could be be refactored via linq to only a single line. Will think about that some more.

share|improve this answer

I would like to rewrite the statemnet as below. This will make you code more compact.

var processedArticleRoleWeight = articles.SelectMany(article => article
            .Roles.Where(articleRoles =>
                roles.Exists(elem => elem.ToLower().Equals(articleRoles.ToLower()))),
            (article, articleRoles) => article).Select((elem, idx) =>
            {
                elem.Weight += idx == 0 ? 3 : 1;
                return elem;
            });

var processedArticleTopicWeight = articles.SelectMany(article => article
           .Roles.Where(articleRoles =>
               topics.Exists(elem => elem.ToLower().Equals(articleRoles.ToLower()))),
           (article, articleRoles) => article).Select((elem, idx) =>
           {
               elem.Weight += 0.8;
               return elem;
           });
share|improve this answer

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.