current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 0 down vote favorite

I must write code for my classes which simulates multi-layers production line. But I don't even know how to check if it's good. Can anyone take a look at my code, please? First machine works in random frequency putting integer into first queue. Then, second machine puts out this number at freq2, adding 100 and so on...

#include <stdio.h>
#include <iostream>
#include <queue>
#include <time.h>
#include <cstdlib>

using namespace std;

int main()
{
queue<int> k1;
queue<int> k2;
queue<int> k3;
int max1=5;
int max2=10;
int max3=2;
int freq1=5;
int freq2=3;
int freq3=4;


int loop=0;
float interwal=0.5;

while (1)
{
loop++;
int tmp=rand()%100;
int tmp2=rand()%5+1;
if (loop%tmp2==0 && k1.size()<max1) k1.push(tmp);
if (k1.empty()==false && k2.size()<max2 && loop%freq1==0)
{
    k2.push(k1.front()+100);
    k1.pop();
}
if (k2.empty()==false && k3.size()<max3 && loop%freq2==0)
{
    k3.push(k2.front()*100);
    k2.pop();
}
if (k3.empty()==false && loop%freq3==0)
{
    cout<<k3.front()<<endl;
    k3.pop();
}

clock_t ticks, ticks2;
ticks=clock();
ticks2=ticks;
while((ticks2/(float)CLOCKS_PER_SEC-ticks/(float)CLOCKS_PER_SEC)<interwal)
    ticks2=clock();
}
}
share|improve this question
1  
Does it work already? And you're looking to improve it? It may help to include what the exercise is as well as comment the code so it's easier to follow. – Raystafarian Mar 8 at 19:37
    
@Raystafarian The OP does not state that the code does not work; do you have reason to believe it is? The OP also states that they are looking to improve their code; do you believe otherwise? – SirPython Mar 8 at 23:52
    
@SirPython nope - "check if its good" was just the reason I asked if it worked already, as you know that's a requirement and it's best to clarify up front. I didn't intend to imply it doesn't work. – Raystafarian Mar 9 at 9:28
up vote 1 down vote

A lot here depends on how you define "good". On one hand, your code is fairly simple and straightforward. There are a few issues with readability:

  1. indentation should be improved
  2. if (x == false) should be written as if (!x)
  3. Some of the names could be improved (e.g., k1, tmp, tmp2 are all pretty meaningless).

Those are pretty minor though.

Perhaps more importantly, it's not immediately clear how much of the code is intended to correspond to a particular part of the overall job. I'd tend to separate each sub-task into its own function, with its input and output queues as parameters to that function, so main would look more like:

while (1) { 
    generate(loop, k1);
    process1(loop, k1, k2);
    process2(loop, k2, k3);
    output(loop, k3);
    wait();
}

A still larger problem is that your code busy-waits to get the timing correct. This wastes a lot of CPU time (and on portable devices, a lot of battery power). At the very least, you probably want to look up std::sleep_for, which will make your waiting for the correct number of ticks quite a bit simpler.

The next step after that (though it's not clear to me that it's justified) would be for each process to execute as a separate thread, with thread-safe queues between the threads.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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