-2
\$\begingroup\$

My goal is to write a CPU Scheduling Simulator in C++ using the STL features as far as possible. So far, I have only written the code for FCFS and have not provided any input method.

What sticks out most to me is the definition of the process class. The way I have designed the program, there is very poor encapsulation. Looking at my current situation, I have only two solutions in mind:

  1. Make all data members private. Provide accessors for PID, Arrival Time and Burst Time and mutators for the rest. However, I fear this would make my code bloated and also the use of mutators would only break encapsulation.

  2. Make FCFS(), displayResult() and any other algorithms that I add friends of the process class. This allows me to control the encapsulation to a certain extent so I like this one.

Another thing I tried to do was make the PID, Arrival Time and Burst Time variables const and public. This allowed algorithms to read their values but not modify them. However, the code wouldn't compile because const members aren't default assignable.

I would like suggestions as to how I can solve the above mentioned problem and also how I can use the STL more effectively to make my code more expressive.

#include <iostream>
#include <vector>
#include <algorithm>
#include <limits>

constexpr int notYetComputed = std::numeric_limits<unsigned int>::max();

struct process 
{
    unsigned int m_procId;
    unsigned int m_arrivalTime;
    unsigned int m_burstTime;

    unsigned int m_responseTime;
    unsigned int m_completionTime;
    unsigned int m_turnaroundTime;
    unsigned int m_waitingTime;

    process(unsigned int pid, unsigned int at, unsigned int bt)
        : m_procId {pid}, m_arrivalTime {at}, m_burstTime {bt}
    {
        m_waitingTime = m_turnaroundTime = m_completionTime = m_responseTime = notYetComputed;
    }
};

void displayResult(const std::vector<process>& procs)
{
    for(auto& x : procs)
        std::cout << "PID: " << x.m_procId << ", "
            << "Waiting Time: " << x.m_waitingTime << ", "
            << "Turnaround Time: " << x.m_turnaroundTime << ", "
            << "Response Time: " << x.m_responseTime << ", "
            << "Completion Time: " << x.m_completionTime << "\n";
}
void FCFS(std::vector<process>& procList)
{
    //Sort based on arrival order. Use PID in case of same arrival time.
    auto arrivalOrder = [] (const process& p1, const process& p2) {
        if(p1.m_arrivalTime < p2.m_arrivalTime) return true;
        if(p1.m_arrivalTime == p2.m_arrivalTime) return (p1.m_procId < p2.m_procId);
        return false;
    };
    std::sort(procList.begin(), procList.end(), arrivalOrder);

    unsigned int clock {0};
    auto computeResult = [&clock] (process& pr) {
        pr.m_responseTime = clock - pr.m_arrivalTime;
        pr.m_waitingTime = (pr.m_turnaroundTime = 
            (pr.m_completionTime = (clock += pr.m_burstTime)) - pr.m_arrivalTime)
            - pr.m_burstTime;
    };

    std::for_each(procList.begin(), procList.end(), computeResult);

}

int main()
{   
    std::vector<process> procs {{0,0,5}, {1,1,3}, {2,2,8}, {3,3,6}};

    FCFS(procs);

    //Sort based on PID before showing result
    std::sort(procs.begin(), procs.end(), 
        [](const process& p1, const process& p2) {
            return p1.m_procId < p2.m_procId;
    });

    displayResult(procs);
}
\$\endgroup\$

closed as off-topic by πάντα ῥεῖ, dfhwze, 200_success, AlexV, pacmaninbw Jul 28 at 16:58

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Code not implemented or not working as intended: Code Review is a community where programmers peer-review your working code to address issues such as security, maintainability, performance, and scalability. We require that the code be working correctly, to the best of the author's knowledge, before proceeding with a review." – πάντα ῥεῖ, dfhwze, 200_success, AlexV, pacmaninbw
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 3
    \$\begingroup\$ If you can't change const values, there's probably a flaw in your design. Don't try to workaround this using a const_cast<> of such. Fix your overall design. What is FCFS BTW? \$\endgroup\$ – πάντα ῥεῖ Jul 28 at 10:14
  • \$\begingroup\$ I understand this question is not well received here. My fault entirely. Is this something more suited to stackexchange as this program is not really complete? \$\endgroup\$ – DS2830 Jul 28 at 10:34
  • 2
    \$\begingroup\$ "as this program is not really complete?" Yes that's the culprit. Don't ask for features not yet implemented or errors you get with these. It's well explained in the help center. \$\endgroup\$ – πάντα ῥεῖ Jul 28 at 11:10
1
\$\begingroup\$

Despite its being incomplete, I think there are a few points that can be made about the current code.

Naming

Naming is always difficult, and this gives quite a few examples. Somebody who already knows the purpose of the code can probably figure out that "FCFS" means "first come, first served", but to anybody else it's probably pretty opaque.

At least to me, a name like computeResult is more problematic though. This could apply about equally to essentially any function in existence, so it tells us nothing about what this particular computeResult actually is or does.

Library Usage

Your displayResult basically writes a collection of process objects to a stream. I'd prefer a stream inserter that writes a single object to a stream, then using a standard alorithm (or ranged for loop) to apply that to a collection of objects.

std::ostream &operator<<(std::ostream &os, process const &p) { 

    return os << "PID: " << x.m_procId << ", "
            << "Waiting Time: " << x.m_waitingTime << ", "
            << "Turnaround Time: " << x.m_turnaroundTime << ", "
            << "Response Time: " << x.m_responseTime << ", "
            << "Completion Time: " << x.m_completionTime;
}

Structure

It seems to me that your FCFS combines what are really a couple of separate functions. One is deciding the order in which to schedule the tasks. The other is going through the tasks in their chosen order, and setting the member variables for each. The initial sort is the only part that's really devoted to the first-come-first-served part. The rest would apply equally to other orders of execution.

As such, it seems to me that this should be split into two separate things. This could be done in a number of different ways. One would be to require the code that currently calls FCFS to make two separate calls, one to sort the tasks, and the other to simulate executing them. Another (that I think I'd generally prefer) would be for the caller to pass a comparator object that specifies the order it wants.

template <class Order>
void schedule(std::vector<process> &processes, Order order) { 

    std::sort(processes.begin(), processes.end(), order);

    unsigned int clock {0};
    auto computeResult = [&clock] (process& pr) {
        pr.m_responseTime = clock - pr.m_arrivalTime;
        pr.m_waitingTime = (pr.m_turnaroundTime = 
            (pr.m_completionTime = (clock += pr.m_burstTime)) - pr.m_arrivalTime)
            - pr.m_burstTime;
    };

    std::for_each(procList.begin(), procList.end(), computeResult);
}

Another that might be worth considering would be to have the process class itself specify the sorting order by defining bool operator< for itself. This would be particularly attractive if you were going to use an inheritance hierarchy. In this case, the base class would probably define operator< as a pure virtual function, and each derived process class would define operator< to specify the order it's going to use. This strikes me as problematic though--it would allow things like passing one vector of processes to the scheduler, where different process objects tried to specify different scheduling algorithms. Unless you really want that (and are prepared for the complexity it's likely to add) this probably isn't the best option.

In some cases this would be a pointless embellishment, but this is a scheduling simulator, so it seems nearly inevitable that you'd want to test different scheduling algorithms.

Clarity

IMO, one of the core computations is quite a bit less readable than it should be:

    pr.m_waitingTime = (pr.m_turnaroundTime = 
        (pr.m_completionTime = (clock += pr.m_burstTime)) - pr.m_arrivalTime)
        - pr.m_burstTime;

At least in my opinion, it would probably be better to break this up into a couple of pieces:

pr.m_waitingTime = clock - pr.m_arrivalTime;
pr.m_completionTime = clock + pr.m_burstTime;
pr.m_turnaroundTime = pr.m_completionTime - pr.m_arrivalTime;
clock += pr.m_burstTime;

[I think I may have gotten that a little wrong--as it stands, waiting time seems to be the same as response time--sorry, I really did find the nested assignments confusing.]

Encapsulation

Being tempted to provide accessors for member variables usually points toward having gotten the functionality in the wrong places. At least to me, that seems to be the case here.

At least in my opinion, most of the code to access the members of process should be in process itself. I'd have the scheduler tell the process to execute at a particular time, and the process would keep track of how that worked out for it:

class Process { 
    unsigned int m_procId;
    unsigned int m_arrivalTime;
    unsigned int m_burstTime;

    unsigned int m_responseTime;
    unsigned int m_completionTime;
    unsigned int m_turnaroundTime;
    unsigned int m_waitingTime;
public:
    process(unsigned int pid, unsigned int at, unsigned int bt)
        : m_procId {pid}, m_arrivalTime {at}, m_burstTime {bt}
    {
        m_waitingTime = m_turnaroundTime = m_completionTime = m_responseTime = notYetComputed;
    }

    int execute(int clock) { 
        m_responseTime = clock - m_arrivalTime;
        m_waitingTime = clock - m_arrivalTime;
        m_completionTime = clock + m_burstTime;
        m_turnaroundTime = m_completionTime - m_arrivalTime;
        return m_burstTime;
    }
};

This makes our scheduler look something like this:

template <class Order>
void schedule(std::vector<process> &processes, Order order) { 

    std::sort(processes.begin(), processes.end(), order);

    unsigned int clock {0};

    for (auto &process : processes)
        clock += process.execute(clock);
}

Now we have a clear separation of concerns. The scheduler is responsible for telling each task when to start executing. The task is responsible for doing its own book-keeping based on when it was told to execute. It then tells the scheduler how much clock time it used.

Accessors tend to give what I'd term "fake" encapsulation--we've hidden the variables themselves, but the client code still knows about the internals of what it's working with.

What we have here is closer to what I'd term "honest" encapsulation: we've created a really narrow interface where the scheduler and the process only know the tiniest bit about each other: the scheduler knows how to execute a process, and the process knows when it executes, it has to tell the caller how much clock time it used. But that's it.

\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.