Optimize code usage and repeated code

Question

I have a method that I use nested foreach and the same variable call multiple times. I read a lot of optimizing code but I'm pretty sure I can do this with less code and best practices:

 public async Task<IEnumerable<IssueViewModel>> Get(IssueSelectModel model)
 {
 async Task<IEnumerable<IssueViewModel>> DoGet()
 {
 var issueList = await _bugTrackerRepository.Get(model);
 var userList = await _userService.Get(new UserSelectModel { RowsPerPage = 1000 });
 foreach (var item in issueList.IssueList)
 {
 item.AssigneeName = userList.Items.FirstOrDefault(x => x.UserId == item.AssigneeId)?.UserName;
 item.CreatedByName = userList.Items.FirstOrDefault(x => x.UserId == item.CreatedBy)?.UserName;
 item.ModifiedByName = userList.Items.FirstOrDefault(x => x.UserId == item.ModifiedBy)?.UserName;
 item.ClosedByName = userList.Items.FirstOrDefault(x => x.UserId == item.ClosedBy)?.UserName;
 foreach(var comment in item.CommentList)
 {
 comment.CreatedByName = userList.Items.FirstOrDefault(x => x.UserId == comment.CreatedBy)?.UserName;
 foreach(var tag in comment.TagList)
 {
 tag.UserByName = userList.Items.FirstOrDefault(x => x.UserId == tag.UserId)?.UserName;
 }
 }
 }
 return issueList.IssueList;
 }
 return await DoGet();
 }

• Is it possible to use userList only one time?
• Is there an alternative of multiple foreach statement?

Regards

• BCdotWEB: The current question title of your question is too generic to be helpful. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?.

Answer 1

Is it possible to use userList only one time?

Every time you call a FirstOrDefault, you are doing an O(n) search through the user list. You should consider keeping a dictionary of user IDs to user objects. Without knowing what kind of frameworks you're using, I can't say whether this is a job for your ORM or whether you have to do it yourself. With a dictionary, user lookup will be much faster.

So short answer, no; but you shouldn't be using a list.

Is there an alternative of multiple foreach statement?

Not really.

Other things I see that could use some love:

Overuse of var

I find var to be useful in contexts where it's visually obvious what type a variable will take, including during instantiation statements. However, that is not the case with any of the instances of var in your code; I have no idea what's going into those variables. Unless the type is some kind of monstrous generic, just use the type.

Data model issues

You have some kind of user object, and some kind of item object with multiple references to user names. Are you sure it's a good idea to refer to the names, and not the user objects? The answer to this might be that you need the names in this case because the object is shortly destined for some kind of serialization, but if that isn't the case, it's probably a better idea to keep object references instead of string references.

In other words, why store comment.CreatedByName when you already have comment.CreatedBy?

• BCdotWEB: The usage of var isn't the problem, it's the badly named variables.
• Thoryn Hawley: "I find var to be useful in contexts where it's visually obvious what type a variable will take, including during instantiation statements. However, that is not the case with any of the instances of var in your code; I have no idea what's going into those variables. Unless the type is some kind of monstrous generic, just use the type." I couldn't agree more. var seems to be over used because its "easy", but that almost always comes at the cost of readability. I have argued with other devs who want to use it when declaring an int... so frustrating.