Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Please tell me how bad is the code...in the book he said it's possible with either nested loop, or 1 loop

The user should provide the 8 double numbers for the program to set the cumulative totals in the second array.

This is the code:

#include <stdio.h>
#define ASIZE 8

int main()

{
    int index = 0, x, index2;
    double cal;
    double array1[ASIZE], array2[ASIZE];

    printf("Please enter 8 numbers:\n");
    for (index = 0; index < ASIZE; index++)//adding the numbers to the first array 
    {
        scanf("%lf", &array1[index]);
    }

    for (x = 0,index2 = 0,index = 0; x < ASIZE; x++, index2++)//adding the second array the elements
    {
        cal += array1[index++];
        array2[index2] = cal;
    }

    printf("the first array numbers are:\n");//printing the first array numbers 
    for (index = 0; index < ASIZE; index++)
    {
        printf("%.1lf ", array1[index]);
    }
    printf("\n");
    printf("\n");
    printf("the second array numbers are:\n");//printing the second array
    for (index2 = 0; index2 < ASIZE; index2++)
    {
        printf("%.1lf ", array2[index2]);
    }



}

I'm a beginner in C, and its important for me to know how to get better.

share|improve this question

3 Answers 3

up vote 2 down vote accepted

About the only thing I would change is this loop

for (x = 0,index2 = 0,index = 0; x < ASIZE; x++, index2++)//adding the second array the elements
{
    cal += array1[index++];
    array2[index2] = cal;
}

You don't need x (its really index1. As all the variables are tracking each other you don't need index2 as index1 is the same value each iteration (and allways do the increment in the for() loop.

The only bug I see is the cal is not initialized to zero and can thus have an indeterminate value.

I would have written it like this:

cal = 0.0;
for (index = 0; index < ASIZE; index++)//adding the second array the elements
{
    cal += array1[index];
    array2[index] = cal;
}

A minor note:

As the code may be changed to have bigger arrays by modifying ASIZE. I would make the user instructions reflect this automaically.

printf("Please enter 8 numbers:\n");

// Try:

printf("Please enter %d numbers:\n", ASIZE);
share|improve this answer
    
Thank you very much fot the detailed answer! It really help :) im only started to learn 2 weeks ago, hope its decent for a newbie, and to get better in my next question :) @Loki Astari –  Nir Jan 25 '13 at 21:40

For a beginner, your code is very good. There are some issues, as others have indicated. One extra issue is that you have repeated code. That is usually a undesirable. You can extract the printing loop into a separate function and call that function twice. You can also combine the first two loops so that the output array is computed as the input is read. Below is an example. Even if you haven't covered functions yet, I think it is fairly self evident what this does. Note also that I am not a fan of long variable names. There is a balance to be struck between long descriptive names and short concise names. The rule many people use is that the more local a variable is, the shorter its name may be.

#include <stdio.h>
#define ASIZE 8

static void
print_array(const char *msg, const double *d, int n)
{
    printf("%s:\n", msg);
    for (int i=0; i < n; i++) {
        printf("%.1lf ", d[i]);
    }
    printf("\n");
}

int main(int argc, char ** argv)
{
    double in[ASIZE];
    double out[ASIZE];
    double total = 0.0;

    printf("Please enter %d numbers:\n", ASIZE);
    for (int i=0; i < ASIZE; i++) {
        scanf("%lf", &in[i]);
        total += in[i];
        out[i] = total;
    }
    print_array("The input array", in, ASIZE);
    print_array("The output array", out, ASIZE);
    return 0;
}
share|improve this answer

I'll just comment on a couple minor things that Loki Astari didn't cover.

First thing is that main() returns int, but you never actually return anything. The standard thing to do is return 0 when your program ends successfully.

Your variable names could use some work. You should try to name your variables in such a way that if someone tried to read your code, without knowing what the program does, they would be able to figure it out fairly easily.

ASIZE - I could probably guess that this means "Array Size", but that doesn't tell me what this really represents in your program - the number of user input numbers. Something like NUM_USER_INPUTS would make this much clearer.

array and array2 - If I didn't know what your program was for, it would take a while to figure out what these arrays are actually for. If you named them something like userInputs and cumulativeTotals, this would be clear without even having to read the rest of your code what the arrays are for.

cal - The most I would be able to say from this name is that it stores some calculation, which doesn't say much. What calculation does it store? How about cumulativeTotal? Combine this with the array name changes, and it's very clear that this single cumulativeTotal is getting added into the cumulativeTotals array - makes this much easier to follow.

Your output could be clearer too. You should make it clear to the user what it is you're outputting assuming they've never seen your program before or the code. If some random person came along and used your program, they'd get asked to enter some numbers, then they'd be told about two arrays... what do these arrays mean? Maybe printing something like Input numbers: and Cumulative totals:. Another thing you could do to make it even nicer is do some output formatting so that the input numbers line up with the totals in columns. Try looking up how to do output formatting with printf into columns.

share|improve this answer
    
Thank you very much for the great points you cleared up..appreciate it! @MahlerFive –  Nir Jan 25 '13 at 23:07

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.