Hackerrank delete a node

Question

I have been learning c++ for a few weeks now and I attempted this problem on Hackerrank Delete a node. I am aware of a similar post Delete a linked list node but the code seems more of a C structure than a C++ structure. Please feel free to suggest areas of Improvement and shorter way of writing this if any exists. Here is excerpt of the question

You’re given the pointer to the head node of a linked list and the position of a node to delete. Delete the node at the given position and return the head node. A position of 0 indicates head, a position of 1 indicates one node away from the head and so on. The list may become empty after you delete the node.

Input Format You have to complete the Node* Delete(Node* head, int position) method which takes two arguments - the head of the linked list and the position of the node to delete. You should NOT read any input from stdin/console. The position will always be at least 0 and less than the number of the elements in the list.

Output Format Delete the node at the given position and return the head of the updated linked list. Do NOT print anything to stdout/console.

Sample Input

1 --> 2 --> 3 --> NULL, position = 0 1 --> NULL , position = 0

Sample Output

2 --> 3 --> NULL NULL

/*
 Delete Node at a given position in a linked list 
 Node is defined as 
 struct Node
 {
 int data;
 struct Node *next;
 }
*/
Node* Delete(Node *head, int position)
{
 // Complete this method
 Node *temp = head;
 if(position == 0){
 head = head->next; 
 return head;
 }
 Node *temp2 = head;
 for(int i = 0 ; i < position-1; i++){
 temp2 = temp2->next; 
 }
 temp = temp2->next->next;
 temp2->next = temp; 
 return head;
}

• CiaPan: See Singly Linked List Implementation in C

Answer 1

A few comments:

1. It is cleaner to define the position parameter as unsigned or std::size_t as it cannot be negative. Same applies to the for loop counter.
2. There is a memory leak, as you don't do delete on the removed node.
3. Names like temp and temp2 are not reader-friendly. How about current_node? Speaking of which, the assignment to temp2 is redundant.
4. The code should validate that we don't go beyond the end of the list. We can of course assume that invalid index cannot be passed to the function, but it's a dangerous assumption that does not lead to defensive code.
5. While we're on the parameter validation topic; if we get passed an empty list (i.e. head == NULL), there will again be undefined behaviour irrespective of the value of position.
6. If we delete the last node, the code will behave in an undefined manner (most likely crash). Consider that temp2->next will be NULL in that case, which means that temp2->next->next won't be a happy statement.
7. Speaking of last comment, do you have unit tests in place? If so, it'd be good to have edge cases covered (delete first node, delete last node, empty list etc.)

Hope this helps.

Answer 2

For the "determining the node from the position" part, you can just do this:

Node *Delete(Node *head, int position) {
 Node *node = head;
 while(position--) node = node->next;
}

Then in RL, I would probably have a "remove specified node from list" function, and just pass "node" to that.