Simple singly linked list implementation in C

I have implemented a LinkedList with the ability to add, get elements from a particular position and ability to show all the elements in the array in to the console (main purpose to implement this was to ensure that all the elements are being populated in the LinkedList correctly).

I would highly appreciate comments based on:

• The efficiency of my code

• Conceptual misinterpretations of a linked list, if any

• Violations of best practices

• Better way of achieving this

This is the code I have written:

```
#include
#include

struct Element {
int value;
struct Element *nextElement;
};

struct LinkedList {
struct Element *firstElement;
};

void add(struct LinkedList *list, int value) {
if(list->firstElement == NULL) {
struct Element *newElement;
newElement = malloc(sizeof(struct Element));

newElement->value = value;
newElement->nextElement = NULL;

list->firstElement = newElement;
} else {
struct Element *lastElement;

lastElement = list->firstElement;

while(lastElement->nextElement != NULL) {
lastElement = lastElement->nextElement;
}

struct Element *newElement;
newElement = malloc(sizeof(struct Element));

newElement->value = value;
newElement->nextElement = NULL;

lastElement->nextElement = newElement;
}
}

int getElement(struct LinkedList *list, int index) {
int iteratedIndex = 0;
int returnValue = 0;

struct Element *temporaryElement = list->firstElement;

while(temporaryElement != NULL) {
if(iteratedIndex == index) {
returnValue = temporaryElement->value;
return returnValue;
}

iteratedIndex = iteratedIndex + 1;
temporaryElement = temporaryElement->nextElement;
}

return 0;
}

void showAllElements(struct LinkedList *list) {
struct Element *lastElement = list->firstElement;

while(lastElement != NULL) {

In terms for efficiency and concept, the implementation is fine.
It lacks the more interesting functionality such as deleting and inserting items.

The most important missing element is deleting the linked list itself:
when you allocate memory using malloc,
it's important to add functions to clean that up at some point.

Code duplication

In addElement,
the logic of creating a new element is duplicated twice.
This could be easily avoided by moving that logic to another function, for example:

struct Element * createElement(int value) {
    struct Element *newElement;
    newElement = malloc(sizeof(struct Element));

    newElement->value = value;
    newElement->nextElement = NULL;
    return newElement;
}

On closer look,
you didn't even really need an extra method,
you could eliminate the duplicated logic by moving the element creation before the if condition:

void add(struct LinkedList *list, int value) {
    struct Element *newElement = createElement(value);
    if(list->firstElement == NULL) {
        list->firstElement = newElement;
    } else {
        // ...
        lastElement->nextElement = newElement;
    }
}

Naming

Most of the names are good and nicely descriptive.
But there are a few exceptions:

• getElement: despite its name, this function returns an int, not an Element. So getValue would be better.

• lastElement as loop is not so great, as it's typically not referencing the last element. node would be a more common name for the purpose

Trivial simplifications

Some of the functions can be simplified very easily.

For example,
in getElement you really don't need the returnValue variable.
You assign to it right before you return it,
and that's the only use.
So avoid the pointless local variable.
Every variable has the potential of getting misused.
So if a variable has no good purpose to serve, then don't use it.

Another minor point is that instead of iteratedIndex = iteratedIndex + 1 you can write simply ++iteratedIndex.

StackExchange Code Review Q#110165, answer score: 4