Thread: Help optimizing a code !!

  1. #1
    Registered User
    Join Date
    Sep 2014
    Posts
    51

    Help optimizing a code !!

    Hey, i was trying to solve this problem Painting | CodeChef. I got a Time limit exceeded error because of the poor sorting algorithm implemented.

    There are two arrays C[] and T[], where C[] holds the cost for a number of cells T[]. Both these array have a one-to-one relationship.
    The sorting I intend to perform is to sort the array C[] in the increasing order, while simultaneously moving the corresponding value in array T[].
    Code:
    #include<stdio.h>
    #define min(a,b) a<b?a:b
    int main()
    {
        long long int N,M;
        int H,j;
        scanf("%lld %lld %d",&N,&M,&H);
        long long int toPaint = N*M;
        long long int ans =0;
        long long int canPaint;
        long long int T[H];
        unsigned long long int C[H]; int i;
        for(i=0;i<H;i++)
        {
            scanf("%lld %lld",&T[i],&C[i]);
        }
    // Sorting the arrays C[] and T[]. Results in a time limit exceeded error.
      for(i=0;i<H;i++)
        {
            for(j=i+1;j<H;j++)
            {
                if(C[i]>C[j])
                {
                    long long temp;
                    temp = T[i];
                    T[i] = T[j];
                    T[j] =temp;
                    long long temp1;
                    temp1 = C[i];
                    C[i] = C[j];
                    C[j] =temp1;
                }
            }
        }
        for(i=0;i<H;i++)
        {
            canPaint = min(toPaint,T[i]);
            ans += canPaint *C[i];
            toPaint -=canPaint;
        }
        if(toPaint >0)
            printf("Impossible\n");
        else
            printf("%lld\n",ans);
        return 0;
    }
    Can someone provide some other better alternative to implement sorting of the arrays. Thanks

  2. #2
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    28,413
    Quote Originally Posted by thebenman
    There are two arrays C[] and T[], where C[] holds the cost for a number of cells T[]. Both these array have a one-to-one relationship.
    The sorting I intend to perform is to sort the array C[] in the increasing order, while simultaneously moving the corresponding value in array T[].
    Perhaps you should define a struct that combines each corresponding element of C and T, then have a single array of these struct objects that you can sort using qsort.
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  3. #3
    Registered User
    Join Date
    Sep 2014
    Posts
    51
    Quote Originally Posted by laserlight View Post
    Perhaps you should define a struct that combines each corresponding element of C and T, then have a single array of these struct objects that you can sort using qsort.
    I have added a structure to hold the values of C[] and T[].
    Code:
    #include<stdio.h>
    #include<stdlib.h>
    #define min(a,b) a<b?a:b
    struct values
    {
        unsigned long long int C;
        unsigned long long int T;
    };
    int compare(struct values *ptr,struct values *ptr1)
    {
        if(ptr->C < ptr1->C)
            return -1;
        else
            if(ptr->C > ptr1->C)
            return 1;
        else
            return 0;
    }
    
    
    int main()
    {
        long long int N,M;
        int H,j;
        scanf("%lld %lld %d",&N,&M,&H);
        long long int toPaint = N*M;
        long long int ans =0;
        long long int canPaint;
        struct values *ptr[H];
        int i;
        for(i=0;i<H;i++)
        {
            ptr[i] = (struct values *)malloc(sizeof(struct values));
            scanf("%lld %lld",&ptr[i]->T,&ptr[i]->C);
        }
        qsort((void *)&ptr,H,sizeof(struct values),compare);
        for(i=0;i<H;i++)
        {
            canPaint = min(toPaint,ptr[i]->T);
            ans += canPaint *ptr[i]->C;
            toPaint -=canPaint;
        }
        if(toPaint >0)
            printf("Impossible\n");
        else
            printf("%lld\n",ans);
        return 0;
    }
    However the compiler shows an warning !!

    D:\Bennet\Codeblocks C\CodeChef CFiles\PNTNG.c||In function 'main':|
    D:\Bennet\Codeblocks C\CodeChef CFiles\PNTNG.c|35|warning: passing argument 4 of 'qsort' from incompatible pointer type [enabled by default]|
    c:\program files\codeblocks\mingw\include\stdlib.h|371|note: expected 'int (*)(const void *, const void *)' but argument is of type 'int (*)(struct values *, struct values *)'|
    ||=== Build finished: 0 error(s), 1 warning(s) (0 minute(s), 0 second(s)) ===|


    Could you tell me if this is the correct approach to do the task using a structure?

  4. #4
    Registered User Al3's Avatar
    Join Date
    Nov 2014
    Posts
    135
    You can't typecast to void*

  5. #5
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    28,413
    This is because qsort expects the comparator to have this declaration:
    Code:
    int compare(const void *x, const void *y);
    What you can do is to write your comparator like this:
    Code:
    int compare(const void *x, const void *y)
    {
        const struct values *ptr = x;
        const struct values *ptr1 = y;
        if (ptr->C < ptr1->C)
            return -1;
        else if (ptr->C > ptr1->C)
            return 1;
        else
            return 0;
    }
    EDIT:
    I am puzzled as to why you are using dynamic allocation when you used a variable length array previously. ptr in main should be an array (possibly variable length array) of struct values objects, or a pointer to a dynamic array of struct values objects. If you want it to be an array of pointers instead, then your comparator must account for this.

    Note that conversions from pointers to objects to pointers to void and vice versa happen implicitly. There is no need to typecast (except in the special case when you have a conversion from void* to a pointer to object type and need your code to be compilable as C++, but that presumably does not apply here).
    Last edited by laserlight; 11-24-2014 at 01:29 PM.
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  6. #6
    Registered User
    Join Date
    Sep 2014
    Posts
    51
    DO you mean the dynamic allocation made here?
    Code:
    ptr[i] = (struct values *)malloc(sizeof(struct values));
    Is the qsort() function call correct? The program crashes when the input is anything other than impossible
    Code:
        qsort(ptr,H,sizeof(struct values),compare);

  7. #7
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    28,413
    Yes, that's part of the problem.

    Your qsort call is wrong because you are trying to use it to sort an array of struct values, but in fact you have an array of pointers to struct values. But... I think that what is actually wrong is your array, not your qsort call.
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  8. #8
    Registered User
    Join Date
    Sep 2014
    Posts
    51
    I made the following change in accepting the values for the structure.
    However this doesn't seem to work as well, when i try to print the values of the structure I had entered.

    Code:
    struct values *ptr[H];
        int i;
        for(i=0;i<H;i++)
        {
            ptr[i] = (struct values*)malloc(sizeof(*ptr[i]));
            scanf("%lld %lld",ptr[i]->T,ptr[i]->C);
        }
    Is the ampersand operator before the ptr[i] necessary? I tried it with the ampersand also but still doesn't work.

    Could you suggest how to implement the same using a structure array.By declaring something like this?
    Code:
    struct values ptr[H];

  9. #9
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    28,413
    This is what I had in mind:
    Code:
    #include <stdio.h>
    #include <stdlib.h>
    
    #define min(a, b) ((a) < (b) ? (a) : (b))
    
    struct values
    {
        unsigned long long int C;
        long long int T;
    };
    
    int compare(const void *x, const void *y)
    {
        const struct values *ptr = x;
        const struct values *ptr1 = y;
        if (ptr->C < ptr1->C)
            return -1;
        else if (ptr->C > ptr1->C)
            return 1;
        else
            return 0;
    }
    
    int main(void)
    {
        long long int N, M;
        int H, j;
        scanf("%lld %lld %d", &N, &M, &H);
        long long int toPaint = N * M;
        long long int ans = 0;
        long long int canPaint;
        struct values values_list[H];
    
        int i;
        for (i = 0; i < H; i++)
        {
            scanf("%lld %llu", &values_list[i].T, &values_list[i].C);
        }
    
        qsort(values_list, H, sizeof(values_list[0]), compare);
    
        for (i = 0; i < H; i++)
        {
            canPaint = min(toPaint, values_list[i].T);
            ans += canPaint * values_list[i].C;
            toPaint -= canPaint;
        }
    
        if (toPaint > 0)
            printf("Impossible\n");
        else
            printf("%lld\n", ans);
    
        return 0;
    }
    Notice that I kept to your code in post #1, except that I changed from using two variable length arrays T and C to a single variable length array values_list. Wherever you had T[i], I changed it to values_list[i].T, and likewise for C. I added the #include <stdlib.h> because you're now using qsort. Also, notice that you originally had T as an array of long long int, so hence the T member in this version is a long long int rather than an unsigned long long int as in your post #3. Furthermore, to read an unsigned long long int, I used %llu instead of %lld.

    Oh, and observe the "extra" parentheses that I added for the macro. This is good practice to avoid potential problems with precedence.
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  10. #10
    Registered User
    Join Date
    Sep 2014
    Posts
    51
    Quote Originally Posted by laserlight View Post
    This is what I had in mind:
    Code:
    #include <stdio.h>
    #include <stdlib.h>
    
    #define min(a, b) ((a) < (b) ? (a) : (b))
    
    struct values
    {
        unsigned long long int C;
        long long int T;
    };
    
    int compare(const void *x, const void *y)
    {
        const struct values *ptr = x;
        const struct values *ptr1 = y;
        if (ptr->C < ptr1->C)
            return -1;
        else if (ptr->C > ptr1->C)
            return 1;
        else
            return 0;
    }
    
    int main(void)
    {
        long long int N, M;
        int H, j;
        scanf("%lld %lld %d", &N, &M, &H);
        long long int toPaint = N * M;
        long long int ans = 0;
        long long int canPaint;
        struct values values_list[H];
    
        int i;
        for (i = 0; i < H; i++)
        {
            scanf("%lld %llu", &values_list[i].T, &values_list[i].C);
        }
    
        qsort(values_list, H, sizeof(values_list[0]), compare);
    
        for (i = 0; i < H; i++)
        {
            canPaint = min(toPaint, values_list[i].T);
            ans += canPaint * values_list[i].C;
            toPaint -= canPaint;
        }
    
        if (toPaint > 0)
            printf("Impossible\n");
        else
            printf("%lld\n", ans);
    
        return 0;
    }
    Notice that I kept to your code in post #1, except that I changed from using two variable length arrays T and C to a single variable length array values_list. Wherever you had T[i], I changed it to values_list[i].T, and likewise for C. I added the #include <stdlib.h> because you're now using qsort. Also, notice that you originally had T as an array of long long int, so hence the T member in this version is a long long int rather than an unsigned long long int as in your post #3. Furthermore, to read an unsigned long long int, I used %llu instead of %lld.

    Oh, and observe the "extra" parentheses that I added for the macro. This is good practice to avoid potential problems with precedence.
    Thanks for the help. It worked fine now. I'll keep those points in mind as I code

    Anyways, the code took some time 0.16s to run and 3.8M Memory. I was wondering if i could improve the execution time by using the fast I/O.

    I found this code being used in almost all submission.
    Code:
    inline void fastRead_int(long long int &x)
    {
        register long long int c = getchar();
        x = 0;
        int neg = 0;
    
    
        for(; ((c<48 || c>57) && c != '-'); c = getchar());
    
    
        if(c=='-') {
        	neg = 1;
        	c = getchar();
        }
    
    
        for(; c>47 && c<58 ; c = getchar()) {
        	x = (x<<1) + (x<<3) + c - 48;
        }
    
    
        if(neg)
        	x = -x;
    }
    However, it shows an error on the first line. It says that "D:\Bennet\Codeblocks C\Learning C\FastIO.c|3|error: expected ';', ',' or ')' before '&' token".

    I was wondering if you could explaina bit as to how this function works? Thanks

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Organizing and optimizing code into source and header files
    By edishuman in forum C++ Programming
    Replies: 3
    Last Post: 01-24-2012, 08:53 AM
  2. Optimizing code
    By KBriggs in forum C Programming
    Replies: 43
    Last Post: 06-05-2009, 04:09 PM
  3. optimizing repetitive code
    By R.Stiltskin in forum C++ Programming
    Replies: 12
    Last Post: 08-24-2008, 11:05 AM
  4. Optimizing my code
    By Lina in forum C Programming
    Replies: 30
    Last Post: 10-22-2006, 01:31 PM
  5. optimizing my quaternion code
    By confuted in forum Game Programming
    Replies: 9
    Last Post: 08-16-2003, 08:50 PM