2
\$\begingroup\$

Any advice on how to make this code; cleaner, more effective, just overall better!

Program setup object that is coming from the database. it loop the object adding to the viewmodel object. Then the object has a certain properties base on type. The config type will only have the setting and value objects.

class Program
{
    static void Main(string[] args)
    {
        //Setting up fake objects
        List<Meeting> meetingList = new List<Meeting>();
        meetingList.Add(new Meeting() { Id =1 ,Name="a", Type= "System"  });
        meetingList.Add(new Meeting() { Id = 2, Name = "b", Type = "System" });
        meetingList.Add(new Meeting() { Id = 3, Name = "c", Type = "Config", Setting="Control", Value="2"  });
        meetingList.Add(new Meeting() { Id = 4, Name = "d", Type = "Config", Setting = "Control", Value = "3" });
        meetingList.Add(new Meeting() { Id = 5, Name = "e", Type = "Config", Setting = "", Value = "" });

        //Real Code
        List<Meeting> meetingviewmodel = new List<Meeting>();
        foreach (var meeting in meetingList)
        {
           Meeting mvm = new Meeting();
           meeting.Id = mvm.Id;
           meeting.Name = mvm.Name;
           meeting.Type = mvm.Type;
           if (meeting.Type == "System")
           {
               meetingviewmodel.Add(mvm);
           }
           if (meeting.Type == "Config" && meeting.Setting != null)
           {
              meeting.Setting = mvm.Setting;
              meeting.Value = mvm.Value;
              meetingviewmodel.Add(mvm);

            }
        }
    }

    class Meeting
    {
        public int Id { get; set; }
        public string Name { get; set; }
        public string Type { get; set; }
        public string Setting { get; set; }
        public string Value { get; set; }
    }
}
\$\endgroup\$
3
\$\begingroup\$

First, I think you've flipped the assigning without noticing that you're assigning null values to the meeting properties (e.g meeting.Id = mvm.Id;) notice that you've already create a new instance of Meeting object, and yet, you replaced the actual values with the new instance values, while you are expected to do the opposite.

So, I assumed you meant to do this mvm.Id = meeting.Id;.

If you need to add the Meeting object to the model based on the Type property. You can do this :

foreach (var meeting in meetingList)
{
    // if this element's Type property is null or empty
    // then skip and go to the next one
    // this condition will be applied on all meeting types 
    if(string.IsNullOrEmpty(meeting.Type)) { continue; }
    
    meetingviewmodel.Add(meeting);  
}

this will add all meeting objects that have defined Type, this will include System and Config types along with any other defined types.

if you only need to add System and Config types meetings, and ignore the rest, you can do this :

foreach (var meeting in meetingList)
{
    // if you want to only add System & Config meetings 
    // you can use this condition
    if(!string.IsNullOrEmpty(meeting.Type) && (meeting.Type == "System" || meeting.Type == "Config"))
    {
        meetingviewmodel.Add(meeting);
    }
}

or if you into LINQ :

var meetingviewmodel = meetingList
            .Where(x=> !string.IsNullOrEmpty(x.Type) && (x.Type == "System" || x.Type == "Config"))
            .ToList(); 
\$\endgroup\$
  • \$\begingroup\$ what if the setting and value are null if the type is system ? That’s why I did the two if statement . \$\endgroup\$ – Jefferson Jul 9 '20 at 21:18
  • \$\begingroup\$ Sorry I mean only add it to the view model when it is not null. So I need to make sure they have values before adding it to the view model \$\endgroup\$ – Jefferson Jul 9 '20 at 21:50
  • \$\begingroup\$ @Jefferson no, I apology for any inconvenience I caused you. It was my fault, I was not paying attention since I didn't have my coffee when I wrote my answer :(. I updated my answer for that. \$\endgroup\$ – iSR5 Jul 9 '20 at 22:27
  • 1
    \$\begingroup\$ @Jefferson yes, it'll add it, if you want to skip it, add a condition on setting like if(string.IsNullOrEmpty(meeting.Type) || string.IsNullOrEmpty(meeting.Setting)) \$\endgroup\$ – iSR5 Jul 9 '20 at 22:43
  • 1
    \$\begingroup\$ @Jefferson same thing on the value or any property that you want to skip if it's null or empty. \$\endgroup\$ – iSR5 Jul 9 '20 at 22:46

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.