Calculating daily project drilling hours

Question

I've got this method, and it's quite a big method. I look at it, and I feel... dirty. I feel that it's doing too much, and that there might be a better way to accomplish what I'm trying to.

/// <summary>
/// Calculates daily project drilling hours
/// </summary>
/// <param name="daily">Bool: True for daily, false for total project</param>
/// <param name="timeFrame">Bool: True for specific time frame, false for whole day </param>
/// <returns>Double:  Drilling hours for time frame</returns>
public double CalculateDrillingHours(bool daily = false, bool timeFrame = false)
{
    Logger.Info("Calculating Drilling Hours");

    var query = "SELECT timestamp FROM myDatabase WHERE measured_dist = bit_loc AND rop > 0";

    query = BuildQuery(query, daily, timeFrame);

    var tempList = ExecuteQuery(query);

    // The user needs to know if there was no data for the time frame chosen
    var noData = new Exception("There was no data for the selected time frame.  Please select another. (Drilling Hours)");

    if (tempList.Count < 1)
    {
        throw noData;
    }

    if(!daily)
    {
        TotalProjectDrillingHours = Convert.ToDouble(Math.Round(TimeCalculations(ConvertStringListToDateTimeList(tempList)).TotalHours, 2, MidpointRounding.AwayFromZero));

        Logger.Info("Total Drilling Hours calculated");

        return TotalProjectDrillingHours;
    }

    DailyDrillingHours = Convert.ToDouble(Math.Round(TimeCalculations(ConvertStringListToDateTimeList(tempList)).TotalHours, 2, MidpointRounding.AwayFromZero));

    Logger.Info("Daily Drilling Hours calculated");

    return DailyDrillingHours;
}

This method returns 1 of 2 items: Either Daily Drilling Hours, or Total Project Drilling Hours. This was originally two methods, but I combined them because a lot of the code was being duplicated, and it made sense at the time (about 6 months ago). But now, looking at it, I'd like to improve this. Is there a better way?

Yes, I know about parameterized queries. I've already incorporated those in the final query, which would look something like this:

query = "SELECT timestamp FROM myDatabase WHERE measured_dist = bit_loc AND rop > 0 AND date = @Date AND timestamp BETWEEN @StartTime AND @EndTime";

Answer 1

I would consider first splitting the method into smaller methods as other people have mentioned. The only suggestion for now is I might suggest changing your bool daily to a enumeration. This in theory means it might be easier to add a Hourly,Monthly etc reporting option. Although this is only slightly better in my opinion and I'm sure there are better approaches (maybe using interfaces?).

Something like this as an exercise in splitting things out into various methods keeping most of your code but just re-arranging it slightly.

public double CalculateDrillingHours(DrillingPeriod period = DrillingPeriod.Daily, bool timeFrame = false)
{
    Logger.Info("Calculating Drilling Hours");
    var tempList = GetDrillingTimes(period, timeFrame);

    var drillingHours = CalculateDrillingHours(tempList);
    SetDrillingTimes(period, drillingHours);

    return drillingHours;
}

private void SetDrillingTimes(DrillingPeriod period, double drillingHours)
{
    switch (period)
    {
        case DrillingPeriod.Daily:
            Logger.Info("Total Drilling Hours calculated");             
            DailyDrillingHours = drillingHours;
            break;
        case DrillingPeriod.Total:
            Logger.Info("Daily Drilling Hours calculated");
            TotalProjectDrillingHours = drillingHours;
            break;
        default:
            throw new NotImplementedException("Drilling period not supported");
    }
}

private List<DateTime> GetDrillingTimes(DrillingPeriod period, bool timeFrame)
{
    var query = "SELECT timestamp FROM myDatabase WHERE measured_dist = bit_loc AND rop > 0";

    var daily = period == DrillingPeriod.Daily;
    query = BuildQuery(query, daily, timeFrame);

    var tempList = ExecuteQuery(query);

    if (tempList.Any())
    {
        return tempList;
    }

    // I would look at possibly using a custom exception here i.e. NoDataException
    throw new NoDataException("There was no data for the selected time frame.  Please select another. (Drilling Hours)");
} 

private double CalculateDrillingHours(List<DateTime> projectHours)
{
    return Convert.ToDouble(Math.Round(TimeCalculations(ConvertStringListToDateTimeList(projectHours)).TotalHours, 2, MidpointRounding.AwayFromZero));
}

Answer 2

Without going too far I've simplified your method. I'll leave the bigger stuff to someone who is more familiar with the content. I haven't touched correctness and/or functionality. You should also have some try/catch around the query stuff and the conversions.

/// <summary>
/// Calculates daily project drilling hours
/// </summary>
/// <param name="daily">Bool: True for daily, false for total project</param>
/// <param name="timeFrame">Bool: True for specific time frame, false for whole day </param>
/// <returns>Double:  Drilling hours for time frame</returns>
public double CalculateDrillingHours(bool daily = false, bool timeFrame = false)
{
    Logger.Info("Calculating Drilling Hours");

    string queryString = "SELECT timestamp FROM myDatabase WHERE measured_dist = bit_loc AND rop > 0";
    var query = BuildQuery(queryString, daily, timeFrame);
    var queryResults = ExecuteQuery(query);

    // The user needs to know if there was no data for the time frame chosen
    if (queryResults.Count < 1)
    {
        throw new Exception("There was no data for the selected time frame.  Please select another. (Drilling Hours)");;
    }

    var drillingHours = Convert.ToDouble(Math.Round(TimeCalculations(ConvertStringListToDateTimeList(queryResults )).TotalHours, 2, MidpointRounding.AwayFromZero));

    Logger.Info((daily ? "Daily" : "Total") + " Drilling Hours calculated");

    return drillingHours;
}

Answer 3

I think your method is doing too much. Any time you have two possible measures returned, you have to be thinking code smell. When code is duplicated, instead of combining the methods into one method, refactor the duplicated code into separate methods. There are also too many concerns in the method.

Personally, I would extract out the data retrieval into its own class/methods, ideally with an interface to you can inject mocks and fakes for unit testing. I like the repository pattern for this, keeps things simple, and a lot of people know about it, so maintenance is fairly easy.

Seems the only way the measures differ is in the query. I would create two methods CalculateDailyDrillingHours and CalculateTotalDrillingHours. These two methods would call a thirds method, passing in the start and end times as required. The third method would then call the repository, and calculate the hours.

public double CalculateDailyDrillingHours()
{
     return CalculateDrillingHoursForDateRange(/*dateStartTime*/, /*dateEndTime*/);
}

public double CalculateTotalDrillingHours()
{
     return CalculateDrillingHoursForDateRange(/*dateStartTime*/, /*dateEndTime*/);
}

private double CalculateDrillingHoursForDateRange(DateTime startAt, DateTime, endAt)
{
     var drillHourList = _repository.GetDrillTimesBasedOnDate(startAt, endAt);

     return Convert.ToDouble(
            Math.Round(
                TimeCalculations(ConvertStringListToDateTimeList(tempList)).TotalHours, 
                2, 
                MidpointRounding.AwayFromZero));
}

I would go one step further and movethe calucation of hours to its own method, and give it a descriptive name, this will further clear up the intent of the above code.

public double ConvertTimestampsToHours(IEnumerable</*class*/> timestamps)
{
    return Convert.ToDouble(
            Math.Round(
                TimeCalculations(ConvertStringListToDateTimeList(tempList)).TotalHours, 
                2, 
                MidpointRounding.AwayFromZero));
}

private double CalculateDrillingHoursForDateRange(DateTime startAt, DateTime, endAt)
{
     var drillHourList = _repository.GetDrillTimesBasedOnDate(startAt, endAt);

     return ConvertTimestampsToHours(drillHourList);
}

You can add logging as required.

One other thing:

Instead of doing if (tempList.Count < 1), do if (!tempList.Any()), it makes the code a little more clear.