Return to Answer

• Have I understood how merge sort works and implemented it in the right way?

You understood the algorithm correctly, and your code yields the desired results.

• Have I used any bad practices?
• How could the code be improved upon in terms of efficiency?

See my further points below please:

1. Don't use using namespace std;

While that would work in your particular case, it's considered bad practice. Especially when you move out your code to separate header files.

See more details here please:

Why is "using namespace std;" considered bad practice?

2. Check constraints in order

This code may call undefined behavior, if the constraints aren't checked first (logical boolean arithmetic operations are executed in order):

if(firstHalf.back() > secondHalf.back() && !firstHalf.empty()){

Since there's the possibility calling std::vector::back() with an empty vector, that code should be

if(!firstHalf.empty() && !secondHalf.empty() && firstHalf.back() > secondHalf.back()){

to avoid calling the offensive statements

3. Simplify your data input processing

Your data input function can be simplified a lot. You don't need another split() function to do this. Simply use a std::istringstream to do this:

vector<float> floatVectorInput(){
 string inputString;
 getline(cin, inputString);
 vector<float> array;
 std::istringstream iss(inputString);
 float val;
 while(iss >> val){
 array.push_back(val);
 }
 return array; 
}

4. Prefer loops and dynamically allocated stacks over recursion

Recursive function calls like

return merge(mergeSort(firstHalf), mergeSort(secondHalf));

always are limited to the (OS defined) stack size regarding the call stack depth.

On large input lists this may bail out with a Stack Overflow error.

You can simply replace that with a std::stack<std::pair<std::vector<float>,std::vector<float>>> structure that is handled within a loop.

• Have I understood how merge sort works and implemented it in the right way?

You understood the algorithm correctly, and your code yields the desired results.

• Have I used any bad practices?
• How could the code be improved upon in terms of efficiency?

See my further points below please:

1. Don't use using namespace std;

While that would work in your particular case, it's considered bad practice. Especially when you move out your code to separate header files.

See more details here please:

Why is "using namespace std;" considered bad practice?

2. Check constraints in order

This code may call undefined behavior, if the constraints aren't checked first (logical boolean arithmetic operations are executed in order):

if(firstHalf.back() > secondHalf.back() && !firstHalf.empty()){

Since there's the possibility calling std::vector::back() with an empty vector, that code should be

if(!firstHalf.empty() && !secondHalf.empty() && firstHalf.back() > secondHalf.back()){

to avoid calling the offensive statements

3. Simplify your data input processing

Your data input function can be simplified a lot. You don't need another split() function to do this. Simply use a std::istringstream to do this:

vector<float> floatVectorInput(){
 string inputString;
 getline(cin, inputString);
 vector<float> array;
 std::istringstream iss(inputString);
 float val;
 while(iss >> val){
 array.push_back(val);
 }
 return array; 
}

4. Prefer loops and dynamically allocated stacks over recursion

Recursive function calls like

return merge(mergeSort(firstHalf), mergeSort(secondHalf));

always are limited to the (OS defined) stack size regarding the call stack depth.

On large input lists this may bail out with a Stack Overflow error.

You can simply replace that with a std::stack<std::pair<std::vector<float>,std::vector<float>>> structure that is handled within a loop.

• Have I understood how merge sort works and implemented it in the right way?

You understood the algorithm correctly, and your code yields the desired results.

• Have I used any bad practices?
• How could the code be improved upon in terms of efficiency?

See my further points below please:

1. Don't use using namespace std;

While that would work in your particular case, it's considered bad practice. Especially when you move out your code to separate header files.

See more details here please:

Why is "using namespace std;" considered bad practice?

2. Pass parameters that won't be changed as const reference

You are passing the vector parameters by value here (and at other places):

vector<float> merge(vector<float> firstHalf, vector<float> secondHalf){

It would be better to pass these as const reference parameters:

vector<float> merge(const vector<float>& firstHalf, const vector<float>& secondHalf){

Even if passing by value would work properly, it makes the function signature clearer for the caller semantically and may be more efficient.

3. Check constraints in order

This code may call undefined behavior, if the constraints aren't checked first (logical boolean arithmetic operations are executed in order):

if(firstHalf.back() > secondHalf.back() && !firstHalf.empty()){

Since there's the possibility calling std::vector::back() with an empty vector, that code should be

if(!firstHalf.empty() && !secondHalf.empty() && firstHalf.back() > secondHalf.back()){

to avoid calling the offensive statements

4. Simplify your data input processing

Your data input function can be simplified a lot. You don't need another split() function to do this. Simply use a std::istringstream to do this:

vector<float> floatVectorInput(){
 string inputString;
 getline(cin, inputString);
 vector<float> array;
 std::istringstream iss(inputString);
 float val;
 while(iss >> val){
 array.push_back(val);
 }
 return array; 
}

5. Prefer loops and dynamically allocated stacks over recursion

Recursive function calls like

return merge(mergeSort(firstHalf), mergeSort(secondHalf));

always are limited to the (OS defined) stack size regarding the call stack depth.

On large input lists this may bail out with a Stack Overflow error.

You can simply replace that with a std::stack<std::pair<std::vector<float>,std::vector<float>>> structure that is handled within a loop.

• Have I understood how merge sort works and implemented it in the right way?

You understood the algorithm correctly, and your code yields the desired results.

• Have I used any bad practices?
• How could the code be improved upon in terms of efficiency?

See my further points below please:

1. Don't use using namespace std;

While that would work in your particular case, it's considered bad practice. Especially when you move out your code to separate header files.

See more details here please:

Why is "using namespace std;" considered bad practice?

2. Check constraints in order

This code may call undefined behavior, if the constraints aren't checked first (logical boolean arithmetic operations are executed in order):

if(firstHalf.back() > secondHalf.back() && !firstHalf.empty()){

Since there's the possibility calling std::vector::back() with an empty vector, that code should be

if(!firstHalf.empty() && !secondHalf.empty() && firstHalf.back() > secondHalf.back()){

to avoid calling the offensive statements

3. Simplify your data input processing

Your data input function can be simplified a lot. You don't need another split() function to do this. Simply use a std::istringstream to do this:

vector<float> floatVectorInput(){
 string inputString;
 getline(cin, inputString);
 vector<float> array;
 std::istringstream iss(inputString);
 float val;
 while(iss >> val){
 array.push_back(val);
 }
 return array; 
}

4. Prefer loops and dynamically allocated stacks over recursion

Recursive function calls like

return merge(mergeSort(firstHalf), mergeSort(secondHalf));

always are limited to the (OS defined) stack size regarding the call stack depth.

On large input lists this may bail out with a Stack Overflow error.

You can simply replace that with a std::stack<std::pair<std::vector<float>,std::vector<float>>> structure that is handled within a loop.

1. Don't use using namespace std;

While that would work in your particular case, it's considered bad practice. Especially when you move out your code to separate header files.

See more details here please:

Why is "using namespace std;" considered bad practice?

2. Prefer passing parameters that won't be changed as const reference

You are passing the vector parameters by value here (and at other places):

vector<float> merge(vector<float> firstHalf, vector<float> secondHalf){

It would be better to pass these as const reference parameters:

vector<float> merge(const vector<float>& firstHalf, const vector<float>& secondHalf){

Even if passing by value would work properly, it makes the function signature clearer for the caller semantically and may be more efficient.

3. Check constraints in order

This code may call undefined behavior, if the constraints aren't checked first (logical boolean arithmetic operations are executed in order):

if(firstHalf.back() > secondHalf.back() && !firstHalf.empty()){

Since there's the possibility calling std::vector::back() with an empty vector, that code should be

if(!firstHalf.empty() && !secondHalf.empty() && firstHalf.back() > secondHalf.back()){

to avoid calling the offensive statements

4. Simplify your data input processing

Your data input function can be simplified a lot. You don't need another split() function to do this. Simply use a std::istringstream to do this:

vector<float> floatVectorInput(){
 string inputString;
 getline(cin, inputString);
 vector<float> array;
 std::istringstream iss(inputString);
 float val;
 while(iss >> val){
 array.push_back(val);
 }
 return array; 
}

• Have I understood how merge sort works and implemented it in the right way?

You understood the algorithm correctly, and your code yields the desired results.

• Have I used any bad practices?
• How could the code be improved upon in terms of efficiency?

See my further points below please:

1. Don't use using namespace std;

While that would work in your particular case, it's considered bad practice. Especially when you move out your code to separate header files.

See more details here please:

Why is "using namespace std;" considered bad practice?

2. Pass parameters that won't be changed as const reference

You are passing the vector parameters by value here (and at other places):

vector<float> merge(vector<float> firstHalf, vector<float> secondHalf){

It would be better to pass these as const reference parameters:

vector<float> merge(const vector<float>& firstHalf, const vector<float>& secondHalf){

Even if passing by value would work properly, it makes the function signature clearer for the caller semantically and may be more efficient.

3. Check constraints in order

This code may call undefined behavior, if the constraints aren't checked first (logical boolean arithmetic operations are executed in order):

if(firstHalf.back() > secondHalf.back() && !firstHalf.empty()){

Since there's the possibility calling std::vector::back() with an empty vector, that code should be

if(!firstHalf.empty() && !secondHalf.empty() && firstHalf.back() > secondHalf.back()){

to avoid calling the offensive statements

4. Simplify your data input processing

Your data input function can be simplified a lot. You don't need another split() function to do this. Simply use a std::istringstream to do this:

vector<float> floatVectorInput(){
 string inputString;
 getline(cin, inputString);
 vector<float> array;
 std::istringstream iss(inputString);
 float val;
 while(iss >> val){
 array.push_back(val);
 }
 return array; 
}

5. Prefer loops and dynamically allocated stacks over recursion

Recursive function calls like

return merge(mergeSort(firstHalf), mergeSort(secondHalf));

always are limited to the (OS defined) stack size regarding the call stack depth.

On large input lists this may bail out with a Stack Overflow error.

You can simply replace that with a std::stack<std::pair<std::vector<float>,std::vector<float>>> structure that is handled within a loop.