4
\$\begingroup\$

I have a bunch of code that deals with modifying a large nested set of IEnumerable<DTO's>

Basically, this is the structure of the DTO's

public class ProductLineDto
{ 
    ...
    public IEnumerable<ProjectTypeDto> ProjectType { get; set; }
}
public class ProjectTypeDto
{
    ...
    public IEnumerable<ProjectDto> Projects { get; set; }
}
public class ProjectDto
{
    ...
    public IEnumerable<SubProjectsDto> SubProjects { get; set; }
}

public class SubProjectDto
{
    ...
    public IEnumerable<SubProjectDto> Children { get; set; }
    public IEnumerable<ActivityDto> Activities { get; set; }
}

Here is what the code that uses them looks like.

  1. I was wondering if there was a way to clean up the code. I don't like to have to make a copy of every nested IEnumerable to modify the ganttData variable. I was thinking of maybe making an extension method take care of that for me, that way I wouldn't have to throw those two lines of code in for every IEnuemerable

  2. Are there any performance enhancements I can make? For instance, would it be faster to use Select() or Where() instead of RemoveAll()?

    public IEnumerable<ProductLineDto> FilterGanttChartData(IEnumerable<ProductLineDto> ganttData, GanttFilterDto ganttFilterDataModel)
    {
        ...
    
        ganttData = GetDataByEndDate(ganttData.ToList(), ganttFilterDataModel);
    
        return ganttData;
    }
    
        private IEnumerable<ProductLineDto> GetDataByEndDate(List<ProductLineDto> ganttData, GanttFilterDto ganttFilterDataModel)
        {
            if (ganttFilterDataModel.EndDateFrom == null && ganttFilterDataModel.EndDateTo == null)
                return ganttData;
    
            var gdata = new List<ProductLineDto>();
            gdata.AddRange(ganttData);
    
            for (var i = 0; i < gdata.Count; i++)
            {
                var pType = new List<ProjectTypeDto>();
                pType.AddRange(gdata[i].ProjectType);
    
                for (var j = 0; j < pType.Count; j++)
                {
                    var projects = new List<ProjectDto>();
                    projects.AddRange(pType[j].Projects);
    
                    for (var a = 0; a < projects.Count; a++)
                    {
                        var subProjects = new List<SubProjectDto>();
                        subProjects.AddRange(projects[a].Children);
                        subProjects = GetActivitiesByEndDateRange(subProjects, ganttFilterDataModel.EndDateFrom, ganttFilterDataModel.EndDateTo).ToList();
                        projects[a].Children = subProjects;
                    }
                    pType[j].Projects = projects;
                }
    
                gdata[i].ProjectType = pType;
            }
    
            ganttData = gdata;
            return ganttData;
        }
    

Then the recursive function follows the same logic

    private IEnumerable<SubProjectDto> GetActivitiesByEndDateRange(List<SubProjectDto> children, DateTime? endDateFrom, DateTime? endDateTo)
    {
        var subProjects = new List<SubProjectDto>();
        subProjects.AddRange(children);

        for (var i = 0; i < subProjects.Count; i++)
        {
            var acts = new List<ActivityDto>();
            acts.AddRange(subProjects[i].Activities);

            if (endDateFrom != null)
                acts.RemoveAll(activity => activity.EndDate < endDateFrom);
            if (endDateTo != null)
                acts.RemoveAll(activity => activity.EndDate > endDateTo);
            subProjects[i].Activities = acts;

            if (subProjects[i].Children.Any())
                subProjects[i].Children = GetActivitiesByEndDateRange(subProjects[i].Children.ToList(), endDateFrom, endDateTo);
        }

        children = subProjects;
        return children;
    }
\$\endgroup\$
5
  • \$\begingroup\$ I recommend you to learn and carefully apply the Linq extension methods (Select, Where, and other are among them). These methods are often possible to chain effectively, and they lead to code that follows functional style rather than imperative. In my experience, functional code is usually more robust and less error prone compared to imperative. I will try to sketch an answer in 8-10 hours when I get to my desktop. \$\endgroup\$ – Igor Soloydenko Apr 4 '17 at 7:36
  • \$\begingroup\$ Thank you, I am looking forward to it. I don't really like the road it's going down now. \$\endgroup\$ – Train Apr 4 '17 at 14:22
  • \$\begingroup\$ Your code appears to have a flaw. Here subProjects.AddRange(projects[a].Children);. Children is not member of ProjectDto. \$\endgroup\$ – user33306 Apr 4 '17 at 15:52
  • \$\begingroup\$ @OrthoHomeDefense the answer is posted \$\endgroup\$ – Igor Soloydenko Apr 4 '17 at 15:57
  • \$\begingroup\$ @tinstaafl Thank you, that was a Typo on my end. I should have just copy and pasted. Will edit soon. \$\endgroup\$ – Train Apr 4 '17 at 16:16
2
\$\begingroup\$

Direction

I tried to rewrite your original code without much luck. After making a little bit of analysis I came to the idea that in order to have a more generic project shaking algorithm, we need a more generic project representation (data structure). Here's what the code looks after my second attempt.

Code

Here's the project tree object that has arbitrary number of subproject children defined as the same type as project itself (see Composite Pattern). The type placeholder T is something that will need some time to design properly, and this we can address separately.

Also, see that we've got the Activities enumerable declared on this level. Which means that even the root of the project hierarchy may have some activities. This is good and bad at the same time: it's good because now there's no artificial limitation applied to the root level; but it's bad because it diverges from what you had originally and if the original limitation is a business rule, you will need an external mechanism to maintain the rule.

public interface IProjectTree<T>
    : IEnumerable<IProjectTree<T>> // !!! The implementation should rely on Children field
{
    IEnumerable<IProjectTree<T>> Children { get; set; }
    IEnumerable<ActivityDto> Activities { get; set; }

    T SubtreeSpecificData { get; set; }
}

Now we can build a Traverse class that implements a generic method ShakeOff which is generalized and can be applied to any part of the tree and with any thinkable predicate on activity object (passed as a parameter).

public static class Traverse
{
    public static IProjectTree<T> ShakeOff<T>(this IProjectTree<T> targetTree, Func<ActivityDto, bool> activityPredicate)
    {
        if (!targetTree.Any())
        {
            targetTree.Children = new List<IProjectTree<T>>();
        }
        else
        {
            targetTree.Children = targetTree
                .Children
                .Select(subTree => subTree.ShakeOff(activityPredicate))
                .ToList();
        }

        targetTree.Children = targetTree.Children.Where(subtree => subtree.ActivityPredicateApplies(activityPredicate));

        return targetTree;
    }

    public static bool ActivityPredicateApplies<T>(this IProjectTree<T> target, Func<ActivityDto, bool> activityPredicate)
    {
        target.Activities = target.Activities.Where(activityPredicate);

        var hasSubtreesAndUnfilteredActivities = target.Any() && target.Activities.Any();
        return hasSubtreesAndUnfilteredActivities;
    }
}

Consumption

The consumer will then look like this:

DateTime fromEndDate = ...;
DateTime toEndDate = ...;
Func<ActivityDto, bool> activityPredicate = activity =>
     activity.EndDate >= fromEndDate && activity.EndDate < toEndDate;

IProjectTree<...> projectTree = null;
projectTree = projectTree.ShakeOff(activityPredicate);

Side notes

But wait, I can't change my DTO classes into IProjectTree. I am pretty sure your DTO structure is shaped by the Database or the service and you want to keep them as they are. This is the hardest part, and you may not like the answer.

You will need to translate your DTOs into IProjectTree<T>. It means, all your DTOs stay unchanged; add the ProjectTree type; and write a class that translates one thing into another (and vice versa). This will require a bit of work. And you will need to find a proper way to define the <T> (the trivial solution would be an object, the SubtreeSpecificData could then point to the source DTO ;) ).

IMO, this would be a better design. Operating directly on DTOs is counter-intuitive to me. DTOs are not supposed to be used as a part of the domain logic, but rather as a dummy objects in client-server communication processes.

Disclaimer

  1. Traverse.ShakeOff() method is not a pure function, it modifies the provided targetTree object in-place. This may be dangerous in some scenarios (but probably, not in your case).
  2. The code above may or may not be easy to apply due to architectural changes -- it's your judgement call. If I were you, I would not limit myself to a rigid unmanageable data structure just because my database/service returns DTOs in this format. As mentioned earlier, generic representation enables generic algorithms which are used a LOT (meaning the effort pays off).
  3. I didn't have much time to test the solution (in fact, I still don't have access to my compiler), so burden of testing is on you.

Update 1

I updated code as per t3chb0t's comment and own observations:

  1. Added IEnumerable<IProjectTree<T>> to IProjectTree<T>.
  2. Replaced .Count() > 0 with .Any() where applicable.
  3. Using extension method subTree.ShakeOff(activityPredicate) instead of an explicit static method invocation.
\$\endgroup\$
7
  • 2
    \$\begingroup\$ I'd made the IProjectTree<T> derive from IEnumerable<T> so that you don't have to check the targetTree.Children == null but just use !targetTree.Any() etc. It's easier to work without nulls and I think here they can be avoided. \$\endgroup\$ – t3chb0t Apr 4 '17 at 16:09
  • \$\begingroup\$ @t3chb0t I agree! Nulls are evil in many many cases including this one. The thing you just mentioned is a nice new trick for me -- will use it in my project. P.S. I really need to get to my compiler so that I don't post answers with Disclaimers. \$\endgroup\$ – Igor Soloydenko Apr 4 '17 at 16:14
  • 1
    \$\begingroup\$ I have to say this is pretty much exactly what I wanted but didn't know how to accomplish. Thank you very much! \$\endgroup\$ – Train Apr 4 '17 at 16:28
  • \$\begingroup\$ @OrthoHomeDefense once again. please carefully check the code for correctness. Assume that I wrote the sketch while being drunk :) \$\endgroup\$ – Igor Soloydenko Apr 4 '17 at 16:30
  • \$\begingroup\$ @IgorSoloydenko :) Even so, This is a great example to work off of. \$\endgroup\$ – Train Apr 4 '17 at 16:38

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.