2
\$\begingroup\$

I'm trying to take a result set and organize it into a dictionary then assign the dictionary values to variables that are publicly accessible from my controller. My issue is I have to check if there is a key, if there is a key, assign it to a variable manually, if there isn't set the variable to 0. Is there a better dynamic way of doing this? The code works but I suspect there is a better implementation.

public class OtherTimeTypes
     {
    private PersonnelManagementEntities db = new    PersonnelManagementEntities();
    private string _usrName;
    public double takenCompHours { get; private set; }
    public double takenBereavementHours { get; private set; }
    public double takenJuryDutyHours { get; private set; }
    //generate the different DayTypes in a dictionary for key/value processing. Key = variable name, Value = DayTypeID
    private Dictionary<string, int> dayType = new Dictionary<string, int>
    {
        { "takenCompHours", 7 },
        { "takenBereavementHours", 3 },
        { "takenJuryDutyHours", 4 }
    };

    public OtherTimeTypes(string userName)
    {
        //set the AD user name in constructor
        _usrName = userName;
        //Setup dictionary to store results
        Dictionary<string, double> hoursCount = new Dictionary<string, double>();
        //initalize base line hours
        double returnedHours = 0;
        // iterate through dictionary with linq queries
        foreach (KeyValuePair<string, int> pair in dayType)
        {
            var result = db.TimeRequests
                           .Include(t => t.User)
                           .Where(t => t.DayTypeID == pair.Value)
                           .Where(t => t.User.ADUserName == _usrName)
                           .Where(t => t.UserSubmitDate.Year == DateTime.Now.Year)
                           .ToList();

            //make the hours calculation and store it in dictionary.
            foreach (TimeRequest hours in result)
            {   
                returnedHours = returnedHours + (((hours.eDateTime - hours.sDateTime).TotalDays + 1) * (int)hours.HoursRequested);
                //If the dictionary does not contain a K/V add it
                if (!hoursCount.ContainsKey(pair.Key))
                {
                    hoursCount.Add(pair.Key, returnedHours);
                }
                //If it does contain a value for the day type add it to the value compDay1 = 2 hrs, compDay2 = 4 hrs = compDayVariable=6
                else
                {
                    hoursCount[pair.Key] = hoursCount[pair.Key] + returnedHours;
                }
                returnedHours = 0;
            }
        }
//THIS IS THE PORTION OF CODE I DISLIKE
        //assign dictionary values to public variables for the TimeOffViewModel
        if (hoursCount.ContainsKey("takenCompHours"))
        { takenCompHours = hoursCount["takenCompHours"]; }
        else { takenCompHours = 0; }

        if (hoursCount.ContainsKey("takenBereavementHours"))
        { takenBereavementHours = hoursCount["takenBereavementHours"]; }
        else { takenCompHours = 0; }

        if (hoursCount.ContainsKey("takenJuryDutyHours"))
        { takenJuryDutyHours = hoursCount["takenJuryDutyHours"]; }
        else { takenJuryDutyHours = 0; }
    }
}
\$\endgroup\$
4
\$\begingroup\$

Naming

The C# standard for publicly-visible properties is to use PascalCase:

public double TakenCompHours { get; private set; }
public double TakenBereavementHours { get; private set; }
public double TakenJuryDutyHours { get; private set; }

Additionally, db is not a very descriptive name for your entities. It seems something like personnel would better indicate what it is.

Similarly, dayType is a dictionary storing the types of time-off requests, and it is a field, so I would call it something like _timeRequestTypes.

Finally, the class name itself, OtherTimeTypes reads more like the type contains a list of possible time types that fall outside a standard set of types.

However, it's telling you the aggregate number of comp, bereavement, and jury duty hours requested for a particular user over the current year. I'm pretty bad at naming myself, though, so perhaps someone else can come up with some good alternatives.

Design

The first thing I see is that the dayType dictionary only seems to exist as a way of identifying the int values the database uses. This would actually be much simpler as an enum value:

public enum TimeRequestType
{
    BereavementHours = 3,
    JuryDutyHours = 4,
    CompHours = 7,
}

The next thing I see is that you have several discrete properties to store different types of hours, but you are using a dictionary to do the same thing. You could just as easily add directly to the properties, or with some method extraction, you could perform a Sum in your LINQ.

Another thing I see is that you are hard-coding the year to be the current year. I would instead pass that as a parameter. Sure, the 99% case is that you are going to ask for the current year, but it is trivial to parameterize that, and you'll be glad you did if you ever have to get the values for not-this-year.

The last major thing I see is two-fold:

  • OtherTimeTypes both stores the data values returned for a user as well as performs the entity look-ups required to retrieve that data
  • All work is being done in a constructor

For this, I would first extract a data object class to store the values we are interested in:

public class UserTakenHours
{
    public UserTakenHours(double compHours, double bereavementHours, double juryDutyHours)
    {
        TakenCompHours = compHours;
        TakenBereavementHours = bereavementHours;
        TakenJuryDutyHours = juryDutyHours;
    }

    public double TakenCompHours { get; }
    public double TakenBereavementHours { get; }
    public double TakenJuryDutyHours { get; }
}

Then, I would move the logic for retrieving the data and aggregating it into a static method that returned the data object, along with some helper functions.

First, you can extract a method for calculating the hours in a particular TimeRequest:

private static double GetRequestedTime(TimeRequest request)
{
    return (((request.eDateTime - request.sDateTime).TotalDays + 1) * (int)request.HoursRequested);
}

With that, you can condense the foreach loop on your requests into a single-line LINQ statement:

result.Select(GetRequestedTime).Sum();

I would then extract the body of your loop on dayType into its own method, taking in arguments for the entities, username, year, and type:

private static double CalculateRequestedTime(PersonnelManagementEntities personnel, string user, int year, TimeRequestType type)
{
    var requests = personnel.TimeRequests
                            .Include(t => t.User)
                            .Where(t => t.DayTypeID == (int)type)
                            .Where(t => t.User.ADUserName == user)
                            .Where(t => t.UserSubmitDate.Year == year)
                            .ToList();

    return requests.Select(GetRequestedTime).Sum();
}

And finally, you can change the constructor into a static method that returns the data object:

public static UserTakenHours GetRequestedHours(string user, int year)
{
    var personnel = new PersonnelManagementEntities();
    var compHours = CalculateRequestedTime(personnel, user, year, TimeRequestType.CompHours);
    var bereavementHours = CalculateRequestedTime(personnel, user, year, TimeRequestType.BereavementHours);
    var juryDutyHours = CalculateRequestedTime(personnel, user, year, TimeRequestType.JuryDutyHours);

    return new UserTakenHours(compHours, bereavementHours, juryDutyHours);
}
\$\endgroup\$
  • \$\begingroup\$ This is beautiful, thank you for taking the time to write out this response. Greatly appreciated. \$\endgroup\$ – Waragi Apr 22 '16 at 18:54
1
\$\begingroup\$

Your else statements are superfluous. Just initialize the variables to zero and only set them if there is a valid key.

takenCompHours = 0; 
takenCompHours = 0;
takenJuryDutyHours = 0;

if (hoursCount.ContainsKey("takenCompHours"))
    { takenCompHours = hoursCount["takenCompHours"]; }

if (hoursCount.ContainsKey("takenBereavementHours"))
    { takenBereavementHours = hoursCount["takenBereavementHours"]; }

if (hoursCount.ContainsKey("takenJuryDutyHours"))
    { takenJuryDutyHours = hoursCount["takenJuryDutyHours"]; }
\$\endgroup\$
  • 2
    \$\begingroup\$ Looks like a case for TryGetValue(key, out value). \$\endgroup\$ – RubberDuck Apr 22 '16 at 20:56

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.