patterncppMinor
Int implementation using linked lists
Viewed 0 times
linkedusinglistsimplementationint
Problem
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
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
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 > num1;
cout > num2;
intNode *list1 = intToList(num1);
intNode *list2 = intToList(num2);
cout num;
tmp = tmp->next;
}
cout 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 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)
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
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 > num1;
cout > num2;
intNode *list1 = intToList(num1);
intNode *list2 = intToList(num2);
cout num;
tmp = tmp->next;
}
cout 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 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)
Solution
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
Make sure all paths return a value
Both the
Match
As mentioned in comments to this question,
Use better naming
The function named
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
Use
In almost all of your functions, one parameter is a reference to a pointer to an
Understand references and pointers
Almost all of your functions take an
Use
Modern C++ uses
Minimize duplication of code
The current
However, most of the code is functionally identical whether
Avoid pointless numeric conversions
In the
Rethink your interface
Right now, only numbers that fit in an
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 deleteAs 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 practicalIn 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 NULLModern 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.Code Snippets
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;
}
}void append(intNode *&s, int n) {
s = new intNode{n, s};
}Context
StackExchange Code Review Q#161266, answer score: 6
Revisions (0)
No revisions yet.