5
\$\begingroup\$

I have a function which needs to detect part of the day depending on whether it's the weekend or not (the weekend affects the lunch time indication). The method is quite long and I'd like to know how it can be better.

double PartOfDayRule::ExtractFeature(dto::ActionableItem & item) {
                double morning, firstWorkPart, lunch, secondWorkPart, afterWork, beforeSleep, sleepTime;
                morning = 0;
                firstWorkPart = 1;
                lunch = 2;
                secondWorkPart = 3;
                afterWork = 4;
                beforeSleep = 5;
                sleepTime = 6;

                double result = -1;
                dto::DateTime itemTime = item.scheduledStart;
                if(util::time::inRange(item.ctx->sleepTimeEnd, item.ctx->workTimeStart, itemTime)){
                    result = morning;
                    goto result;
                }

                dto::DateTime lunchStart;
                dto::DateTime lunchStop;
                if(!item.ctx->isWorkingDay){
                    lunchStart = item.ctx->workTimeStart;
                    lunchStop = item.ctx->workTimeFinished;
                }else{
                    long firstWorkPartLength = util::time::between<std::chrono::hours>(item.ctx->workTimeStart, item.ctx->workTimeFinished) / 2;
                    lunchStart = item.ctx->workTimeStart + std::chrono::hours{firstWorkPartLength};
                    lunchStop = lunchStart + std::chrono::hours{1};
                }
                if(util::time::inRange(lunchStart, lunchStop, itemTime)){
                    result = lunch;
                    goto result;
                }

                if(util::time::inRange(item.ctx->workTimeStart, lunchStart, itemTime)){
                    result = firstWorkPart;
                    goto result;
                }

                if(util::time::inRange(lunchStop, item.ctx->workTimeFinished, itemTime)){
                    result = secondWorkPart;
                    goto result;
                }

                dto::DateTime sleepPreparationTime = item.ctx->sleepTimeStart - std::chrono::hours{1};
                if(util::time::inRange(item.ctx->workTimeFinished, sleepPreparationTime, itemTime)){
                    result = afterWork;
                    goto result;
                }

                if(util::time::inRange(sleepPreparationTime, item.ctx->sleepTimeStart, itemTime)){
                    result = beforeSleep;
                    goto result;
                }

                result = sleepTime;
                result:
                return result;

            }
\$\endgroup\$
4
\$\begingroup\$

I would create a range structure to hold each range you care about, along with the value to associate with each, then search through that.

I'd also use an enumeration for the names of the parts of the day (double doesn't seem to make any real sense for this task).

IMO, anytime you're using std::chrono, it's worth considering using a namespace directive to bring it into scope.

Putting those together, we'd end up with code something on this order:

enum result_type { morning, firstWorkPart, lunch, secondWorkPart, afterWork, beforeSleep, sleepTime };

struct range {
    double begin;
    double end;
    result_type result;
};

double PartOfDayRule::ExtractFeature(dto::ActionableItem & item) {
    dto::DateTime lunchStart;
    dto::DateTime lunchStop;
    if (!item.ctx->isWorkingDay) {
        lunchStart = item.ctx->workTimeStart;
        lunchStop = item.ctx->workTimeFinished;
    }
    else {
        using namespace std::chrono;
        long firstWorkPartLength = util::time::between<hours>(item.ctx->workTimeStart, item.ctx->workTimeFinished) / 2;
        lunchStart = item.ctx->workTimeStart + hours{ firstWorkPartLength };
        lunchStop = lunchStart + hours{ 1 };
    }

    std::vector<range> ranges{
        { item.ctx->sleepTimeEnd, item.ctx->workTimeStart, morning},
        { lunchStart, lunchStop, lunch },
        { item.ctx->workTimeStart, lunchStart, firstWorkPart },
        { lunchStop, item.ctx->WorkTimeFinished, secondWorkPart },
        { item.ctx->workTimeFinished, sleepPreparationTime, afterWork },
        { sleepPreparationTime, item.ctx->sleepTimeStart, beforeSleep }
    };

    for (auto const &r : ranges)
        if (inRange(r.begin, r.end, itemTime))
            return r.result;
    return sleepTime;
}

The loop could also be written using find_if, but I'm not sure it provides a real improvement:

auto pos = find_if(ranges.begin(), ranges.end(), 
    [=](auto r) { return inRange(r.begin, r.end, itemTime); });
if (pos != ranges.end())
    return pos->result;
return sleepTime;
\$\endgroup\$
4
\$\begingroup\$

You return what is effectively an enum as a double. Why? create the enum and return that it's much clearer that way wat the return value means:

If you still want a single point of return (for whatever reason) then you can use an if-else chain:

enum PartOfDay{
    morning = 0,
    firstWorkPart = 1,
    lunch = 2,
    secondWorkPart = 3,
    afterWork = 4,
    beforeSleep = 5,
    sleepTime = 6
}

PartOfDay PartOfDayRule::ExtractFeature(dto::ActionableItem & item) {

    dto::DateTime lunchStart;
    dto::DateTime lunchStop;
    if(!item.ctx->isWorkingDay){
        lunchStart = item.ctx->workTimeStart;
        lunchStop = item.ctx->workTimeFinished;
    }else{
        long firstWorkPartLength = util::time::between<std::chrono::hours>(item.ctx->workTimeStart, item.ctx->workTimeFinished) / 2;
        lunchStart = item.ctx->workTimeStart + std::chrono::hours{firstWorkPartLength};
        lunchStop = lunchStart + std::chrono::hours{1};
    }

    dto::DateTime itemTime = item.scheduledStart;
    dto::DateTime sleepPreparationTime = item.ctx->sleepTimeStart - std::chrono::hours{1};
    PartOfDay result = sleepTime;

    if(util::time::inRange(item.ctx->sleepTimeEnd, item.ctx->workTimeStart, itemTime)){
        result = morning;
    } else if(util::time::inRange(item.ctx->workTimeStart, lunchStart, itemTime)){
        result = firstWorkPart;
    } else if(util::time::inRange(lunchStart, lunchStop, itemTime)){
        result = lunch;
    } else if(util::time::inRange(lunchStop, item.ctx->workTimeFinished, itemTime)){
        result = secondWorkPart;
    }else if(util::time::inRange(item.ctx->workTimeFinished, sleepPreparationTime, itemTime)){
        result = afterWork;
    }else if(util::time::inRange(sleepPreparationTime, item.ctx->sleepTimeStart, itemTime)){
        result = beforeSleep;
    }else {
        result = sleepTime;
    }
    return result;

} 

Or you can sort the ranges and then use std::find to get the range you want.

\$\endgroup\$
3
\$\begingroup\$

You can return at any point of the method and it will end, so there is no need to use goto (which is a bad idea most of the time). You can also delete some braces and fix the indentation.

double PartOfDayRule::ExtractFeature(dto::ActionableItem & item) {
    double morning, firstWorkPart, lunch, secondWorkPart, afterWork, beforeSleep, sleepTime;
    morning = 0;
    firstWorkPart = 1;
    lunch = 2;
    secondWorkPart = 3;
    afterWork = 4;
    beforeSleep = 5;
    sleepTime = 6;

    double result = -1;
    dto::DateTime itemTime = item.scheduledStart;

    if(util::time::inRange(item.ctx->sleepTimeEnd, item.ctx->workTimeStart, itemTime))
        return = morning;

    dto::DateTime lunchStart;
    dto::DateTime lunchStop;

    if( !item.ctx->isWorkingDay ){
        lunchStart = item.ctx->workTimeStart;
        lunchStop = item.ctx->workTimeFinished;
    } else {
        long firstWorkPartLength = util::time::between<std::chrono::hours>(item.ctx->workTimeStart, item.ctx->workTimeFinished) / 2;
        lunchStart = item.ctx->workTimeStart + std::chrono::hours{firstWorkPartLength};
        lunchStop = lunchStart + std::chrono::hours{1};
    }


    if(util::time::inRange(lunchStart, lunchStop, itemTime))
        return lunch;

    if(util::time::inRange(item.ctx->workTimeStart, lunchStart, itemTime))
        return firstWorkPart;

    if(util::time::inRange(lunchStop, item.ctx->workTimeFinished, itemTime))
        return secondWorkPart;

    dto::DateTime sleepPreparationTime = item.ctx->sleepTimeStart - std::chrono::hours{1};
    if(util::time::inRange(item.ctx->workTimeFinished, sleepPreparationTime, itemTime))
        return afterWork;

    if(util::time::inRange(sleepPreparationTime, item.ctx->sleepTimeStart, itemTime))
        return beforeSleep;

    return sleepTime;

} 
\$\endgroup\$
1
  • 2
    \$\begingroup\$ I'd also add that you can declare and initialize a variable on the same line (and with one per line), that constants should me marked as such and that so long function may be broken into smaller more self-descriptive snippets. \$\endgroup\$ – Adriano Repetti Nov 10 '16 at 16: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.