Given a matrix, create a list which..

Hi everyone!

I'm trying to create this function:

Given a matrix, create a list which has in his nodes the values of the matrix that are greater than zero. The function has to return the double pointer to the list.

Here there is my function:

Code:

---


struct node** positive_mtr (int *A, int r, int c){

        struct node **doub;
        struct node *point;
        doub = &point;
        *doub= NULL;
        int i,j, value;


        for (i=0;i<r;i++){
            for (j=0;j<c;j++){
                if(A[i*c+j]>0){
                    value= A[i*c+j];
                    suf_insert(doub, value);


/* suf_insert is the function which insert the value at the end of the list*/
                }
            }
        }
        return doub;


}

---

Here I write you the main where I use this function:

Code:

---


int main(){


  struct node **dptr;


  int *B;
  int n=2;
  B=(int*)calloc(n*n, sizeof(int));


  int i,j,value;


        for (i=0;i<n;i++){
              for (j=0;j<n;j++){
                  printf ("\n\which values do you want to put ");
                  printf("  in the position a=%d c=%d ?\n\n",i+1,j+1);


                  scanf("%d",&value);
                  B[i*n+j]=value;
              }
        }


        dptr=positive_mtr(B,n,n);


        printf ("\n\n\n\n The values of the matrix greater than zero \n\n");


        while ((*dptr)!=NULL){


            printf ("\n\n one of the values in the list is %d\n\n", (*dptr)->value);
            (*dptr)= (*dptr)->next;
        }


  return 0;
}

---

In the main, the program terminates at the first cycle of while.

Instead of printing "one the values in the list is (the first number greater than zero of the matrix)",
it prints "one the values in the list is 268433539".

Someone told me that there are some mistakes in the main, while the function is correct.
May someone help me to find the mistakes in the main please?

Quote:

---

Originally Posted by letthem

I'm trying to create this function:

Given a matrix, create a list which has in his nodes the values of the matrix that are greater than zero. The function has to return the double pointer to the list.

---

Are you coming up with these exercises yourself, or are you referring to a book or something? I ask because there is no need to return a pointer to a pointer for this function. You also don't need a pointer to a pointer in the function. You only need pointers to the nodes, in particular the head node (but keep reading).

Furthermore, ditch the suf_insert function. It's all good to have a function to append to the tail when you want to do it as a one off thing or when you have a linked list in which you are keeping track of the tail. But here you are creating a linked list and appending nodes... calling suf_insert in a loop means that instead of creating the linked list in linear time, you're creating the linked list in quadratic time! Rather, keep track of the tail in the function and keep appending the new node, making it the new tail.

In your main function, you only need a pointer to the head node of the linked list, and another pointer to the current node for things like printing the elements of the linked list. Don't overcomplicate with a pointer to a pointer.

Quote:

---

Originally Posted by letthem

Someone told me that there are some mistakes in the main, while the function is correct.

---

That person is mistaken. As part of the problem of your overcomplicating with a pointer to a pointer, you caused the pointer to a pointer to point to a pointer local to the function, i.e., you returned a pointer to a non-static local variable: such a variable no longer exists after the function returns, so your pointer to a pointer immediately ends up pointing to something invalid.

A few other things to address:

• Check that calloc did not return a null pointer. Also, there's no need to cast the return value of calloc to int*
• Check the return value of scanf
• free what you malloc/calloc/realloc
• Indent your code consistently and avoid using multiple consecutive blank lines within a function body.

Quote:

---

Originally Posted by laserlight

Are you coming up with these exercises yourself, or are you referring to a book or something?

---

They're exercises taken by exams of the previous years.
The university course is "Fundamentals of C language".
It regards just the fundamentals of this language, so it isn't about the best possibile implementation, but, I guess, its aim is to teach people how to handle the main elements of this language.
There are some things that are criticizable (a lot of people complain about that course), but now my targets are to learn and to pass the exam.
I can understand that for experts a lot of things are stupid.

Quote:

---

Originally Posted by laserlight

In your main function, you only need a pointer to the head node of the linked list, and another pointer to the current node for things like printing the elements of the linked list. Don't overcomplicate with a pointer to a pointer.

---

I would like to do it, but unfortunately it is required by the exercise (sorry if I didn't say this before).

Quote:

---

Originally Posted by laserlight

A few other things to address:

• Check that calloc did not return a null pointer. Also, there's no need to cast the return value of calloc to int*
• Check the return value of scanf
• free what you malloc/calloc/realloc
• Indent your code consistently and avoid using multiple consecutive blank lines within a function body.

---

As always, thanks for your precious advices Laserlight.
Unluckly, I checked and there aren't problems with the scanf or with the calloc (they work as they're supposed to work).

It compiles, but it doesn't work, it doesn't do what is supposed to do.
I don't know why at the end, when it prints, instead of printing the elements of the list (which are the elements of the matrix greater than zero),
it prints just one time the number 268433539 and then terminates.
I'm using also the debugger but I still don't find the problem.




P.S. This is the suf_insert (it uses another function init_root if the list is empty)

Code:

---


void init(struct list **ptrptr) {
    *ptrptr = NULL;
}



void suf_insert(struct list **doubleptr, int val) {


  if (*doubleptr != NULL) {



    while ((*doubleptr)->next != NULL) {


      doubleptr = &((*doubleptr)->next);
    }


    (*doubleptr)->next = (struct list *)malloc(sizeof(struct list));
 


    ((*doubleptr)->next)->value = val;
    ((*doubleptr)->next)->next = NULL;
  }
else{


    init_root_node(doubleptr,val);
  }
}


void init_root_node(struct list **head,int value ){


  *head = malloc(sizeof(struct list));
  (*head)->value = value;
  (*head)->next = NULL;
}

---

If possible, I would like to avoid to change this functions, because I used them for a lot of projects :D

Quote:

---

Originally Posted by letthem

I would like to do it, but unfortunately it is required by the exercise (sorry if I didn't say this before).

---

Ah, in this case it is a "trap" set by the exam setter, and you fell right into it. I expect that students would tend to do two things:

• Try to work with a pointer to a pointer all the way in the function, thereby making it harder for them to understand their own code, increasing the chance of a mistake.
• Return a pointer to a non-static local pointer.

Unfortunately, you did both. The trick is to work with a pointer to a node in the function (so you avoid the unnecessary extra level of indirection), then at the end, you dynamically allocate a pointer to a node (hence you store the pointer's address in a pointer to a pointer that you return) and copy over.

In the main function, you would use a pointer to a pointer to store the return value of positive_mtr, but thereafter you will immediately use a pointer to a node as a pointer to the head of the linked list, thereby avoiding having to keep dereferencing a pointer to a pointer.

Quote:

---

Originally Posted by letthem

Unluckly, I checked and there aren't problems with the scanf or with the calloc (they work as they're supposed to work).

---

I can get your scanf call to fail by entering "abc" as input. The point is to check in your program and handle a possible error before proceeding. It doesn't matter if there are no problems when you check; you still have to write the checks in the program to ensure that such errors are detected and handled.

Quote:

---

Originally Posted by letthem

If possible, I would like to avoid to change this functions, because I used them for a lot of projects

---

I'd recommend changing suf_insert to use the same approach of working with pointer to node as much as possible, only using the pointer to pointer when needed: you will find that the implementation becomes simpler and much easier to understand. However, it remains true that you should not use suf_insert to implement positive_mtr, or for any other situation in which you are populating the linked list by appending.

By the way, doubleptr is a terrible name for a parameter. If it is supposed to be a pointer fo a pointer to the head of the linked list, name it head. If it is supposed to be the tail, name it tail. If it can be a pointer to a pointer to an arbitrary node, name it node. Don't name it doubleptr because anyone can tell that's what it is by looking at the type. Same goes for ptrptr.

Here's an example of what I mean by avoiding the pointer to a pointer:

Code:

---

void init_root_node(struct node **head, int value) {
    struct node *node = malloc(sizeof(*node));
    if (!node) {
        *head = NULL;
        return;
    }

    node->value = value;
    node->next = NULL;
    *head = node;
}

---

If you prefer, you could simplify this slightly with:

Code:

---

void init_root_node(struct node **head, int value) {
    struct node *node = malloc(sizeof(*node));
    *head = node;
    if (!node) {
        return;
    }

    node->value = value;
    node->next = NULL;
}

---

Quote:

---

Originally Posted by laserlight

Ah, in this case it is a "trap" set by the exam setter, and you fell right into it. I expect that students would tend to do two things:

• Try to work with a pointer to a pointer all the way in the function, thereby making it harder for them to understand their own code, increasing the chance of a mistake.
• Return a pointer to a non-static local pointer.

Unfortunately, you did both. The trick is to work with a pointer to a node in the function (so you avoid the unnecessary extra level of indirection), then at the end, you dynamically allocate a pointer to a node (hence you store the pointer's address in a pointer to a pointer that you return) and copy over.

---

Amazing Laserlight! That's great.

I already changed everything.
I will have in the next days a conference with a teacher, nothing too important, but the information you gave me will be precious.

However, the fact is that I already sent him the function I wrote.

Code:

---

struct list** positive_mtr (int *A, struct list *point, int r, int c){
        struct list **doub;
        doub = &point;
        init (doub);
        int i,j, value;


        for (i=0;i<r;i++){
            for (j=0;j<c;j++){
                if(A[i*c+j]>0){
                    value= A[i*c+j];
                    suf_insert(doub, value);
                }
            }
        }
        return doub;




}

---

Is there any chance to make this function work in a main?
It's not the correct one, actually it's pretty awful.
But I would like to know if there is any chance to let this work in a main, because theorically there aren't mistakes.