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 cannot edit the QueryPermissions class because it is an external .dll that was given to me to use. Yes, I realize it's a bad practice to use an out variables, but I am pushing for a rewrite of that service.

As you can see below the two methods do the almost the EXACT same thing only one uses a groupkey and the other uses a userkey and then calls the appropriate method. For whatever reason I am drawing a blank on refactoring these methods.

public IEnumerable<Node> GetNodesForUser(int userKey, IEnumerable<OnlineReport> onlineReports)
{
    var nodes = new List<Node>();
    foreach (var onlineReport in onlineReports)
    {
        var parentNode = new Node {Text = onlineReport.Key, Value = onlineReport.Value};
        var webQueries = new List<WebQuery>();
        //Line that is different
        _QueryPermissions.GetQueriesForTaskAndUser(Int32.Parse(onlineReport.Value), userKey, ref webQueries, true);
        foreach (var childNode in webQueries.Select(webQuery => new Node(parentNode){Text = webQuery.QueryTitle, Value = webQuery.QueryKey.ToString()}))
        {
            parentNode.Children.Add(childNode);
        }
        nodes.Add(parentNode);
    }
    return nodes;
}

public IEnumerable<Node> GetNodesForGroup(int groupKey, IEnumerable<OnlineReport> onlineReports)
{
    var nodes = new List<Node>();
    foreach (var onlineReport in onlineReports)
    {
        var parentNode = new Node { Text = onlineReport.Key, Value = onlineReport.Value };
        var webQueries = new List<WebQuery>();
        //Line that is different
         _QueryPermissions.GetQueriesForTaskAndUserGroup(Int32.Parse(onlineReport.Value), groupKey, ref webQueries, true);
        foreach (var childNode in webQueries.Select(webQuery => new Node(parentNode) { Text = webQuery.QueryTitle, Value = webQuery.QueryKey.ToString() }))
        {
            parentNode.Children.Add(childNode);
        }
        nodes.Add(parentNode);
    }
    return nodes;
}
share|improve this question
add comment

2 Answers 2

up vote 5 down vote accepted

The big difference between the functions is not the userKey vs. groupKey but rather than they have different queries embedded in them

_QueryPermissions.GetQueriesForTaskAndUser(Int32.Parse(onlineReport.Value), userKey, ref webQueries, true);

vs.

_QueryPermissions.GetQueriesForTaskAndUserGroup(Int32.Parse(onlineReport.Value), groupKey, ref webQueries, true);

It looks like these can be abstracted as

 delegate void NodeQueryDelegate(int reportValue, int key, ref List<WebQuery> webQueries, bool someBool);

so it should be possible to refactor thus

public IEnumerable<Node> GetNodesFromQuery(int key, IEnumerable<OnlineReport> onlineReports, NodeQueryDelegate query) {
    var nodes = new List<Node>();
    foreach (var onlineReport in onlineReports)  {
        var parentNode = new Node { Text = onlineReport.Key, Value = onlineReport.Value };
        var webQueries = new List<WebQuery>();
        //Line that is different
        query(Int32.Parse(onlineReport.Value), key, ref webQueries, true);
        foreach (var childNode in webQueries.Select(webQuery => new Node(parentNode) { Text = webQuery.QueryTitle, Value = webQuery.QueryKey.ToString() })) {
            parentNode.Children.Add(childNode);
        }
        nodes.Add(parentNode);
    }
    return nodes;
}

Getting the user nodes becomes

var userNodes = GetNodesFromQuery(userKey, onlineReports, _QueryPermissions.GetQueriesForTaskAndUser);

Getting the group node

var groupNodes = GetNodesFromQuery(groupKey, onlineReports, _QueryPermissions.GetQueriesForTaskAndUserGroup);
share|improve this answer
add comment

What about using a delegate?

private delegate void Query(int value, int key, ref List<WebQuery>, bool b);

public void SomeCallingMethod()
{
  IEnumerable<OnlineReport> reports;
  // one way
  int myUserKey;
  Query q = _QueryPermissions.GetQueriesForTaskAndUser;
  var returnValue = GetNodesForUser(q, myUserKey, reports);
  //or the other
  int groupKey;
  q = _QueryPermissions.GetQueriesForTaskAndUserGroup;
  returnValue = GetNodesForUser(q, groupKey, reports);
}

public IEnumerable<Node> GetNodesForUser(Query query, int key, IEnumerable<OnlineReport> onlineReports)
{
  var nodes = new List<Node>();
  foreach (var onlineReport in onlineReports)
  {
    var parentNode = new Node { Text = onlineReport.Key, Value = onlineReport.Value };
    var webQueries = new List<WebQuery>();
    //Line that is different
    // _QueryPermissions.GetQueriesForTaskAndUser(Int32.Parse(onlineReport.Value), userKey, ref webQueries, true);
    query.Invoke(Int32.Parse(onlineReport.Value), key, ref webQueries, true);
    foreach (var childNode in webQueries.Select(webQuery => new Node(parentNode) { Text = webQuery.QueryTitle, Value = webQuery.QueryKey.ToString() }))
    {
      parentNode.Children.Add(childNode);
    }
    nodes.Add(parentNode);
  }
  return nodes;
}
share|improve this answer
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.