Counting Program

[audifanatic518]

Hi,

I've just started taking an intermediate C/C++ class at my school and it's been about a year since I've last programed in C. Needless to say, I'm a little rusty. My first assignment is to write a program which scans in up to 50 unique numbers (ie. you can type in the number 1 one-million times, the number 2 two-million times, and still have room for 48 more numbers). I've written the program, but I have one small bug in it that I'm trying to figure out how to overcome. I know what the problem is, but I need a little advice on how to solve it. My program stores each unique number in an array of 50 elements and increments a counter at the same index as the respective number. So for instance, if the number 2 is stored in index 17 of my 'numbers' array, and the user inputs 1000 number two's, then the number in my 'numberCounts' array at index 17 should be 1000. However, it seems that my program overwrites some numbers in my 'numbers' array depending on what kind of sequence the user enters. It used to be that it would overwrite all zeroes if there was more than one zero in the array. I tried to fix the problem using a 'newNumber' function which ensured that every element in the 'numbers' array was unique, but now if the user enters "1,2,3,4,5" in that order, nothing prints out whatsoever. I was thinking of using the srand() function to remedy it, but I feel like that's a bit sloppy and there has to be a better way to do this. If someone could give me a few pointers, I would appreciate it. I'm not looking for someone to do my homework at all, as you can see, I've done it. I just need some help debugging. Thanks for your time. My code is pasted below:

#include <stdio.h>
#include <stdlib.h>
#include <time.h>
#define MaxNumElements 50
#define true 1
#define false 0
typedef int boolean;

int newNumber();     // used to place a unique number in each array position so that no number is in array more than once, which would otherwise cause overwriting.

void main()
{
    //Declare and initialize arrays and variables
    int numbers[50], numberCounts[50];
    int number = 0;

    //initialize arrays
    for (int i = 0; i < MaxNumElements; ++i) {numbers[i] = newNumber(); numberCounts[i] = 0;}

    //begin scanning
    printf("Please Enter some integers to be counted or press 'q' to quit\n");

    boolean exists = false;  //used to ensure number is only placed in numbers array once
    int scanCounter = 0;    // used to detect how many new elements are introduced into array
    while(scanf("%i", &number))
    {
        for(int i = 0; i < MaxNumElements; ++i)
        {
            if(number == numbers[i])
            {
                ++numberCounts[i];
                exists = true;
                break;
            }
        }

        if((exists == false))
        {
            numbers[scanCounter] = number;
            ++numberCounts[scanCounter];
            ++scanCounter;
        }
        exists = false;
    }

    //sort array from highest to lowest
    int tempNum, tempCount;
    for (int i = 0; i< scanCounter-1; i++)
        for(int j = i+1; j < scanCounter; ++j)
            if (numbers[i] < numbers[j])
            {
              tempNum = numbers[i]; tempCount = numberCounts[i];
              numbers[i] = numbers[j]; numberCounts[i] = numberCounts[j];
              numbers[j] = tempNum; numberCounts[j] = tempCount;
            }

    printf("       N      Count  \n");
    printf("     -----    -----  \n");
    for(int i = 0; i < scanCounter; ++i)
    {
        printf("%9i%9i\n", numbers[i], numberCounts[i]);
    }
}

int newNumber()
{
    static int number = 0;
    ++number;
    return number;
}

[memcpy]

Don't typedef "int" boolean, you're wasting tons of memory. Just include <stdbool.h>, which defines true, false, and bool.

I haven't really read through all of it yet, but at first glance, newNumber will be one every time.

[audifanatic518]

Don't typedef "int" boolean, you're wasting tons of memory. Just include <stdbool.h>, which defines true, false, and bool.

I haven't really read through all of it yet, but at first glance, newNumber will be one every time.

I know, I was going to do that, but for some reason Eclipse (C/C++ plugin) wouldn't recognize that library and gave me a problem whenerver I tried to use 'bool.' So that's why I used a typedef.

EDIT: I just tried stdbool.h again. Turned out I had to compile the project once with <stdbool.h> included before I could use the bool type in my program. Must be some sort of bug in Eclipse.

As far as newNumber is concerned, it does return a new number every time (incrememnted by one each time it's called). I ran it through the debugger just to make sure.

[memcpy]

Okay, I see.

Instead of newnumber(), why not just assign it to i? Actually, why assign it to anything? You're going to overwrite it with data anyway.

Split main (make it int main, not void main) into different functions that actually do what you're trying to do. For example, something like this:

bool does_exist(int *array, int value)
{
    int *q = array;
    while (*q) {
        if (*q++ == value)
            return true;
    }
    return false;
}

for all of your different methods.

Once you've done this (and used some variation of the function I just said, that way, you don't need to store two arrays), I'll help you from there.

[audifanatic518]

Instead of newnumber(), why not just assign it to i? Actually, why assign it to anything? You're going to overwrite it with data anyway.

The reason I was assigning values to the array was because the array would otherwise be filled with whatever junk was left in the memory previously. When I ran the debugger, that turned out to be mostly zeroes but the occasional large number like 3476294. So I would have been back to square one with my problems with zeroes. I had to ensure that all values were unique. Although now that you mention it, i could just assign it to i. I must've been overthinking haha.

Split main (make it int main, not void main) into different functions that actually do what you're trying to do. For example, something like this:

for all of your different methods.

Grrr...I was hoping it wouldn't come down to pointers, but I gotta do what I've gotta do I guess.

Once you've done this (and used some variation of the function I just said, that way, you don't need to store two arrays), I'll help you from there.

How will that solve my two arrays problem? I need the two arrays because I have to somehow save the numbers the user inputs and also save the amount of times they entered the number as well (since both have to be printed to the console later on).

Thanks for all your help by the way, I appreciate what you've done so far.

[memcpy]

The reason I was assigning values to the array was because the array would otherwise be filled with whatever junk was left in the memory previously. When I ran the debugger, that turned out to be mostly zeroes but the occasional large number like 3476294. So I would have been back to square one with my problems with zeroes. I had to ensure that all values were unique.

Right, but assigning to zero is still the best solution. Otherwise, every number that you input could appear as a duplicate, so just disallow inputting zero and get rid of the loops initializing to zero. Instead, use

memset((void *)numbers, 0, sizeof(int) * 50);
memset((void *)numberCounts, 0, sizeof(int) * 50);

given that memset is specifically optimized per architecture to give the fastest possible throughput. I'm not certain, but it should break the arrays into bigger chunks of memory for zeroing for maximum efficiency. Instead of incrementing by four bytes, it tries to use eight (and etc.)

Grrr...I was hoping it wouldn't come down to pointers, but I gotta do what I've gotta do I guess.

Well, C is all about pointers, so you'd better learn them sooner or later

How will that solve my two arrays problem? I need the two arrays because I have to somehow save the numbers the user inputs and also save the amount of times they entered the number as well (since both have to be printed to the console later on).

It won't solve the two arrays problem, I forgot that you wanted to also print the amount of duplicates (sorry!). However, there's probably a better way to solve the issue of printing both values, you might want to use a struct?

struct number_entry {
    int value;
    unsigned short duplicates; /* short, because no one's gonna enter more than 65k times */
};

bool does_exist(struct number_entry ***_entry, int value)
{
    struct number_entry **entry = *_entry; 
    int tmp;
/* this way, you can smash two functions into one by incrementing the count while you check for duplicates */

    for (tmp = 0; tmp < 50; tmp++) {
        if (entry[tmp]->value == value) 
            entry[tmp]->duplicates++;
    }

    if (entry[tmp]->duplicates)
        return true;
    else
        return false;

Or perhaps that's completely off-base and you won't need/use it at all. Just an idea.

[audifanatic518]

I think I'm gonna put this down for a bit and come back to it tomorrow. too much information to wrap my head around at once lol.

[iMalc]

memset((void *)numbers, 0, sizeof(int) * 50);
memset((void *)numberCounts, 0, sizeof(int) * 50);

Or you could do that more nicely as:

memset(numbers, 0, sizeof(numbers));
memset(numberCounts, 0, sizeof(numberCounts));