Int implementation using linked lists

Question

This is the first piece of code I've written on my own since I started learning a couple of months ago. The code works and it takes an int as input and creates a linked list with each node representing a digit. I've added a function that sums two linked listed ints and returns a third list with the result. The rest of the code is also distributed in functions.

Before I move on, it would be greatly appreciated to know if I'm doing something wrong, if my code is properly structured or if I'm following a good/bad style.

#include <iostream>
using std::cout;
using std::cin;
struct intNode {
 int num;
 intNode *next;
};
int deallocateList(intNode *&s);
int intConversionHelper(int n);
int lenList(intNode *&s);
int sumListContent(intNode *&s);
intNode *sumLists(intNode *&s1, intNode *&s2);
int printNode(intNode *&s);
int intLen(int n);
intNode *intToList(int n);
void append(intNode *&s, int n);
int main() {
 int num1;
 int num2;
 cout << "Give me number 1: ";
 cin >> num1;
 cout << "Give me number 2: ";
 cin >> num2;
 intNode *list1 = intToList(num1);
 intNode *list2 = intToList(num2);
 cout << "\nPrinting ints as linked lists: \n";
 printNode(list1);
 printNode(list2);
 cout << "\nSum of the two lists (as a list too!)\n";
 intNode *sumList = sumLists(list1, list2);
 printNode(sumList);
 deallocateList(list1);
 deallocateList(list2);
 deallocateList(sumList);
}
int printNode(intNode *&s) {
 intNode *tmp = s;
 while (tmp != NULL) {
 cout << tmp->num;
 tmp = tmp->next;
 }
 cout << "\n";
}
int intLen(int n) {
 int len = 1;
 while (n > 9) {
 len++;
 n /= 10;
 }
 return len;
}
intNode *intToList(int n) {
 intNode *intList = NULL;
 int num;
 int dec = 10;
 num = n % 10;
 append(intList, num);
 int count = 1;
 while (count < intLen(n)) {
 num = n / dec % 10; 
 dec = dec * 10;
 append(intList, num);
 count++;
 }
 return intList; 
}
void append(intNode *&s, int n) {
 if (s == NULL) {
 s = new intNode;
 s->num = n;
 s->next = NULL;
 }
 else {
 intNode *nNode = new intNode;
 nNode->num = n;
 nNode->next = s;
 s = nNode;
 }
}
intNode *sumLists(intNode *&s1, intNode *&s2) {
 int sum1 = sumListContent(s1);
 int sum2 = sumListContent(s2);
 return intToList(sum1 + sum2);
}
int sumListContent(intNode *&s) {
 intNode *tmp = s;
 int sum = 0;
 int len = lenList(s);
 while (tmp != NULL) {
 sum += intConversionHelper(len) * tmp->num;
 len--;
 tmp = tmp->next;
 }
 return sum;
}
int lenList(intNode *&s) {
 intNode *tmp = s;
 int len = 0;
 while (tmp != NULL) {
 len++;
 tmp = tmp->next;
 }
 return len;
}
int intConversionHelper(int n) {
 float value = n;
 for (int i = 1; i < n; i++) {
 value = value * 10;
 }
 value = value / n;
 return value;
}
int deallocateList(intNode *&s) {
 intNode *tmp = s;
 while (s != NULL) {
 tmp = tmp->next;
 delete[] s;
 s = tmp;
 }
}

• anon: Does this code work? Your question doesn't make it clear if it does or not.
• James: This is essentially C
• Zeta: I had a quick look, the code should work, except for the delete[] in deleteList (should be delete, since the node gets allocated with new, not new[]). However, you, msaltz, should edit the purpose of your code into your post.

Answer 1

Here are some observations and suggestions that may help you improve your code.

Use objects

The program is written much more in the procedural style of C rather than in the object-oriented style of C++. The list itself could be an object (as could the individual nodes), with most of the procedures as functions of that object. This would reduce coupling and make the program easier to understand. Not least, instead of requiring the user to explicitly call deallocateList(), the object's destructor could automatically be called.

Make sure all paths return a value

Both the printNode() and deallocateList() functions claim to return an int but neither of them actually do. Either return a meaningful int value or change the function signature to void.

Match new and delete

As mentioned in comments to this question, new should be matched with delete and not delete[] which is a different operation.

Use better naming

The function named printNode is not well named. A better name might be printList since it actually prints the entire list and not just a single node.

Avoid using raw pointers

In modern C++, we don't tend to use raw pointers as much as in C. Instead, we either use smart pointers or objects. In this case, I'd say that if you created objects, it would be better to return an object from sumLists() and intToList() rather than a pointer to a node.

Use const where practical

In almost all of your functions, one parameter is a reference to a pointer to an intNode which is the beginning node of a list. In all cases, that list is unmodified by the function and so can (and should) be declared as const.

Understand references and pointers

Almost all of your functions take an intNode *& as an argument. I would highly recommend that you use either pointers or references but not references to pointers.

Use nullptr rather than NULL

Modern C++ uses nullptr rather than NULL. See this answer for why and how it's useful.

Minimize duplication of code

The current append looks like this:

void append(intNode *&s, int n) {
 if (s == NULL) {
 s = new intNode;
 s->num = n;
 s->next = NULL;
 }
 else {
 intNode *nNode = new intNode;
 nNode->num = n;
 nNode->next = s;
 s = nNode;
 }
}

However, most of the code is functionally identical whether s is NULL or not. I'd rewrite it like this:

void append(intNode *&s, int n) {
 s = new intNode{n, s};
}

Avoid pointless numeric conversions

In the intConversionHelper() routine, we inexplicably have a conversion to float which is then converted back to an int when the value is returned. Just use int and avoid the conversions.

Rethink your interface

Right now, only numbers that fit in an int are representable within this code, severely restricting the use of the list and functions such as sumLists. Instead, consider adding digit at a time without converting each list to an int first.