2
\$\begingroup\$

Please critique the following code on removing a node from a singly linked list, at the moment it seems to me like the code is quite clunky.

typedef struct Node {
    int data;
    struct Node* next;
}Node;

Node* create(int A[], int n){
    Node *head = malloc(sizeof(Node));
    Node *temp = NULL;
    Node *tail;
    head->data = A[0];
    head->next = NULL;
    tail = head;
    int i;
    for (i = 1; i < n; i++){
        temp = malloc(sizeof(Node));
        temp->data = A[i];
        temp->next = NULL;
        tail->next = temp;
        tail = temp;
    }
    return head;
}

void delete(Node** head, int index) {
    Node* temp;
    Node* curr;
    Node* prev = NULL;
    curr = *head;
    int pos;
    if (*head == NULL) {
        return;
    }
    if (index == 0) {
        temp = curr->next;
        *head = temp;
        free(curr);
    }
    else {
        for (pos = 0; pos < index; pos++) {
            prev = curr;
            curr = curr->next;
        }
        prev->next = curr->next;

        free(curr);
    }


}

int main(){
    int A[] = {3, 5, 7, 9, 10};
    Node *head = create(A, 5);
    delete(&head, 1);
    return 0;
   }

\$\endgroup\$
0

2 Answers 2

6
\$\begingroup\$
  • Mark variables that will remain constant as const.
  • Initialise variables right away when they're created, if possible. You're just adding one extra line with no benefit.
  • Limit the scope of variables by creating them only when they're required, not at the top. They'll go unused in certain conditions. Also one won't have to sanity check the whole function to see if the variable has been modified since it was created.
  • Use a more descriptive function name. del_node_at_index is even better.
void delete_node(Node **head, const int index)
{
  // Initialise curr when it's defined. 
  Node *curr = *head;

  if (head == NULL) {
    return;
  }
  if (index == 0) {
    // Keep the scope of temp limited.
    // Initialise temp when it's defined.
    Node *temp = curr->next;
    *head = temp;
    free(curr);
  }
  else {
    // Keep the scope of prev limited. 
    Node *prev;
    // Define pos if and when needed.
    for (int pos = 0; pos < index; pos++) { 
      prev = curr;
      curr = curr->next;
    }
    prev->next = curr->next;

    free(curr);
  }
}
\$\endgroup\$
5
  • 1
    \$\begingroup\$ What does your code for 'delete' do if index is larger than the number of items in the list??? If it is negative? \$\endgroup\$ Jul 5, 2020 at 20:57
  • \$\begingroup\$ yes i have to fix that too \$\endgroup\$
    – r4bb1t
    Jul 5, 2020 at 21:36
  • \$\begingroup\$ @user1666959 I suppose that is a question for PyWalker27 (which they replied to) . I didn't check the correctness of the algorithm. \$\endgroup\$
    – aki
    Jul 6, 2020 at 8:40
  • \$\begingroup\$ @PyWalker27 Is the index a one-based indexing or zero-based ? i.e.: if you want to delete the first node, would you pass 0 or 1 as the index argument ? I'd suggest keeping it zero-based. \$\endgroup\$
    – aki
    Jul 6, 2020 at 8:41
  • \$\begingroup\$ I would pass 0. \$\endgroup\$
    – r4bb1t
    Jul 6, 2020 at 8:55
5
\$\begingroup\$

@akki covered most of what can be done, however, the best reason to rename the function is that C can be easily ported to C++, and delete is a key word in the C++ language. Avoid using C++ key words in C when possible.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ oo i didn't notice that! \$\endgroup\$
    – r4bb1t
    Jul 5, 2020 at 19:57
  • 1
    \$\begingroup\$ @PyWalker27 that's what code reviews are for. \$\endgroup\$
    – pacmaninbw
    Jul 5, 2020 at 20:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.