pointers/arrays/structures

I'm trying to go through a text file I'm passing and keep track of the numbers I'm reading in and the frequency they occurs in the 12 number file. But here's the thing, I only want to use pointers. The problem I'm running into is at the frequency part. When I compile and run the code, this is the output:
N: 10 F: 3
N: 10 F: 3
N: 8 F: 1
N: 7 F: 65
N: 5 F: 0
N: 4 F: 63
N: 10 F: 3
Segmentation fault (core dumped)

I had it printing out all the numbers before without it core dumping, but they were still wrong, as it is now (except the first 2). The N (number) is being stored in the hist struct array properly, but the F (frequency) is wrong after the first tens tens. I know the problem is in calcHistogram somewhere in the loops of chaos. Any tips would be greatly appreciated!

My guess would be that I'm going out of the range of the array somewhere, somehow. I simply can't find it though. Thanks in advance.

Code:

Code:

---


#include <stdio.h>

struct freq {
    int number;
    int frequency;
};

//function prototypes
void readScores(int* ar, int* count);
void displayScores(int* ar, int* count);
void calcHistogram(int* ar, int* count, struct freq* hist, int* ct);

int main() {
    int ar[100];
    int count = 0;
    int ct = 0;
   
    readScores(ar, &count);
    displayScores(ar, &count);
   
    struct freq hist[count];

    printf("\n");
    calcHistogram(ar, &count, hist, &ct);   
}

void readScores(int* ar, int* count) {
   
    for (int i=0; i<11; i++) {
        (*count)++;
        scanf("%d", (ar+i));
    }
}

void displayScores(int* ar, int* count) {
    for(int i=0; i<*count; i++) {
        printf("score %d: %d\n", i, *(ar+i));
    }
}
void calcHistogram(int* ar, int* count, struct freq* hist, int* ct) {
    int uniqueNumber;
    for(int i=0; i<*count; i++){
        uniqueNumber = *(ar+i);
        for(int j=0; j<=*ct; j++){
            if(uniqueNumber != (*(hist+i)).number){
                (*(hist+i)).number = uniqueNumber;
               
            }
            if(*(ar+i) == *(ar+j)){
                (*(hist+i)).frequency += 1;
                *ct++;
            }
        }
        printf("N: %d F: %d\n", (*(hist+i)).number (*(hist+i)).frequency);
    }
}

---

I don't have a computer to run your code at the moment, but Seg fault means that you have tried to access memory which you don't have permission to access

A few things -

"main" is not returning anything -> Very bad for your operating system.

You are not initialising all your variables

and most importantly, you have forgotten a "," in your arguments for that printf statement on line 54

Here's what I get when I try to compile the code you provided:

Code:

---

$ make foo
gcc -Wall -Wunreachable-code -ggdb3 -std=c99 -pedantic  -lm -lpthread  foo.c  -o foo
foo.c: In function ‘calcHistogram’:
foo.c:51: warning: value computed is not used
foo.c:54: error: called object ‘(hist + (unsigned int)((unsigned int)i * 8u))->number’ is not a function

---

The second error appears to be due to a missing comma (,) between the second and third parameters to printf.

The first error is related to operator precedence. The postincrement (++) is higher than the dereference (*), thus you take the pointer ct, increment it (effectively changing the address it points to), then dereference it. But you don't do anything with the dereferenced value, hence the warning. The more disturbing thing is that ct (in calcHistogram) no longer points to the correct piece of memory. Thus, in your inner for loop, it keeps going until j reaches *ct, which is some unknown number since ct now points at garbage memory. That seems to be the cause of your seg fault. To fix it, wrap *ct in parentheses before incrementing (like count in your readScores function). Change line 51 to

Code:

---

(*ct)++;

---

Note, your algorithm still seems to have some bugs, but I'll let you work on that, wouldn't want to spoil the fun :p.

Let's start by fixing your readscores function:

• You need to check the return value of scanf. If it only happens to store 4 elements successfully, your function will pretend it stored 11.
• It makes more sense to return the number of elements stored than to store it through a pointer.
• If the maximum limit changes from 11 to something else, you have to change your function's definition.

Code:

---

int readscores(int *ar, int n) { int i; for (i = 0; i < n && scanf("%d", ar++) == 1; i++); return i; }

---

Then call it like so:

int count = readscores(ar, 11);

It's unnecessary to pass pointers in most of the places that you have. The prototypes for your other two functions can be changed to the following:

Code:

---

void displayscores(const int *ar, int n);
int calchistory(const int *ar, int n, struct freq *hist);

---

Quote:

---

*(ar+i)
...
(*(hist+i)).frequency

---

a[i], hist[i].frequency — they do the exact same thing and they're much easier to read.

Fixing those thing should make your code more compact and less confusing.

@Anduril462 ... thank you very much for that catch on the (*ct)++....i've been weary of that mistake in the past but i guess let that one slip by. fixing that now puts me back to where i was before with the following output:

N: 10 F: 2
N: 10 F: 2
N: 8 F: 1
N: 7 F: 64
N: 5 F: 1
N: 4 F: 64
N: 10 F: 3
N: 2 F: 2
N: 2 F: 32769
N: 8 F: 32769
N: 7 F: 32769


It's weird because some of the elements of the array are correct, for instance the 10, but others give me garbage...Really not sure what is going on

Code:

---

(*(hist+i)).frequency += 1;

---

What is the value in (*(hist+i)).frequency the first time, before you add 1? Hint, it's not necessarily zero.

I also would like say, I agree with Barney, that a[i] is much easier to read, and works the same as *(a+i). I didn't mention it because you wanted to use only pointers. However, there's absolutely no reason to use *(a+i), except as an academic exercise in how to write awkward, unreadable code.

Quote:

---

Originally Posted by anduril462

Code:

---

(*(hist+i)).frequency += 1;

---

What is the value in (*(hist+i)).frequency the first time, before you add 1? Hint, it's not necessarily zero.

I also would like say, I agree with Barney, that a[i] is much easier to read, and works the same as *(a+i). I didn't mention it because you wanted to use only pointers. However, there's absolutely no reason to use *(a+i), except as an academic exercise in how to write awkward, unreadable code.

---

I think it's juts random garbage until it's initialized, which it isn't off the get go.... I'm trying to complete this program with only using pointers, not bracket notation so that's the reason behind my overuse of pointers

Correct, it's not initialized, so initialize it. Line 47 seems like a good place.

Quote:

---

Originally Posted by anduril462

Correct, it's not initialized, so initialize it. Line 47 seems like a good place.

---

Awesome, that fixed the strange outputs...but now how could I go about displaying these without duplications, so only one "10" number would show up with frequency of "3" instead of three "10"s

Preferably in another function

I think the fact that you are displaying duplicates means you are not calculating your histogram correctly. Another function (or two) would be good to separate two different parts of your code. This could also help you isolate your problem and test your code.

First, create a function for printing the histogram data. Something like:

Code:

---

void printHistogram(struct freq *hist, int histSize)  // note, histSize is not a pointer

---

Make sure that works, so you can use it to see the contents of the histogram as you test your code. You can make a static array of histogram data to test:

Code:

---

struct freq testHist[3] = {
    {10, 2},
    {1, 1},
    {7, 3}
};
printHistogram(testHist, 3);

---

Second, I would suggest a function like the following:

Code:

---

int addNumToHistogram(struct freq *hist, int *histSize, int maxHistSize, int num)

---

That function will search hist for num. If num is alread in the histogram, it will increment the count, otherwise it adds it to the end (assuming it doesn't exceed maxHistSize), with a count of 1, and increments *histSize. Use that function by itself (without calcHistogram), and simply insert 5 or so numbers with this function, with, perhaps one or two duplicates to test. Use the print function to make sure it looks correct. Like so:

Code:

---

struct freq testHist[10] = {{0}};
int histSize = 0;
addNumToHistogram(hist, &histSize, 10, 7);  // add 7 to the histogram
addNumToHistogram(hist, &histSize, 10, 7);  // add 7 to the histogram
addNumToHistogram(hist, &histSize, 10, 3);  // add 3 to the histogram
addNumToHistogram(hist, &histSize, 10, 4);  // add 4 to the histogram
addNumToHistogram(hist, &histSize, 10, 3);  // add 3 to the histogram
addNumToHistogram(hist, &histSize, 10, 1);  // add 1 to the histogram
printHistogram(hist, histSize);

---

Once you have those two functions working, the rest should be easy. You can simply call that addNumToHistogram from calcHistogram:

Code:

---

void calcHistogram(int* ar, int* count, struct freq* hist, int* ct)
{
    for i from 0; < *count
        addNumToHistogram(hist, ct, *(ar + i))
}

---

Quote:

---

Originally Posted by anduril462

I think the fact that you are displaying duplicates means you are not calculating your histogram correctly. Another function (or two) would be good to separate two different parts of your code. This could also help you isolate your problem and test your code.

First, create a function for printing the histogram data. Something like:

Code:

---

void printHistogram(struct freq *hist, int histSize)  // note, histSize is not a pointer

---

Make sure that works, so you can use it to see the contents of the histogram as you test your code. You can make a static array of histogram data to test:

Code:

---

struct freq testHist[3] = {
    {10, 2},
    {1, 1},
    {7, 3}
};
printHistogram(testHist, 3);

---

Second, I would suggest a function like the following:

Code:

---

int addNumToHistogram(struct freq *hist, int *histSize, int maxHistSize, int num)

---

That function will search hist for num. If num is alread in the histogram, it will increment the count, otherwise it adds it to the end (assuming it doesn't exceed maxHistSize), with a count of 1, and increments *histSize. Use that function by itself (without calcHistogram), and simply insert 5 or so numbers with this function, with, perhaps one or two duplicates to test. Use the print function to make sure it looks correct. Like so:

Code:

---

struct freq testHist[10] = {{0}};
int histSize = 0;
addNumToHistogram(hist, &histSize, 10, 7);  // add 7 to the histogram
addNumToHistogram(hist, &histSize, 10, 7);  // add 7 to the histogram
addNumToHistogram(hist, &histSize, 10, 3);  // add 3 to the histogram
addNumToHistogram(hist, &histSize, 10, 4);  // add 4 to the histogram
addNumToHistogram(hist, &histSize, 10, 3);  // add 3 to the histogram
addNumToHistogram(hist, &histSize, 10, 1);  // add 1 to the histogram
printHistogram(hist, histSize);

---

Once you have those two functions working, the rest should be easy. You can simply call that addNumToHistogram from calcHistogram:

Code:

---

void calcHistogram(int* ar, int* count, struct freq* hist, int* ct)
{
    for i from 0; < *count
        addNumToHistogram(hist, ct, *(ar + i))
}

---

---

I think it'd be cool if I could get it to add to frequency rather than fill in another N, like i just said, but in the same function..i feel like that is doable...Here's my function

Code:

---

void calcHistogram(int* ar, int* count, struct freq* hist, int* ct) {
    int uniqueNumber;
    for(int i=0; i<*count; i++){
            (*(hist+i)).frequency = 0;
            uniqueNumber = *(ar+i);
            for(int j=0; j<=*ct; j++){
                  if(uniqueNumber != (*(hist+i)).number){
                          (*(hist+i)).number = uniqueNumber;
                  }
                  if(*(ar+i) == *(ar+j)){
                        (*(hist+i)).frequency += 1;
                        (*ct)++;
              }
        }
        printf("N: %d F: %d\n", (*(hist+i)).number, (*(hist+i)).frequency);
    }
}

---

And here is the output of this:

N: 10 F: 2
N: 10 F: 2
N: 8 F: 1
N: 7 F: 1
N: 5 F: 1
N: 4 F: 1
N: 10 F: 3
N: 2 F: 2
N: 2 F: 2
N: 8 F: 2
N: 7 F: 2

I want it to be sorted by F in descending order but with no duplicate N's

I think you missed the whole point of my post. I intended for it to merely increment the frequency if the number already existed, or to add it if did not exist yet. That would avoid duplicates. I was leaving the design/implementation for you to do. I didn't realize about the sorting, but that is a whole separate problem from removing duplicates. Don't try to do everything at once.

The plan I laid out is meant to help you conquer small parts of your program one at a time. Frankly, it is a better design than doing it all at once, and all in one function. A function should do one thing and do it well. You want "separation of concerns", i.e. keep separate parts of code away from each other. The print function should not care whether you read data from a user's keyboard or a file. The function that adds a number to the histogram should not care whether that number came from an array or from a constant, a user's keyboard, a file, etc; nor should it care what order you want to display the data. Ultimately, that results in loosely coupled code, which is easier to develop, maintain and extend. What if you no longer have an array of nums from the user you're using as a data source? What if you want to process a file with 10,000,000 numbers in it? Do you want to have to declare an array of 10,000,000 elements just in case you want to do that? No. Do you want to have to change your print function or your calcHistogram function because you are simply reading from a file instead of the keyboard? No. You should be able to write a readFromFile function that reads numbers one-by-one and inserts them directly into the histogram instead of using an array as an intermediate, and everything else should just work.

And lastly, you could use a sort function that uses comparator functions like the built-in qsort or design your own that uses comparators. You can have multiple comparator functions, one that sorts by frequency, another that sorts by number. Then you can display the data sorted by F or N, or both, whatever the user wants. That sorting should be handled outside of reading the data in and populating the histogram. Just some more examples of good design by separating your concerns.

Take for example, the problem you're encountering. There are duplicates. How do you know what the problem is? It could be in your for (i=0...) loop, because you're mishandling ar and going off the end, corrupting data in hist. Or it could be in your for(j = 0...) loop where you're mishandling hist, or not properly initializing something else. You have several places to debug one single problem. Compare that to compartmentalizing each small task to it's own function, and test that function in isolation before you integrate it into the larger solution. Once you test the function in isolation, you know that it works. Thus, if there are problems when you integrate it, it must be in the calling code, since you know the function works. That is a much smaller area of code to look for problems.

I really, really strongly encourage you to follow the plan I provided above. I will make solving this problem much easier.