1
\$\begingroup\$

In my ASP.NET MVC application, I am loading Discussion Entry records along with User info (who posted the entry), Badges (assigned to the entry), Reactions (made to the entry like Facebook-style reaction). I retrieve these records from an SQL-View (with 23 columns) via a stored procedure (executed within Entity Framework). Each time I only retrieve the first 10 Entries (and all the associated data).

Then, I map these items (with 23 property) into objects properly and display them on the page:

 Discussion Entry 1 Content
    User: Name1 Surname1
    Badges: B51, B12, B23...
    Reactions: R1, R32, R3....

          Reply to Discussion Entry 1 (This is also a discussio entry actually)
             User: Name5 Surname5
             Badges: B1, B6, B3...
             Reactions: R1, R12, R3....

 Discussion Entry 2 Content
    User: Name2 Surname2
    Badges: B1, B2, B3...
    Reactions: R1, R2, R3....

However, I need to have several loops to properly load the Badges and Reactions list objects for each entry. I realized this consumes too much memory (200MB) and processor power. I thought making a single db call would be the most effective instead of making multiple db calls. However, it does not seem to work. Do you think my code is inefficient and need some revisions?

var entryviews = db.Database.SqlQuery<ViewEntryRecord>("CALL TO THE STORED PROCEDURE").ToList();            

foreach (var entryview in entryviews)
{

        var entryitem = new Entry()
        {
            EntryId = entryview.EntryId,
            Date = entryview.Date,
            Title = entryview.Title,
            Content = entryview.Content,
            ParentEntryId = entryview.ParentEntryId,
            EntryDepthness = entryview.EntryDepthness,
            IsAnonymous = entryview.IsAnonymous,
            IsHighlighted = entryview.IsHighlighted,
            IsPinned = entryview.IsPinned,
            ChildCount = entryview.ChildCount,
            FirstDescendantCount = entryview.FirstDescendantCount,
            User = new ApplicationUser
            {
                Id = entryview.UserId,
                FirstName = entryview.FirstName,
                LastName = entryview.LastName,
                AnonymousName = entryview.AnonymousName,
                AnonymousPhoto = entryview.AnonymousPhoto,
                ProfilePhoto = entryview.ProfilePhoto,
            }
        };
        entryitem.Badges = new List<BadgeAssignment>();
        List<int> recordedBadgeIds = new List<int>();

        foreach (var item in entryviews.Where(ew => ew.EntryId == entryview.EntryId && ew.BadgeId != null))
        {
            if (!recordedBadgeIds.Contains((int)item.BadgeId))
            {
                var bdg = new BadgeAssignment
                {
                    BadgeId = (int)item.BadgeId,
                    BadgeAssociated = new Badge
                    {
                        BadgeId = (int)item.BadgeId,
                        BadgeName = item.BadgeName,
                        Description = item.BadgeDescription
                    }
                };

                entryitem.Badges.Add(bdg);

                recordedBadgeIds.Add((int)item.BadgeId);
            }
        }

        entryitem.ReactionsToEntry = new List<ReactionToEntry>();
        List<string> recordedReactionIdUserIds = new List<string>();
        foreach (var item in entryviews.Where(ew => ew.EntryId == entryview.EntryId && ew.ReactionId != null))
        {
            if (!recordedReactionIdUserIds.Contains(item.ReactionId + "." + item.RTEUserId))
            {
                var rte = new ReactionToEntry
                {
                    ReactionId = (int)item.ReactionId,
                    EntryId = entryitem.EntryId,
                    UserId = item.RTEUserId
                };
                entryitem.ReactionsToEntry.Add(rte);
                recordedReactionIdUserIds.Add(item.ReactionId + "." + item.RTEUserId);
            }
        }

        if (!entries.Any(ent => ent.EntryId == entryitem.EntryId))
            entries.Add(entryitem);

}
\$\endgroup\$
0

1 Answer 1

1
\$\begingroup\$

This

if (!entries.Any(ent => ent.EntryId == entryitem.EntryId))
    entries.Add(entryitem);  

shouldn't be placed at the bottom of your loop. Instead before you are creating the Entry you should check if there is any Entry in the entries and if yes you should just continue. Your code should only do the work it needs to do.


For each item you are iterating over the whole views additional two times which isn't quite necessary. You should extract a collection out of entryviews which contains only the items where ew => ew.EntryId == entryview.EntryId and filter this later on for the BadgeAssignment and ReactionToEntry like so

var currentEntries = entryviews.Where(ew => ew.EntryId == entryview.EntryId).ToList();

....

foreach (var item in currentEntries.Where(ew.BadgeId != null))
{
    if (!recordedBadgeIds.Contains((int)item.BadgeId))
    {
        var bdg = new BadgeAssignment
        {
            BadgeId = (int)item.BadgeId,
            BadgeAssociated = new Badge
            {
                BadgeId = (int)item.BadgeId,
                BadgeName = item.BadgeName,
                Description = item.BadgeDescription
            }
        };

        entryitem.Badges.Add(bdg);

        recordedBadgeIds.Add((int)item.BadgeId);
    }
}

I would like to encourage you to always use braces {} although they might be optional. This helps you to make your code less error prone (e.g by mistake) and better structured which will improve the readability.


You coding style isn't consistent. One time you are using () although you are using object initialization (see var entryitem) and the other time you omit them (see User = new ApplicationUser).

Switching styles is a bad habit which should be avoided to improve the readability of your code.


\$\endgroup\$
1
  • \$\begingroup\$ Thanks for all recommendations. Memory use decreased from ~250 MB to ~180MB. \$\endgroup\$
    – renakre
    Apr 19, 2016 at 17:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.