5
\$\begingroup\$

I have written this code as a college assignment fot the famous job sequencing problem. Please tell me any improvements if possible. Oh, for algorithms.h header file, refer my github repo

The code:

#include "algorithms.h"

struct Job {
    int index = 0;
    int time_slot = 0;
    int profit = 0;
public:
    Job() = delete;
    explicit Job(int i, int time, int pr) {
        index = i;
        time_slot = time;
        profit = pr;
    }
    bool operator>=(Job j) {
        if ((this->time_slot == j.time_slot && this->profit >= j.profit) || (this->time_slot > j.time_slot)) {
            return true;
        }
        return false;
    }
    bool operator<=(Job j) {
        if ((this->time_slot == j.time_slot && this->profit <= j.profit) || (this->time_slot < j.time_slot)) {
            return true;
        }
        return false;
    }
    bool operator<(Job j) {
        if ((this->time_slot == j.time_slot && this->profit < j.profit) || (this->time_slot < j.time_slot)) {
            return true;
        }
        return false;
    }
    bool operator>(Job j) {
        if ((this->time_slot == j.time_slot && this->profit > j.profit) || (this->time_slot < j.time_slot)) {
            return true;
        }
        return false;
    }
    bool operator==(Job j) {
        if (this->time_slot == j.time_slot) {
            return true;
        }
        return false;
    }
    friend std::ostream& operator<<(std::ostream& out, Job job) {
        out << job.index;
        return out;
    }
};

int main() {
    Job j1(1, 1, 3);
    Job j2(2, 3, 5);
    Job j3(3, 4, 20);
    Job j4(4, 3, 18);
    Job j5(5, 2, 1);
    Job j6(6, 1, 6);
    Job j7(7, 2, 30);
    std::vector<Job> vect = { j1,j2,j3,j4,j5,j6,j7 };
    vect = Sorter<Job>::mergeSort(vect);
    auto order = [&]() {
        auto it = vect.begin();
        while (it + 1 != vect.end()) {
            if (*it == *(it + 1)) {
                it = vect.erase(it);
                continue;
            }
            it++;
        }
    };
    order();
    std::for_each(vect.begin(), vect.end(), [](Job i) {std::cout << i << " "; });
    return 0;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you for accepting my answer 1 minute 4 seconds after I posted it, but you may consider waiting for a couple of days to accept an answer. This gives other reviewers the opportunity to see the question and provide their answers. \$\endgroup\$ – L. F. Mar 29 at 7:06
  • \$\begingroup\$ Okay. Sure :) I will keep this in mind henceforth \$\endgroup\$ – cpplover Mar 29 at 7:07
5
\$\begingroup\$

The Job class has public members, so it's simpler to make it an aggregate:

struct Job {
    int index;
    int time_slot;
    int profit;
};

Then, use std::tie to simplify the comparison operators: (operands to these operators should be marked as const, which you didn't)

bool operator<(const Job& lhs, const Job& rhs)
{
    return std::tie(lhs.time_slot, lhs.profit) < std::tie(rhs.time_slot, rhs.profit);
}

// implement other operators in terms of <

You == is not consistent with other comparison operators, which is confusing and likely to cause problems.

Also note that if (condition) { return true; } return false; should be changed to return condition; for clarity.

This is convoluted:

Job j1(1, 1, 3);
Job j2(2, 3, 5);
Job j3(3, 4, 20);
Job j4(4, 3, 18);
Job j5(5, 2, 1);
Job j6(6, 1, 6);
Job j7(7, 2, 30);
std::vector<Job> vect = { j1,j2,j3,j4,j5,j6,j7 };

It should be simplified to

std::vector<Job> vect {
    {1, 1, 3}, {2, 3, 5}, // ...
};

This:

auto order = [&]() {
    auto it = vect.begin();
    while (it + 1 != vect.end()) {
        if (*it == *(it + 1)) {
            it = vect.erase(it);
            continue;
        }
        it++;
    }
};
order();

should be

vect.erase(std::unique(vect.begin(), vect.end()), vect.end());

(provided that I understand the code correctly).

And this:

std::for_each(vect.begin(), vect.end(), [](Job i) {std::cout << i << " "; });

is a complicated way of writing

for (const auto& job : vect) {
    std::cout << job << ' ';
}

or

std::copy(vect.begin(), vect.end(),
          std::ostream_iterator<Job>{std::cout, ' '});
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks a lot. Well, I tried the std::unique thing looking at the SO questions asked on it. It somehow doesn't work. I mean, it does not do what's expected. The problem is that it is not calling my overloaded operator. How do I go about this? \$\endgroup\$ – cpplover Mar 29 at 7:07
  • 1
    \$\begingroup\$ @cpplover std::unique should call the == associated with Job. It is possible that failing to mark your operator== as const made std::unique unable to pick it up. Anyway, you should not overload == just for the purpose of std::unique - pass a custom comparator instead. \$\endgroup\$ – L. F. Mar 29 at 7:12
  • 1
    \$\begingroup\$ @cpplover std::unique is calling your comparison function, the difference is in how it eliminates duplicates. Your order() handles consecutive duplicates by removing the first duplicate, whereas std::unique will leave the first, and move any that follow to the end of the list. So, when you use std::unique you get the same result in terms of .time_slot (1, 2, 3, 4), but the jobs retained are different. (Enhancing the << operator to output the entire Job triple, not just the .index makes inspection easier.) You might be able to finesse your behavior with reverse iterators. \$\endgroup\$ – FeRD Mar 29 at 22:21
  • \$\begingroup\$ +1 for " if (condition) { return true; } return false; should be changed to return condition;". \$\endgroup\$ – nkvns Apr 5 at 7:45
3
\$\begingroup\$
  1. public: is excess in a struct.
  2. Members directly initialized in ctors are better directly initialized:
Job(int index, int time_slot, int profit)
    : index(index), time_slot(time_slot), profit(profit) {}

only more complex logic should go in ctor's body, if any.

  1. As a matter of fact,
    if(cond) {
        return true;
    }
    return false;

is actually return cond;, indeed.

  1. this-> simply increases the code size without any additional benefits.
| improve this answer | |
\$\endgroup\$
  • 2
    \$\begingroup\$ "explicit is excess for a ctor of arity greater than one." Actually no, explicit prevents copy-list-initialization. \$\endgroup\$ – L. F. Mar 29 at 9:18
  • \$\begingroup\$ @L.F. Thanks, forgot. \$\endgroup\$ – bipll Mar 30 at 4:21

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.