4
\$\begingroup\$

I have two collections that I want to retrieve from the database and associate with each other. My solution works, however I know it isn't the most elegant one. Do you have any suggestions on how to improve the algorithm?

Classes:

 public class JobAnnouncement : IDbEntity
{
    public int Id { get; set; }
    public JobAnnouncementPosition Position { get; set; }
    public int PositionId { get; set; }
    public DateTime From { get; set; }
    public DateTime To { get; set; }
    public bool IsDeleted { get; set; }
    [Required]
    public string PositionNameAdvertised { get; set; }
    public int? PortalId { get; set; }
    public JobAnnouncementPortal Portal { get; set; }
    public string OtherPortal { get; set; }
    public int? UserId { get; set; }
    public PragisUser User { get; set; }
    [Required]
    public DateTime From { get; set; }
    [Required]
    public DateTime To { get; set; }
    public int ReceivedCvCount { get; set; }
    public int InterviewedCount { get; set; }
    public int RecruitedCandidatesCount { get; set; }
    public string Comment { get; set; }
}
public class JobAnnouncementPosition
{
    public int Id { get; set; }
    public ICollection<JobAnnouncement> JobAnnouncements { get; set; }
     public ICollection<JobAnnouncementDocumentInfo> DocumentsInfo { get; set; }
    public PragisUserType PragisUserType {get;set;}
    [Range(1, int.MaxValue)]
    public int? TeamId { get; set; }
    public Team Team { get; set; }

    [Range(1, int.MaxValue)]
    public int? TechnologyId { get; set; }
    public JobAnnouncementTechnology Technology { get; set; }
    public string OtherTechnology { get; set; }

    [Range(1, int.MaxValue)]
    public int? LevelId { get; set; }
    public JobAnnouncementLevel Level { get; set; }
    public bool IsDeleted { get; set; }
     [NotMapped]
    public int JobAnnouncementsCount
    {
        get
        {
            return JobAnnouncements != null ? JobAnnouncements.Count : 0;
        }
    }
    }

Repository returning all positions that are not deleted with all associated announcements that are not deleted. I was not able to return all data on one database call as the condition of IsDelted for child entity announcements was not checked and I always got all announcements back, no matter if they were or weren't deleted.

I made two calls and then iterated over them assigning announcements to positions again:

 public IEnumerable<JobAnnouncementPosition> GetForListActive()
    {
        var announcements = _context.JobAnnouncements
            .Include(a => a.Portal)
            .Where(a => a.To.Day >= DateTime.Now.Day)
            .Where(a => a.IsDeleted == false)
            .ToList();

        var positions = _context.JobAnnouncementPositions
            .Include(a => a.Team)
            .Include(a => a.Level)
            .Include(a => a.Technology)
            .Include("DocumentsInfo.Document")
            .Where(p => p.IsDeleted == false)
            .ToList();

        foreach (var announcement in announcements)
        {
            foreach (var position in positions)
            {
                if (announcement.PositionId == position.Id)
                {
                    position.JobAnnouncements.Add(announcement);
                }
            }
        }

        var result = new List<JobAnnouncementPosition>();
        for (var i = 0; i < positions.Count; i++)
        {
            if (positions[i].JobAnnouncements != null)
            {
                result.Add(positions[i]);
            }
        }


        return result;

    }
\$\endgroup\$
  • 1
    \$\begingroup\$ It looks like you're basically performing an inner join - have you looked at LINQ join operations? You might even be able to perform the join before your .ToList calls to have the join execute on the database. \$\endgroup\$ – Dan Lyons May 23 '16 at 17:56
2
\$\begingroup\$

Two comments -

  • Entity Framework already does what you do in the first loop. If you load entities into a context, for example by querying them, EF populates navigation properties by a process called relationship fixup.

    So if you do ...

    var announcements = _context.JobAnnouncements
        .Include(a => a.Portal)
        .Where(a => a.To.Day >= DateTime.Now.Day)
        .Where(a => a.IsDeleted == false)
        .ToList();
    
    var positions = _context.JobAnnouncementPositions
        .Include(a => a.Team)
        .Include(a => a.Level)
        .Include(a => a.Technology)
        .Include(a => a.DocumentsInfo.Select(d => d.Document))
        .Where(p => p.IsDeleted == false)
        .ToList();
    

    ... You'll notice that both JobAnnouncement.JobAnnouncementPosition and JobAnnouncementPosition.JobAnnouncements are populated as far as their entities are available.

  • The second loop isn't necessary. You can return positions. It won't contain null objects.

That would leave you with the two queries and a return statement.

But there's more.

As you say, if you use Include, you get all data, including deleted items. It's because of this annoying inability of EF to filter Includes why such elaborate workarounds are necessary. But there is a third-party library that makes this a lot easier: EntityFramework.DynamicFilters.

This library offers a simple API to define global filters that can be switched on and off. In you case it could look like:

modelBuilder.Filter("ActiveJobAnnouncements",
                        (JobAnnouncement a) => !a.IsDeleted);
modelBuilder.Filter("ActiveJobAnnouncementPositions",
                        (JobAnnouncementPositions p) => !p.IsDeleted);

Having these filters added to the model builder in OnModelCreating, you'll see that whenever you query any of these entities, the filter !IsDeleted will be part of the SQL query. Also when using Includes. If you want deleted items as well, just disable the filters in a context instance:

context.DisableFilter("ActiveJobAnnouncements");
context.DisableFilter("ActiveJobAnnouncementPositions");

I think this library is a gem.

One more thing: I my opinion, repositories, if you decide to use them, should be generic repositories. That means, for each entity class they do exactly the same thing. Your GetForListActive() method doesn't belong in a repository, rather in a service (that uses repositories).

\$\endgroup\$

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.