clean up the code

I've made some comments in your code for you to consider. I am not saying this is a definitive list of things, just what I spotted with a cursory look through.

void task_submit(void(*fun(int,int)),int,int);// <--- RX: I'd typedef the function pointer and use it here to make code simpler
void f1(int,int); // <--- RX: I'd choose more meaning func function names
void f2(int,int);
void f3(int,int);
void f4(int,int);
void f5(int,int);
void f6(int,int);
 
struct task1
{
	void (*sptr)(int, int); // <--- RX: Use type'def function pointer to make code clearer
	int sper;
	int spri;
}z1[6],z2[6]; // <--- RX: Use more meaningful variable names
 
 
struct task1 *buffer1;
struct task2 *buffer2;
 
 
void task_submit(void(*fun(int,int)),int per,int pri) // <--- RX: Use function poiner typedef here
{
	static int i;
	z1[i].sper=per;
	z1[i].spri=pri;
	z1[i].sptr=fun;
	i++;
}
 
 
 
main()
{
	static int a,b,c,value;
	buffer1=(struct task*)malloc(sizeof(z1)); // <--- RX: You never free these (memory leaks) -- are they ever even used?
	buffer2=(struct task*)malloc(sizeof(z2));
	task_submit(f1, 30000, 1); 
	task_submit(f2, 30000, 12);
	task_submit(f3, 30000, 3);
	task_submit(f4, 30000, 4);
	task_submit(f5, 60000, 5);
	task_submit(f6, 30000, 6);
 
	while(a<6) // <--- RX: This might be better as a for loop, also I don't see where a has been initialised
	{	
		if(z1[a].sper==30000)
		{
			z2[b].sptr=z1[a].sptr;
			z2[b].sper=z1[a].sper;
			z2[b].spri=z1[a].spri;
			b++;
		} 
		a++;	
	}  
 
 
	c=z2[0].spri;
	for(a=1;a<=b;a++) // <--- RX: Arrays start from 0 not 1, was this intended?
	{
		if(c<z2[a].spri)
		{
			c=z2[a].spri;
			value=a;
		} 		
	} 
	if(value==0)  // <--- RX: You could have avoided this if by using a ternary operator.  int idx = value?value:0; z2[idx].sptr(z2[idx].sper, z2[idx].spri);
	{
		z2[0].sptr(z2[0].sper, z2[0].spri);
	}
	else
	{
		z2[value].sptr(z2[value].sper, z2[value].spri);
	}
 
}
 
 
 // <--- RX: Why do you have 6 identical functions?
void f1(int per,int pri)
{
	printf("\n%d",pri); // <--- RX: You could do this with 1 printf
	printf("\n%d",per);
}
 
 
void f2(int per,int pri)
{
	printf("\n%d",pri);
	printf("\n%d",per);
}
 
void f3(int per,int pri)
{
	printf("\n%d",pri);
	printf("\n%d",per);
}
 
void f4(int per,int pri)
{
	printf("\n%d",pri);
	printf("\n%d",per);
}
 
 
void f5(int per,int pri)
{
	printf("\n%d",pri);
	printf("\n%d",per);
}
 
 
void f6(int per,int pri)
{
	printf("\n%d",pri);
	printf("\n%d",per);
}

Select allOpen in new window

And some more :


1)
>>         static int i;

Always initialize your variables :

        static int i = 0;

(same elsewhere)


2)
>> main()

main HAS to return int :

        int main(void) {
            /* ... */
            return 0;
        }


3) structs with trivial member types can simply be assigned, so :

>>                         z2[b].sptr=z1[a].sptr;
>>                         z2[b].sper=z1[a].sper;
>>                         z2[b].spri=z1[a].spri;

can be written as :

        z2[b] = z1[a];



4) To elaborate on this comment by evilrix :

>>         for(a=1;a<=b;a++) // <--- RX: Arrays start from 0 not 1, was this intended?

It's normal that it starts at 1, since you compare with the first element, but the upper limit should be a < b since b is one position PAST the last element.

can we modify above the code to to decrase the time process. Actually i am using two buffer's. I want to execute the task based on period(30000,60000,..) and based on the priority(1,4,3,2...........)... so in my program i used the just copying the existing buffer to 2nd buffer based on period(while loop)... after that i am searching for the priority (for loop and if condition)..... based on the priority i am intializing the first element of the 2ndbuffer.

Is this the better way do it or is there any other logic................

A priority queue seems to be what you're after.

If you're coding in C++, then the STL has one :

        http://www.cplusplus.com/reference/stl/priority_queue/

In C, you can write your own.

i am in confusion about your statement   can you explain some deeply

>> >>         static int i;
>> Always initialize your variables :
Agreed, although static variables are automatically initialized hence I didn't comment on this line specifically

Just to be clear (for Purna2010) this is what the C standard (ISO/IEC 9899:TC3) says: -

"If an object that has automatic storage duration is not initialized explicitly, its value is
indeterminate.

If an object that has static storage duration is not initialized explicitly, then:
 if it has pointer type, it is initialized to a null pointer;
 if it has arithmetic type, it is initialized to (positive or unsigned) zero;
 if it is an aggregate, every member is initialized (recursively) according to these rules;
 if it is a union, the first named member is initialized (recursively) according to these rules."

Do you know what a priority queue is ? It is a data structure that imo fulfills your requirements.

        http://en.wikipedia.org/wiki/Priority_queue

>> Agreed,

It's just nicer to do it explicitly :)

Btw, which reminds me, why did you make the variables in main static ? Unless you plan on calling main recursively, that doesn't really serve a purpose.

>> i am in confusion about your statement   can you explain some deeply
The priority queue concept is a design pattern that ensures the largest (or smallest) number is always in the top. It is normally (but not always) implemented as a max heap.
http://en.wikipedia.org/wiki/Heap_(data_structure)

ya i know the priority queue.

The above operation what i did is finding highest priority and making that as first element.  

I am asking whether it is better logic or not.  I don't think it as better logic that's why  i posted here

A priority queue formalizes the concept into a data structure. Not only will that make your code easier to read/understand, it will also abstract all the sorting etc. out for you, so you don't have to worry about that. Simply add events to the priority queue, and get the next one out of it whenever you need it.

Btw, I just noticed that the last if statement is a bit unnecessary. This :

>>         if(value==0)
>>         {
>>                 z2[0].sptr(z2[0].sper, z2[0].spri);
>>         }
>>         else
>>         {
>>                 z2[value].sptr(z2[value].sper, z2[value].spri);
>>         }

can easily be replaced with :

        z2[value].sptr(z2[value].sper, z2[value].spri);

>>>>I don't think it as better logic that's why  i posted here

As far as the priority table was fix, a better code would be

   f1(30000, 1);

Assuming you want the table to be retrieved from somewhere outside, then you should sort the table and invoke the first entry (if any) after sorting. That would be better code as you have now cause your current code needs two loops and a second table to find the minimum entry what easily could be achieved by a single loop as well. Furthermore your loops seem to operate on some assumptions (e. g. that the tasks with  sper=30000 have the highest priority) what is rarely a good design. So, if you have C++ and can use STL use the priority_queue evilrix has suggested. If you are stuck on C code or can't use the STL use a loop like

                b = 0;
      for (a = 1; a < (int)sizeof(z1)/sizeof(z1[0]); ++a)
      {      
                        if(z1[a].sper < z1[b].sper ||
                           (z1[a].sper == z1[b].sper && z1[a].spri < z1[b].spri ))
                            b = a;
      }  
                // invoke minimum
                z1[b].sptr(z1[b].sper, z1[b].spri);

Note, after that you most probably have to remove the entry from the queue, but I can't see a way how you would decrement the static counter in task_submit. Generally a queue hardly can be made with a fixed-sized array and a one-time initialiazation. You either need a dynamic array which can be increased when necessary or you use a big-sized array which was supposed to be able to take all reasonably amounts of tasks (and would exit with error if there is an overflow nevertheless).

In C++ you would make a manager class (singleton) to handle the queue (add, remove, get_next). In C you either would have global variables and functions which handle these variables or you have on single function which holds the queue and counters as static data and can make add, remove, pop_next as different operations).

Actually i didn't implemented the entry removing part.... Can you give me suggestion on how to remove that

>>>> Can you give me suggestion on how to remove that

Assuming you want to do it with a static table in a global function:

const int MAX_TASKS = 10;

typedef void (*SPTR)(int, int);

enum { TASK_ADD, TASK_SEL, TASK_NEXT };

void managePriorityTable(int opcode, SPTR& sptr, int& per, int& prio)
{
     static task1 priorityTable[MAX_TASKS] = { 0 };
     static taskCount = 0;

     switch(opcode)
     {
        case  TASK_ADD:
             if (taskCount +1 >= MAX_TASKS)
             {
                   cout << "too many tasks" << endl;
                   abort();
              }
              task1 t = {sptr, per, prio};    
              priorityTable[taskCount++] = t ;
              break;
        case  TASK_DEL:
              memmove(&priorityTable, &priorityTable[1], sizeof(task1)*(--taskCount);
              task1 t = {NULL, 0, 0};    
              priorityTable[taskCount] = t ;
              break;
        case TASK_NEXT:
           {
              // bubble sort
              for (int i = 0; i < taskCount - 1; ++i)
                   for (int n = i + 1; n < taskCount; ++ n)
                   {
                       if (priorityTable[i].sper > priorityTable[n].sper ||
                           (priorityTable[i].sper == priorityTable[n].sper &&  priorityTable[i].spri > priorityTable[n].spri))
                       {
                           task1 t = priorityTable[i];
                           priorityTable[i] = priorityTable[n];
                           priorityTable[n] = t;
                        }
                   }
             }
             sptr  =  priorityTable[0].sptr;
             per   =  priorityTable[0].sper;
             prio  =  priorityTable[0].spri;
         }
    }    
}

>> Can you give me suggestion on how to remove that

What do you mean ? Are you planning to implement your own priority queue ? If so, you probably shouldn't do it the way you started out, but make a proper data structure with associated methods for operating on it.

>> Assuming you want to do it with a static table in a global function:

Why make it so complicated ? There's no reason to put everything into one function, is there ?

>>>> Why make it so complicated ? There's no reason to put everything into one function, is there ?
It is an approach which has the advantage that the container is encapsulated in one single function. One single function to handle all operations of a container isn't complicated and avoids the usage of global variables. It is the best after putting all into a class (and is even simpler than that). I took that approach cause because Purna's task_submit  had a similar concept.

>> because Purna's task_submit  had a similar concept.

I know that. My question was more directed to Purna2010, rather than you ;)

The problem with doing it this way, is that you can have only one instance (actually, you WILL have one instance whether you want to or not), so you're not implementing a generic priority queue.

>>>> so you're not implementing a generic priority queue.
Yes, but that doesn't seem to be a requirement. You called it a 'complicated' solution but actually it isn't.

>> You called it a 'complicated' solution

I called it complicated due to the need of extra parameters, the need for a switch to handle different actions, etc.

It is not how I would do this, unless there is no other way (due to API constraints for example). Having a proper data structure would be less limiting (not just the one instance for example), allow for clearer code (separate functions for separate actions - also handy when debugging), faster (no overhead for extra parameters, switches, etc.) - just to name a few.
I really can't see an advantage to doing it this way ...

I see you accepted just the last post. Does that mean the other posts weren't helpful to you ?

>> I see you accepted just the last post. Does that mean the other posts weren't helpful to you ?
I too noticed that. I was somewhat surprised since it seems to me a number of posts, including my own, provided useful info in this thread.