I was rated 5 out of 10 for this code

Question

Any feedback for this code of odd even printing using two threads?

I discussed initially and got feedback that it is only worth of 5 out of 10.

Feel free to share your suggestions to improve this code.
https://sololearn.com/compiler-playground/cY6n1J39OmTj/?ref=app

Answer 1

it feels over-engineered for such a simple task. But for demonstrating thread synchronization, i think it's ok..

maybe it was rated by people who were taught using namespace std is bad. I have no problem with it. For small demo scripts, I feel it reduces the visual clutter and improves readability. Just as long as one is aware that it's a footgun for bigger codebases.

as for it being functional, it's actually a plus for me. OOP is overused and adds a lot of unnecessary boilerplate. Functional and imperative styles go straight for the problem.

Answer 2

Reported michs23 for spamming. Reasons: New user + invalid analysis of the code + unrelated link + fake website = spam bot. (The real NJ DMV is at https://www.nj.gov/mvc/).

Answer 3

Brian
i was wondering what the race conditions comment was about... it was clearly being addressed to in the code.
If this is a bot, it's context recognition is scarily getting better. It can now recognize the topic is about multi threading

Answer 4

What's the criteria?
Improve in which direction?

Answer 5

Any point is fine. Code readability, code quality, performance enhancement etc.

One point I got is the functional paradigm and global variables. It could have been avoided

Answer 6

Hard to give feedback without knowing what the goal/grading criteria were.

You mentioned global variables counting against you.
Possibly declare and initialize your 3 global variables inside main() and then pass them as arguments to your 2 functions, like you did with maxNum.

Answer 7

Bob_Li yes, it is a concerning level of sophistication, like they are testing for weakness and continually revising to become undetectable. I didn't click on the link, as I assume it is for phishing or installing malware.

Answer 8

I think that the problem here is that there are much simpler ways to do it.
But whoever rated you is an idiot. Because with this, you proved that you can handle complex tasks. So the code is great! Keep up the good work!

Answer 9

Ok. Will change it. Regarding goal of the exercise, it's like trying to implement a better code for printing odd and even numbers with thread synchronization

Answer 10

An upgraded version of your code now exhibits efficient mutex control along with lower contention while also providing more distinct output. The program becomes more efficient when the application releases the mutex lock first and iterates maxNum through reference. Hope this helps!
#include <iostream>
#include <mutex>
#include <condition_variable>
#include <thread>

using namespace std;

mutex m;
condition_variable cv;
bool isOddTurn = true;

void printOdd(const int& maxNum) {
for (int i = 1; i <= maxNum; i += 2) {
unique_lock<mutex> ul(m);
cv.wait(ul, []() { return isOddTurn; });

cout << "Odd Thread " << this_thread::get_id() << ": " << i << endl;

isOddTurn = false;
ul.unlock();
cv.notify_one();
}
}

void printEven(const int& maxNum) {
for (int i = 2; i <= maxNum; i += 2) {
unique_lock<mutex> ul(m);
cv.wait(ul, []() { return !isOddTurn; });

cout << "Even Thread " << this_thread::get_id() << ": " << i << endl;

isOddTurn = true;
ul.unlock();
cv.notify_one();
}
}

int main() {
int maxNum = 20;
thread odd(printOdd, cref(maxNum));
thread even(printEven, cref(maxNum));

odd.join();
even.join();

return 0;
}

Answer 11

MD SHAREK I could figure out that you passed input to thread function as const ref rather than copy . Am I missing something else ?

As int is basic data type and also don't get copied more than once per thread, it should not be a more overhead. Also , most of the times, people suggest not to pass const ref for int and copy constructor will be enough.

This is good point, but may be not more contributing to the performance. Right? Or anything else also which I am missing

Answer 12

not sure if this is right, but here is a no cv, no mutex, no flag code.

ok, no thread synchronisation, too. 😅
but the array does the data organization.

why use threads if you're just going to make them wait unnecessarily? the more you constrain them, the less advantage you get from them.

avoid complexity and simplify whenever possible.

https://sololearn.com/compiler-playground/cqaNceA1p2mU/?ref=app

Answer 13

Sounds good but not sure Bob_Li about data race.

Is it sure that no data race even with write acces at different index on same vector to be updated by two threads??

Answer 14

https://stackoverflow.com/questions/55647722/does-c-guarantees-it-is-safe-to-access-adjacent-elements-of-an-array-from-two

Answer 15

Thanks Bob_Li . Great to know this about object members and index of array as thread safe.

Answer 16

Ok thank you

Answer 17

welcome

Answer 18

Ketan Lalcheta
it's an array, not a vector. The way it's set up, the two threads are not overwriting each other.

the threads each outputting to cout is where the craziness happens. avoid that and things become simpler.

Answer 19

Still is it thread safe ? Array is same and not sure which index might be updated to compiler. If compiler knows this, it is smart .

I feel it is two right operation by two threads, but not sure