Convert ?: statement to handle more than 2 answers

Question

I'm fairly new to programming but have been tasked with maintaining some applications that were created by a previous employee. I have a ?: statement that now needs to handle more than a true or false statement but I'm not sure how to go about doing it. The code in question is:

    MailDomainContext mail = new MailDomainContext();
      mail.Load(mail.GetMailsQuery("Workforce Attendence Issue",
               loadEmp.Entities.Where(emp => emp.EmployeeID == _EmployeeID).First().Username,
               (loadEmp.Entities.Where(emp => emp.EmployeeID == _EmployeeID).First().EmployeeShiftID >= 2 ? "supervisor1" : "supervisor2"),
               loadEmp.Entities.Where(emp => emp.EmployeeID == _EmployeeID).First().FirstName,
               attendence.AttendenceDate.ToString("MM/dd/yyyy"),
               attendence.TimeLost,
               loadAbs.Entities.Where(abs => abs.AbsenceID == attendence.AbsenceID).First().AbsenceDescription,
               (from inf in loadAtt.Entities
               where inf.EmployeeID == _EmployeeID
               where inf.AttendenceDate > DateTime.Now.AddDays(30 * -1)
               where inf.Approved == false
               select inf).Count() + 1,
               attendence.UTOUsed
               ), null, null);

More specifically this line:

    (loadEmp.Entities.Where(emp => emp.EmployeeID == _EmployeeID).First().EmployeeShiftID >= 2 ? "supervisor1" : "supervisor2"),

I need to add 4 more supervisors to the list but haven't figured out a way to do it that doesn't make everything else unhappy. I apologize if this is too simple a question or if I left out some details you might need to know, as I said I'm pretty new to all of this.

Making you code more readable would be a good start. I'd place `loadEmp.Entities.Where(emp => emp.EmployeeID == _EmployeeID).First()` into a variable instead calling it over and over again. — Ash Burlaczenko, Aug 31 '12 at 19:39
I think this is a case of someone getting a little too clever for their own good, code should never look like this. — Kevin DiTraglia, Aug 31 '12 at 19:50

devdigital · Answer 1 · 2012-09-01T06:30:54.623

This code is needlessly hard to maintain and also inefficient and not very defensive. The code is retrieving the employee three times.

loadEmp.Entities.Where(emp => emp.EmployeeID == _EmployeeID).First().Username

The above line (and others) will throw an exception if the employee with _EmployeeID doesn't exist. Instead, you could use FirstOrDefault, or SingleOrDefault if you expect there to only ever be one employee with that ID (which should be the case as it looks like primary key for that entity). If loadEmp is actually an Entity Framework DbContext then you could also use Find.

You can do this query once and store the result in a local variable.

var employee = loadEmp.Entities.SingleOrDefault(emp => emp.EmployeeID == _EmployeeID);

if (employee == null)
{
   // Handle employee not found
}

To then get the supervisor string based on the employee, you could create a method which takes the minimum amount of information needed to calculate the supervisor string, and pass this into the method to get your result.

GetSupervisorRole(employee.EmployeeShiftID);

...

private string GetSupervisorRole(int employeeShiftID)
{
   // Logic here
}

You are the only person that actually suggested the author fix his horrible code. — Security Hound, Aug 31 '12 at 20:12

score 3 · Answer 2 · answered Aug 31 '12 at 19:38

One approach is to extract that code into a method and write that method any way you want.

Another approach is to use dictionary to map keys (if you have small number of them) to values.

var id =3;
var mapping = new Dictionary<int, string>() { 
  { 1, "first" },
  { 2, "second" },
  { 3, "first" } //you can map 2 values (1,3) to the same "first" string
};

string value;
if (!mapping.TryGetValue(id, out value))
{
  value = "unknown";
}

score 2 · Answer 3 · answered Aug 31 '12 at 19:43

Create the following method:

string GetSupervisor(int employeeShiftId) {
    if (employeeShiftId == 1) supervisor = "supervisor1";
    else if (employeeShiftId == 2) supervisor = "supervisor2";
    else if (employeeShiftId == 3) supervisor = "supervisor3";
    else if (employeeShiftId == 4) supervisor = "supervisor4";
}

Then call it from your code and assign the result to a variable supervisor, which you can then use in mail.Load():

int employeeShiftId = loadEmp.Entities
          .Where(emp => emp.EmployeeID == _EmployeeID).First()
          .EmployeeShiftID;
string supervisor = GetSupervisor(employeeShiftId);

MailDomainContext mail = new MailDomainContext();
mail.Load(mail.GetMailsQuery("Workforce Attendence Issue",
               loadEmp.Entities.Where(emp => emp.EmployeeID == _EmployeeID).First().Username,
               supervisor, // <-- Then use it here
               ...
);

Kevin DiTraglia · Answer 4 · 2012-08-31T19:54:47.260

I would replace this whole section

loadEmp.Entities.Where(emp => emp.EmployeeID == _EmployeeID).First().Username,
(loadEmp.Entities.Where(emp => emp.EmployeeID == _EmployeeID).First().EmployeeShiftID >= 2 ? "supervisor1" : "supervisor2"),
 loadEmp.Entities.Where(emp => emp.EmployeeID == _EmployeeID).First().FirstName,

with

//Assign these variables ahead of time so others reading your code can 
//figure out what's going on
var EmpID = loadEmp.Entities.Where(emp => emp.EmployeeID == _EmployeeID).First();
var UserName = EmpID.UserName;
var FirstName = EmpID.FirstName;
var Title = GetTitle(EmpID.EmployeeShiftID);

//Your original call
Mail.Load(mail.GetMailsQuery("Workforce Attendence Issue",
     UserName,
     Title,
     FirstName,
     //...Etc
);

//New method you will need to add, you could do this logic in line, but this will
//be less messy
private string GetTitle(int ShiftID)
{
    switch (ShiftID)
    {
         case 1:
             return "supervisor1";
             break;
         case 2:
             return "supervisor2";
             break;
         //...etc
    }
}

score 1 · Answer 5 · edited May 23 '17 at 12:12

Not that this is a great idea, but you can combine inline if's as so:

    static void Main(string[] args)
    {
        int EmpId = 2;
        string supervisor = EmpId == 4 ? "Supervisor4" :
                            EmpId == 3 ? "Supervisor3" :
                            EmpId == 2 ? "supervisor2" :
                                         "supervisor1" ;
        Console.WriteLine(supervisor);

    }

You can see another example of this in another stack question: Legible or not: C# multiple ternary operators + Throw if unmatched

I would probably instead go for the Method approach like KDiTraglia proposed where you just pass in the EmpId and get the supervisor name back, or alternatively the Dictionary lookup approach like Alexei Levenkov propsed though.

@BlueM - I would argue that this a horrible peice of code even the author thinks that. This solve the problem for the author of the question but its a horrible idea overall. — Security Hound, Aug 31 '12 at 20:08

Convert ?: statement to handle more than 2 answers

5 Answers5