1
\$\begingroup\$

I have written code to print a range of numbers using multi-threaded programming. It takes in the number of threads to spawn as input.

It is working fine and giving the expected results. I just wanted you to check it and see if the code can be written a bit more efficiently.

 #include<stdio.h>
 #include <windows.h>


 void print(LPVOID );


 struct range
 {
  int start;
  int end;
  int threadnum;
 };


   void print(LPVOID param)
   {

     struct range *ptr=(struct range *)(param);
     for(int i=ptr->start;i<=ptr->end;i++)
     printf("\n Thread %d printed %d",ptr->threadnum,i);

   }


 int main()
 {

         int start,end,range,split;
         int threadcount=0;

         printf("\n Enter the start range :");
         scanf_s(" %d",&start);

         printf("\n Enter the End Range : ");
         scanf_s("%d",&end);

         printf("\n Enter the number of threads : ");
         scanf_s("%d",&threadcount);

         struct range **ptr;

         ptr = (struct range **)malloc(threadcount*sizeof(struct range *));

         if(ptr==NULL)
         {
           printf("\n Could not allocate memory for struct range * pointer array ");
           free(ptr);
           return 1;
         }

         DWORD  *ThreadId;
         ThreadId=(DWORD *)malloc(threadcount*sizeof(DWORD));
         if(ThreadId==NULL)
         { 
            printf("\n Could not allocate memory for DWORD * pointer array ");
            free(ThreadId);
            return 1;
         }

         HANDLE  *hThreadArray;
         hThreadArray=(HANDLE *)malloc(threadcount*sizeof(HANDLE));
         if(hThreadArray==NULL)
         { 
            printf("\n Could not allocate memory for hThreadArray * pointer array ");
            free(hThreadArray);
            return 1;
         }

         range=end-start+1;

         split=(range/threadcount);

         for(int i=0;i<threadcount;i++)
         {
                    ptr[i]=(struct range *)malloc(sizeof(struct range));
        if(ptr[i]==NULL)
        {
            free(ptr[i]);
        }
        if(split%2==0) // if the split is even then calculated the start    and end range for the worker threads as below
        {

         if(i==0)
         {
            start=start;
            end=start+split-1;
            ptr[i]->start=start;
            ptr[i]->end=end;
            ptr[i]->threadnum=i;


           hThreadArray[i] = CreateThread(NULL,0,(LPTHREAD_START_ROUTINE)print,ptr[i],0,&ThreadId[i]);
           if(hThreadArray[i]==NULL)
           { 
              CloseHandle(hThreadArray[i]);
           }


         }
         else
         {
            start=end+1;
            end=start+split-1;
            ptr[i]->start=start;
            ptr[i]->end=end;
            ptr[i]->threadnum=i;
            hThreadArray[i] = CreateThread(NULL,0,(LPTHREAD_START_ROUTINE)print,ptr[i],0,&ThreadId[i]);
            if(hThreadArray[i]==NULL)
            { 
              CloseHandle(hThreadArray[i]);
            }
         }
    }
        else
        {
            if(i==0)
            {
             start=start;
             end=start+split;
             ptr[i]->start=start;
             ptr[i]->end=end;
             ptr[i]->threadnum=i;


             hThreadArray[i] = CreateThread(NULL,0,(LPTHREAD_START_ROUTINE)print,ptr[i],0,&ThreadId[i]);
             if(hThreadArray[i]==NULL)
             { 
              CloseHandle(hThreadArray[i]);
             }


         }
          else
          {
             start=end+1;
             end=start+split-1;
             ptr[i]->start=start;
             ptr[i]->end=end;
             ptr[i]->threadnum=i;
             hThreadArray[i] = CreateThread(NULL,0,(LPTHREAD_START_ROUTINE)print,ptr[i],0,&ThreadId[i]);
             if(hThreadArray[i]==NULL)
             { 
               CloseHandle(hThreadArray[i]);
             }
         }
     }
}

   WaitForMultipleObjects(threadcount, hThreadArray, TRUE, INFINITE);

   for(int i=0; i<threadcount; i++)
   {
      CloseHandle(hThreadArray[i]);
      free(ptr[i]);
   }

 }
\$\endgroup\$
1

1 Answer 1

3
\$\begingroup\$
  • First of all, while reviewing your code the bad code formatting style really hurts my eyes. Good code formatting and indentation makes it easier to review and maintain code.
  • Get rid of all unnecessary spaces and tabs.
  • Your main function is doing many things at once. Try to break the logic in smaller modules and write function for them.
  • Define your structure into one type using typedef. The code below

    struct range
    {
     int start;
     int end;
     int threadnum;
    };
    

    should be like

    typedef struct range
    {
     int start;
     int end;
     int threadnum;
    }Range;
    

    So you can just use Range instead of struct range everytime.

    Then

    struct range *ptr=(struct range *)(param);
    

    will be something like

    Range *ptr = (Range *)(param);
    
  • Keep one space both side of operators (=,==).

  • Your main function should return EXIT_SUCCESS or return 0.
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Good points. About casting you should also read the link in my comment on the question. \$\endgroup\$ Commented Jul 28, 2013 at 16:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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