Thread: Quality Check

  1. #1
    Registered User
    Join Date
    May 2014
    Posts
    1

    Quality Check

    Hi all,

    I just started studing and practicing C programming recently. Below there is my first functioning prototype:


    Code:
    #include <stdio.h>
    #include <limits.h>
    #include <stdlib.h>
    
    /*Declare glob variables*/
    int numberOfIterations;
    char MyPath[250];
    char MyPath2[250];
    int imax = INT_MAX;
    
    /* Declare glob functions */
    int* m_Rand (int iterations);  /*This will return a pointer to the array in memory allocated with malloc*/
    int* m_fWrite (char path[250], int buffer_size, int *mem_addr); /*This will read the buffer spiecified in the mem_addr and the relative size (buffer_size) and write the content to a file (path)*/
    int* m_fRead (char path[250], int buffer_size); /*This will read a binary file and put the content of it into the buffer then return the pointer address*/
    
    
    
    int main() {
    
    printf("Integer MAX is %i and integer size is %d \n", imax, sizeof(int));
    
    printf("Hello, put the source file path\n");
    scanf("%s", &MyPath);
    printf("Input is %s\n", MyPath);
    printf("put the destination file path\n");
    scanf("%s", &MyPath2);
    printf("Input is %s\n", MyPath2);
    printf("Now put the iterations number\n");
    scanf("%d", &numberOfIterations);
    printf("Input is %s and %i\n", MyPath, numberOfIterations);
    
    
    // initialise an array in memory with numberOfIterations amount of lines filled with rand()
    int* memory_address = m_Rand(numberOfIterations);
    printf("Buffer Allocated, mem_addrress is %d\n", *memory_address);
    
    // write it in a binary file (MyPath) from that memory_address
    m_fWrite(MyPath, numberOfIterations, memory_address);
    free(memory_address);
    
    // read from that binary file (MyPath) and initialise another array 
    memory_address = m_fRead(MyPath, numberOfIterations);
    printf("Buffer Allocated from file, mem_addrress is %d\n", *memory_address);
    
    // write the content of memory_address in another file (MyPath2)
    m_fWrite(MyPath2, numberOfIterations, memory_address);
    free(memory_address);
    
    
    return 0
    
    };
    
    
    
    int* m_Rand (int iterations){
    
    int *mem_location;
    int loopCount;
    mem_location = malloc(sizeof(int) * iterations);
    
    
    for (loopCount = 0; loopCount < iterations; loopCount++) {
             /*printf("LoopCount %i, Iteration n %i\n", loopCount, iterations);*/
         int randomicValue = rand();
             mem_location[loopCount] = randomicValue;
    };
    
    
    return mem_location;
    
    };
    
    
    
    int* m_fWrite(char path[250],int buffer_size, int *mem_addr){
    printf ("Start path is %s buffer_size is %d and *mem_addr is %i\n", path, buffer_size, *mem_addr);
    
    FILE* the_file;
    
    if ((the_file = fopen(path, "wb")) == NULL) {
        printf ("Cannot open %s", path);
        return 1;
    };
    
    
    printf ("calling fwrite to write from mem_addr %d with buffer_size %d into file %s\n", *mem_addr, buffer_size, the_file);
    fwrite(mem_addr, sizeof(int), buffer_size, the_file); 
    
    if ( (fclose(the_file)) != 0) {
            printf("Could not close file %s\n", path);
        return 1;
    };
    
    return 0;
    
    };
    
    
    int* m_fRead(char path[250],int buffer_size){
    printf ("Start path is %s buffer_size is %d\n", path, buffer_size);
    
    int *mem_location = malloc(sizeof(int) * buffer_size);
    
    FILE* the_file;
    
    if ((the_file = fopen(path, "rb")) == NULL) {
            printf ("Cannot open %s", path);
            return 1;
    };
    
    
    printf ("calling fread to read to mem_location %d with buffer_size %d from file %s\n", *mem_location, buffer_size, the_file);
    fread(mem_location, sizeof(int), buffer_size, the_file);
    
    if ( (fclose(the_file)) != 0) {
            printf("Could not close file %s\n", path);
            return 1;
    };
    
    return mem_location;
    
    };
    I would like to ask to some experienced person to judge this code (not to hard please ) and make me understand how can it be improved especially in:


    • formatting and indenting;
    • efficency;
    • variables names;
    • error handling;


    The program is basically taking 3 inputs from the user:

    MyPath -- Path for a file to write
    MyPath2 -- Path for a different file to write
    numberOfIterations -- an integer

    The function m_Rand() takes numberOfIterations as loop counter and write with rand() as much lines as numberOfIterations defines. Then it uses malloc to allocate these in an array in memory and returns the a pointer with the memory address.

    m_fWrite() is writing that array into a binary file (MyPath) and than I call free().

    m_fRead reads that file and put it into memory with malloc() and the the same gets writted with m_fWrite() into MyPath2. Then I calld free() again to free the memory up.

    The expected result is that MyPath and MyPath2 are identical.


    The program is working but I would like to get a quality check from someone more expert than me, since this is my first piece of software.

    Thanks everyone.

  2. #2
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    28,413
    Get rid of imax as you might as well use INT_MAX. The other global variables should be local to the main function instead.

    This is wrong:
    Code:
    scanf("%s", &MyPath);
    MyPath is an array, so it will be converted to a pointer to its first element. Therefore, you should write:
    Code:
    scanf("%s", &MyPath);
    But this is still problematic because the user can enter more characters than can be stored in MyPath, so it would be better to write:
    Code:
    scanf("%249s", &MyPath);
    Even better would be to avoid hardcoding this, but first you should define a named constant for the size of your array instead of directly using 250.

    Furthermore, you should check the return value of scanf.

    Your indentation needs some work. For example, instead of:
    Code:
    int main() {
     
    printf("Integer MAX is %i and integer size is %d \n", imax, sizeof(int));
    I would expect:
    Code:
    int main() {
     
        printf("Integer MAX is %i and integer size is %d \n", imax, sizeof(int));
    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
    Jun 2005
    Posts
    6,815
    Quote Originally Posted by lifeonatrip View Post
    I would like to ask to some experienced person to judge this code (not to hard please ) and make me understand how can it be improved especially in:
    • formatting and indenting;
    • efficency;
    • variables names;
    • error handling;
    Bear in mind that opinions on any of these things are like backsides - everyone has one, and they're not all the same.

    On formatting and indenting, I suggest trying to look at your code from a perspective of trying to work out things about it. With your indenting I have trouble recognising when one function (or block within a function) ends, and the next begins. Being able to recognise those things is crucial because, if those things are difficult to recognise, code maintenance is tough. For example, all of the statements inside main() are left justified, which makes it difficult to recognise where main() ends.

    Rather than give my opinion on your variable names, I suggest asking yourself the question: does the choice of names help make the code (overall) easier to understand. As a rule of thumb, if you need comments, either your variable and function naming is insufficient, or you're doing something too complex to be captured in any other way but comments. A primary goal in writing software should be to minimise complexity (make the code no more complex than it needs to be, as long as it does what is intended). Sometimes comments are needed for involved code but, more often than not, they are a sign that your code is more complex than it needs to be.

    I'm not going to comment on efficiency of your code at all. Your time is better spent on getting the program working correctly, not efficiently. Once it's working, you can then worry about whether it is fast enough (e.g. is a user complaining it is non-responsive) and use appropriate tools to find hot-spots (places in code that contribute to inefficiency). Your time is better spent picking decent algorithms (which beginners rarely even consider) not microoptimisations of code (which is where most beginners focus when worrying about efficiency).

    Your error handling is minimal, and you probably need more of it.
    1) scanf(), printf(), and fwrite() all return error conditions, which your code is not testing for at all
    2) malloc() returns NULL when it fails, and your code is not checking for that.
    3) your m_rand() m_fread() function are both calling malloc() - and not checking if it succeeded - and returning that value. That obligates the caller to check the return value of your functions - which is not happening.
    4) m_fwrite() is returning different values (presumably success of failure) but the caller is not checking those values. So why return them.


    Unrelated to the questions you asked

    1) m_fwrite() is returning a pointer, but returns values 0 and 1. That is not good, because the value 1 is meaningless for a pointer (zero is the NULL pointer). Either change the function so it returns an int (not a pointer) or return something that makes sense as a pointer.

    2) Use of static/global variables is rarely a good idea. They have their place but, unless used carefully, make code harder for mere humans to understand. Until you understand the implications of using them, you would be better off not using them. That would require passing extra arguments to the functions. Beginners try to avoid passing extra arguments to functions. Professionals prefer passing extra arguments (bundled in structures if needed) over use of statics, unless there is a real reason to use statics.

    3) You've got a few misplaced semi-colons, which tend to affect code readability (and can cause unexpected surprises by changing behaviour, on occasion)
    Last edited by grumpy; 05-25-2014 at 12:20 AM.
    Right 98% of the time, and don't care about the other 3%.

    If I seem grumpy or unhelpful in reply to you, or tell you you need to demonstrate more effort before you can expect help, it is likely you deserve it. Suck it up, Buttercup, and read this, this, and this before posting again.

  4. #4
    Registered User
    Join Date
    Feb 2014
    Posts
    13
    Some more comments for the above program,

    1) In m_Rand(),

    Code:
    int *mem_location = NULL;
    
    
    mem_location = malloc(sizeof(int) * iterations);
    
    
    if (NULL != mem_location)
    {
    	/* Do operation s */
    }
    else
    {
    	printf("Err: Memory allocation failed");
    }
    Please modify the same kind of checking in all the places.


    2) In main(), make memory_address as NULL once freeing it.

  5. #5
    Registered User
    Join Date
    Jun 2005
    Posts
    6,815
    Quote Originally Posted by kannanm View Post
    2) In main(), make memory_address as NULL once freeing it.
    If the pointer will no longer exist after calling free() - as is the case here, since the call of free() is the last statement in main() - setting said pointer to NULL achieves exactly nothing of use.

    Setting to NULL is only necessary or worthwhile if a pointer is being recycled and (more importantly) there is a subsequent check of whether it is NULL. Neither is true here.
    Right 98% of the time, and don't care about the other 3%.

    If I seem grumpy or unhelpful in reply to you, or tell you you need to demonstrate more effort before you can expect help, it is likely you deserve it. Suck it up, Buttercup, and read this, this, and this before posting again.

  6. #6
    Registered User
    Join Date
    Jun 2005
    Posts
    6,815
    Quote Originally Posted by kannanm View Post
    2) In main(), make memory_address as NULL once freeing it.
    If the pointer will no longer exist after calling free() - as is the case here, since the call of free() is the last statement in main() - setting said pointer to NULL achieves exactly nothing of use.

    Setting to NULL is only necessary or worthwhile if a pointer is being recycled and (more importantly) there is a subsequent check of whether it is NULL. Neither is true here.
    Right 98% of the time, and don't care about the other 3%.

    If I seem grumpy or unhelpful in reply to you, or tell you you need to demonstrate more effort before you can expect help, it is likely you deserve it. Suck it up, Buttercup, and read this, this, and this before posting again.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. help- quality control program
    By jccccc in forum C Programming
    Replies: 10
    Last Post: 01-21-2011, 10:42 PM
  2. Quality
    By peckitt99 in forum Windows Programming
    Replies: 4
    Last Post: 03-24-2007, 11:02 AM
  3. Quality C Programmer Requested
    By cencyclopedia in forum Projects and Job Recruitment
    Replies: 9
    Last Post: 01-08-2007, 08:52 PM
  4. Code Quality
    By silk.odyssey in forum C++ Programming
    Replies: 1
    Last Post: 07-31-2004, 09:33 PM
  5. video quality...
    By ober in forum Linux Programming
    Replies: 8
    Last Post: 10-04-2001, 09:44 AM

Tags for this Thread