Skip to main content
added 570 characters in body
Source Link
Emily L.
  • 16.6k
  • 1
  • 37
  • 88

Some quick comments:

  • Your code style is all over the place. You have three different styles for function names: initialize_tree, PushChildren and getTopParent, pick one and stick to it. I would recommend the last one.
  • Your code looks more like C and not C++ with what I presume to be free functions.
  • item is an inappropriate name, a more typical name is Node.
  • You really should push the functionality to modify the nodes, or tree in general onto the item type and turn it into a class using member functions (methods).
  • You are allocating memory with new but never freeing it with delete thus you are leaking memory like it's 1995.
  • You are handling raw pointers, this is not recommended. Prefer to use the std containers foror smart pointers for managing memory.
  • If you feel that you need to explain something to the reader, that's usually a hint that your code needs better structure. hint
  • std::list is slow, prefer to use std::vector on modern CPUs. Vector is \$\mathcal{O}(n)\$ insertion and removal at the front while list is \$\mathcal{O}(1)\$. However the cost of poor locality of reference on the list means that the cross-over point when the list becomes faster than the vector for just about anything at all is when you have about 100-1000 elements (depending on size) and are hitting the worst case for vector. Which I doubt you won't reach in your program. So unless you have a very specific reason to use std::list prefer a std::vector.

Some quick comments:

  • Your code style is all over the place. You have three different styles for function names: initialize_tree, PushChildren and getTopParent, pick one and stick to it. I would recommend the last one.
  • Your code looks more like C and not C++ with what I presume to be free functions.
  • item is an inappropriate name, a more typical name is Node.
  • You really should push the functionality to modify the nodes, or tree in general onto the item type and turn it into a class using member functions (methods).
  • You are allocating memory with new but never freeing it with delete thus you are leaking memory like it's 1995.
  • You are handling raw pointers, this is not recommended. Prefer to use the std containers for smart pointers for managing memory.
  • If you feel that you need to explain something to the reader, that's usually a hint that your code needs better structure. hint

Some quick comments:

  • Your code style is all over the place. You have three different styles for function names: initialize_tree, PushChildren and getTopParent, pick one and stick to it. I would recommend the last one.
  • Your code looks more like C and not C++ with what I presume to be free functions.
  • item is an inappropriate name, a more typical name is Node.
  • You really should push the functionality to modify the nodes, or tree in general onto the item type and turn it into a class using member functions (methods).
  • You are allocating memory with new but never freeing it with delete thus you are leaking memory like it's 1995.
  • You are handling raw pointers, this is not recommended. Prefer to use the std containers or smart pointers for managing memory.
  • If you feel that you need to explain something to the reader, that's usually a hint that your code needs better structure. hint
  • std::list is slow, prefer to use std::vector on modern CPUs. Vector is \$\mathcal{O}(n)\$ insertion and removal at the front while list is \$\mathcal{O}(1)\$. However the cost of poor locality of reference on the list means that the cross-over point when the list becomes faster than the vector for just about anything at all is when you have about 100-1000 elements (depending on size) and are hitting the worst case for vector. Which I doubt you won't reach in your program. So unless you have a very specific reason to use std::list prefer a std::vector.
Source Link
Emily L.
  • 16.6k
  • 1
  • 37
  • 88

Some quick comments:

  • Your code style is all over the place. You have three different styles for function names: initialize_tree, PushChildren and getTopParent, pick one and stick to it. I would recommend the last one.
  • Your code looks more like C and not C++ with what I presume to be free functions.
  • item is an inappropriate name, a more typical name is Node.
  • You really should push the functionality to modify the nodes, or tree in general onto the item type and turn it into a class using member functions (methods).
  • You are allocating memory with new but never freeing it with delete thus you are leaking memory like it's 1995.
  • You are handling raw pointers, this is not recommended. Prefer to use the std containers for smart pointers for managing memory.
  • If you feel that you need to explain something to the reader, that's usually a hint that your code needs better structure. hint