I am practicing opearator overloading and to write copy constructor. I have written this program of scheduling algoritrhm. Please suggest me some improvements.

#include <iostream>
#include <vector>

class Scheduling

 int process;
 std::vector<int> burst_time;
 std::vector<int> waiting_time;

 public:
 Scheduling() : process(0) 
 Scheduling(int n) : process(n)
 
 for (int i = 0; i < n; i++)
 
 std::cout << "Enter Burst time for process " << i+1 << " : ";
 int val;
 std::cin >> val;
 burst_time.push_back(val);
 
 

 //copy constructor
 Scheduling(const Scheduling &other)
 
 process = other.process;
 int size = other.burst_time.size();

 for (int i = 0; i < size; i++)
 
 burst_time.push_back(other.burst_time[i]);
 
 

 //copy assignment
 Scheduling &operator=(const Scheduling &other)
 
 process = other.process;
 int size = other.burst_time.size();

 for (int i = 0; i < size; i++)
 
 burst_time.push_back(other.burst_time[i]);
 

 return *this;
 

 std::vector<int> cal_waiting_time(std::vector<int>& burst_time)
 
 waiting_time.push_back(0);

 for (int i = 0; i < burst_time.size() - 1; i++)
 
 waiting_time.push_back(waiting_time[i] + burst_time[i]);
 
 return waiting_time;
 

 void print_table()
 
 waiting_time = cal_waiting_time(burst_time);
 std::cout << "Processt Burst Timet Waiting Timen";
 for (int i = 0; i < burst_time.size(); i++)
 
 std::cout << i+1 << "tt " << burst_time[i] << "tt " << waiting_time[i] << "n";
 
 
;

int main()

 int num;
 std::cout << "Enter number of processn";
 std::cin >> num;
 Scheduling batch1(num);
 batch1.print_table();

 Scheduling batch2(batch1);
 batch2.print_table();

Let's dive into the review.

int process;

Does process serve any purpose? The only time it's used is when constructing the burst_time vector. And it's the same as burst_time.size().

Scheduling() : process(0) 
Scheduling(int n) : process(n)

 for (int i = 0; i < n; i++)
 
 std::cout << "Enter Burst time for process " << i+1 << " : ";
 int val;
 std::cin >> val;
 burst_time.push_back(val);
 

I don't know if you realize it, but both of these constructors do the same thing. You could remove the default constructor, and make the other one Scheduling(int n = 0), and avoid repeating yourself.

Whenever you write a constructor that takes a single argument, you almost always want to declare it explicit. This is for safety and efficiency, to avoid accidentally constructing an expensive Scheduling object.

Another issue here is that you don't do any error checking on your input. For toy code that's not a problem, but in real code, you'll want to check that each input doesn't fail. One easy (but not very efficient) way to do that is to read input into a string, and then convert it to an int with std::stoi(). If the conversion fails, an exception will be thrown, but at least your program won't fly into a tailspin.

So all of the above code might become something like:

explicit Scheduling(int n = 0)

 auto buffer = std::string;
 for (int i = 0; i < n; i++)
 
 std::cout << "Enter Burst time for process " << i+1 << " : ";
 std::getline(std::cin, buffer);
 burst_time.push_back(std::stoi(buffer));
 

I would also recommend not doing the input in the constructor. This is inflexible. Instead what you should do is have a constructor that takes a vector<int> as a parameter, and put I/O stuff elsewhere. That way you could read in your times either from std::cin (as you are), or from a data file, or a GUI element, or literally anything.

Next is the copy constructor. Now, the default copy constructor will mostly work for this class, so really the best thing you could do would be to not write a copy constructor at all - just let the compiler generate it.

That is... except for the fact that the default copy constructor would also copy waiting_time, which you don't seem to want.

So if you want to copy burst_time (and maybe process) but not waiting_time... then yes, you need a custom copy constructor.

However, there is no need to manually copy every item in a vector one-by-one. vector knows how to copy itself. So your copy constructor can just be:

Scheduling(const Scheduling& other) :
 burst_timeother.burst_time

 // nothing needed here

Whenever you manually define the copy constructor or assignment operator, you prevent automatic generation of the move operations... which is why you normally really don't want to do it. You'll probably want them back. You'll see why shortly.

The best way to do the special operations in a class is usually to define a swap function. If you want to make swapping part of the public interface - which you almost always do - you can make it a friend function. And you'll almost always want your swap function to be noexcept.

friend void swap(Scheduling& a, Scheduling& b) noexcept

 using std::swap;
 swap(a.burst_time, b.burst_time);
 // you can swap waiting_time too, if you want

Once you have a swap function, the move constructor is easy:

Scheduling(Scheduling&& other) noexcept

 using std::swap;
 swap(*this, other);

Move assignment is also easy:

Scheduling& operator=(Scheduling&& other) noexcept

 using std::swap;
 swap(*this, other);
 return *this;

But the real reason we want swap and the move operations is next.

Scheduling &operator=(const Scheduling &other)

 process = other.process;
 int size = other.burst_time.size();

 for (int i = 0; i < size; i++)
 
 burst_time.push_back(other.burst_time[i]);
 

 return *this;

Unfortunately, your copy assignment is buggy. The first problem is that it's not really copying the burst_time vector... it's appending to it.

In other words, if you have a Scheduling object s1 with burst_times "1, 2, 3", and s2 with burst_times "4, 5, 6", when you do s1 = s2;, you're not going to end up with both having the same value. s2 will remain unchanged, but s1 will now be "1, 2, 3, 4, 5, 6". That's probably not what you want.

To solve this, you probably want to clear the burst_time vector before doing the push_back()s.

The other problem is deeper. Each time you try to push_back, that might force the vector to reallocate... and that might throw an exception. If that happens midway through, you'll end up with a broken Schedule object - it will only be half-copied.

There is a standard technique used to solve this problem called copy-and-swap or copy-and-move. In fact, it would solve all your problems with this assignment operator. Here's how it looks:

Scheduling& operator=(const Scheduling& other)

 // First we copy other...
 auto temp = other;

 // If that fails in any way, no problem. temp is in an indeterminate
 // state... but who cares? It will be destroyed, and this object
 // hasn't been touched yet.

 // Next we move temp into *this (or we swap(temp, *this) - same thing)
 // Because we made the move operations noexcept, this cannot fail.
 *this = std::move(temp);

 // And we're done!

 return *this;

This technique prevents all problems that assignment can possibly have: it handles exceptions, it handles self-assignment, it handles everything. There is an efficiency cost, but it's often unavoidable, unfortunately.

So your copy assignment operator should almost always look like this:

Type& operator=(Type const& other)

 auto temp = other;

 *this = std::move(temp);
 // OR:
 // using std::swap;
 // swap(*this, temp);

 return *this;

And that means you should almost always write the move operations and swap, and make them noexcept.

std::vector<int> cal_waiting_time(std::vector<int>& burst_time)

 waiting_time.push_back(0);

 for (int i = 0; i < burst_time.size() - 1; i++)
 
 waiting_time.push_back(waiting_time[i] + burst_time[i]);
 
 return waiting_time;

This function might have a bug in it. If you call it twice, the waiting_time vector will be doubled in size. Are you sure you don't want to clear it first?

The other, bigger issue with this function is that it doesn't really seem to make sense as a member function as written. Here burst_time is a function parameter, which shadows burst_time in the class. Is that intentional? Even weirder, you use the class waiting_time member in the function... then promptly overwrite it with the return in print_table().

It seems to me that this function should not be in the class. It should be a free function outside. Or perhaps a static member function. Maybe even private. It depends on the interface you want.

So assuming it's not going to be a member function anymore, there are a few more things to fix.

The first is that you probably want to take the burst_time argument by const reference.

The second is that you don't handle the case where burst_time is empty. If it is, when you subtract 1 from the size (which is 0) and start your loop, you're going to trigger a spectacular crash - your program will attempt to read through gigabytes of random memory. (Though it will probably segfault and die fairly quickly, in reality.)

The third is another minor bug because you use int as the loop counter variable, but burst_time.size() returns an unsigned type. Comparing signed and unsigned types is dangerous. Your loop should be for (decltype(burst_time.size()) i = 0; i < burst_time.size(); ++i).

The final problem is a matter of efficiency. push_back() is cool and all, but each time you use it, you might be triggering a reallocation... which is expensive and slow. (In practice, you'll probably only get a reallocation every 2^n elements.) When you know how many elements are going to be in the vector, you can use reserve() to do all the allocation up front. This can be an enormous speed-up.

Putting that altogether gives:

// This function is NOT inside the class. It is a free function.
std::vector<int> cal_waiting_time(std::vector<int> const& burst_time)

 if (burst_time.empty())
 return ; // or maybe "return 0;"? that's up to you

 auto waiting_time = std::vector<int>;
 waiting_time.reserve(burst_time.size());

 waiting_time.push_back(0);

 for (decltype(burst_time.size()) i = 0; i < burst_time.size() - 1; i++)
 
 waiting_time.push_back(waiting_time[i] + burst_time[i]);
 

 return waiting_time;

Once you've done this, this raises the question of why you have waiting_time as a class data member. It doesn't seem to serve any purpose. And if you don't have waiting_time as a member, then there's no justification whatsoever for a custom copy constructor... or any of the other stuff we were forced to write because of that.

In other words, it seems like all you want for the class is this:

class Scheduling

 std::vector<int> burst_time;

 public:
 Scheduling() = default;

 explicit Scheduling(std::vector<int> burst_time_values) :
 burst_timestd::move(burst_time_values)
 
 // Nothing needed here.
 

 void print_table()
 
 auto waiting_time = cal_waiting_time(burst_time);
 std::cout << "Processt Burst Timet Waiting Timen";
 for (int i = 0; i < burst_time.size(); i++)
 
 std::cout << i+1 << "tt " << burst_time[i] << "tt " << waiting_time[i] << "n";
 
 

 // Automatic copy constructor, copy assignment, move constructor
 // move assignment, and destructor are all perfect.
;

Then you'd do your I/O separately into a vector<int>, maybe in main(), like this:

std::cout << "Enter number of processn";

int num;
std::cin >> num;
// Should do error checking!

auto burst_times = std::vector<int>;
burst_times.reserve(num);
for (int i = 0; i < n; i++)

 std::cout << "Enter Burst time for process " << i+1 << " : ";

 int val;
 std::cin >> val;
 // Again, error checking!

 burst_time.push_back(val);


auto batch1 = Schedulingstd::move(burst_times);
batch1.print_table();

auto batch2 = batch1;
batch2.print_table();

Anyway, the rest of the code is cool (though you should probably do some error checking when you read num in main().)

Summary

• Make any constructors that take only one argument explicit.


• Avoid unnecessary member functions and data members.


• If you implement either the copy constructor or copy assignment operator (or both), you almost always want to implement the move constructor, move operator, and swap functions. The move constructor, move operator, and swap should all be noexcept.


• When implementing the copy assignment operator, use the copy-and-swap or copy-and-move technique. (This requires noexcept move and/or swap operations. But you want those anyway.)


• Beware of shadowing variables with other variables with the same name.


• Watch out for signed/unsigned comparisons - use auto and decltype() to avoid problems.


• When you can know the size of a vector in advance, use reserve().


• You should check input operations for errors, because they happen often.

 int process;

 public:
 Scheduling() : process(0) 
 Scheduling(int n) : process(n) /* ... */ 

When initializing constant values for member data, prefer in-class member initializers.

Use reasonably descriptive names. For new users, is process a count? An id? They have to dig through the code to understand that it represents the number of processes stored. Do you need process? Can you get by with the size of burst_time?

 std::size_t process_count 0;

 public:
 Scheduling() = default;

 Scheduling(std::size_t number_of_processes) 
 : process_count (number_of_processes) 
 /* ... */ 

 std::vector<int> burst_time;
 std::vector<int> waiting_time;

burst_time and waiting_time would benefit from stronger typing. Prefer duration types over int.

 for (int i = 0; i < n; i++)
 
 std::cout << "Enter Burst time for process " << i+1 << " : ";
 int val;
 std::cin >> val;
 burst_time.push_back(val);
 

Compiling with warnings would have caught the signed/unsigned comparison mismatch.

You should check to see if reading from std::cin was successful.

 Scheduling(const Scheduling &other)
 
 process = other.process;
 int size = other.burst_time.size();

 for (int i = 0; i < size; i++)
 
 burst_time.push_back(other.burst_time[i]);
 
 

When you need to initialize a member variable with a non-constant value in a constructor, prefer initialization instead of assignment.

 Scheduling(const Scheduling &other)
 : processother.process
 , burst_timeother.burst_time
 , waiting_timeother.waiting_time
 

Since each member variable has a well-defined copy behavior, we can use the compiler-generated copy constructor.

 Scheduling(const Scheduling &) = default;

 Scheduling &operator=(const Scheduling &other)
 
 process = other.process;
 int size = other.burst_time.size();

 for (int i = 0; i < size; i++)
 
 burst_time.push_back(other.burst_time[i]);
 

 return *this;
 

Use const to define objects with values that do not change after it has been constructed.

 const int size = other.burst_time.size();

Your copy assignment is actually a copy append. You forgot to clear the existing list before copying.

Obey the rule of five/zero. In C++, there are six special member functions. Ignoring the default constructor, the rules cover the following member functions:

• Destructor


• Copy Constructor


• Move Constructor


• Copy Assignment Operator


• Move Assignment Operator

The rule of five is simple. If you define, =default, or =delete any of these five special member functions, you should consider how the other special member functions operate in the class.

The rule of five defaults goes a step further. If you define, =default, or =delete any of the five special member functions, then explicitly define, =default, or =delete them all.

 class Scheduling 
 ...
 public:
 Scheduling() = default;
 Scheduling(int);

 ~Scheduling() = default; 
 Scheduling(Scheduling const&) ... 
 Scheduling(Scheduling &&) = default; 
 Scheduling& operator=(Scheduling const&) ... 
 Scheduling& operator=(Scheduling &&) = default; 

 void print_table() ... 
 std::vector<int> cal_waiting_time(std::vector<int>) ... 
 ; 

The rule of zero is just as simple. If you can avoid defining all of the special members, then do so. As it turns out, the compiler generated behavior of the copy, move, and destruct operations are well-defined for your member variables and consistent in their use. You do not need to define these operations yourself.

 class Scheduling 
 ...
 public:
 Scheduling() = default;
 Scheduling(int) ... ;

 void print_table() const ... 
 std::vector<int> cal_waiting_time(std::vector<int>) ... 
 ;

 std::vector<int> cal_waiting_time(std::vector<int>& burst_time)
 
 waiting_time.push_back(0);

 for (int i = 0; i < burst_time.size() - 1; i++)
 
 waiting_time.push_back(waiting_time[i] + burst_time[i]);
 
 return waiting_time;
 

Know your algorithms. See std::partial_sum.

On multiple calls, the new results do not overwrite the old results. Instead, the new results are appended to the old results.

Did you intend for this function to be a part of the public api for this class? Have you considered returning a cached waiting_time rather than recalculating the partial sum on each call?

 void print_table()
 
 waiting_time = cal_waiting_time(burst_time);
 std::cout << "Processt Burst Timet Waiting Timen";
 for (int i = 0; i < burst_time.size(); i++)
 
 std::cout << i+1 << "tt " << burst_time[i] << "tt " << waiting_time[i] << "n";
 
 

This function isn't being truthful in what it does. It recalculates the waiting_time on every call which means its doing more than "print". Consider an alternative design, such as calculating the partial sum on construction.

I. std::vector has a more convenient interface actually:

//copy constructor
Scheduling(const Scheduling &other)
 : process(other.process), burst_time(other.burst_time) 

Same holds true for the copy-assignment.

II.

std::vector<int> cal_waiting_time(std::vector<int>& burst_time)

Here, you don't modify burst_time so it should be a const-ref:

std::vector<int> cal_waiting_time(const std::vector<int>& burst_time)

III.

void print_table()

Maybe adding a non-void return type, like Scheduling &, with return *this; as the last statement, can prove useful—or maybe not.