Return to Answer

Commonmark migration

edited Jun 10, 2020 at 13:24

Don't import the standard namespace

#Don't import the standard namespace NamespaceNamespace std is a large, and growing, namespace. Do you know every identifier in it? Including the ones to be defined in C++20 and beyond? Bringing all its names into the global namespace not only eliminates the benefits of using namespaces, but also has the potential to silently and subtly change the meaning of your program in future (e.g. by supplying an unambiguously better match for one of your function calls).

Syntax errors

#Syntax errors ThisThis doesn't compile:

Self-initialization

#Self-initialization It'sIt's useless to initialize packet using its own (uninitialized) value here:

Memory leak

#Memory leak I'mI'm not sure how you exercised the test program, but when I ran it with Valgrind, it immediately told me about this leak:

Encapsulation

#Encapsulation Node isn't part of the public interface. If we make it a private struct within BST, then BST gets full access (not needing a friend declaration), but no other code does. That's what we really want.

Flexibility

#Flexibility preorderTraversal() hard-codes the action to take for each node (printing it). What we want is to use the Visitor pattern, where we pass the action as a parameter to the call.

Ease of use

#Ease of use We'veWe've made the interface unnecessarily hard to use, by insisting that packets are passed by reference. This means that the calling code is obliged to ensure that every packet outlives the tree. If packets could be copied/moved to the tree, then it would be much easier for other code to use it.

Clean output

#Clean output IsIs there any reason not to end the output with a newline? It's very annoying when commands leave the next shell prompt dangling halfway across the terminal.

Portability

#Portability Don'tDon't use std::system() if you can avoid it:

#Don't import the standard namespace Namespace std is a large, and growing, namespace. Do you know every identifier in it? Including the ones to be defined in C++20 and beyond? Bringing all its names into the global namespace not only eliminates the benefits of using namespaces, but also has the potential to silently and subtly change the meaning of your program in future (e.g. by supplying an unambiguously better match for one of your function calls).

#Syntax errors This doesn't compile:

#Self-initialization It's useless to initialize packet using its own (uninitialized) value here:

#Memory leak I'm not sure how you exercised the test program, but when I ran it with Valgrind, it immediately told me about this leak:

#Flexibility preorderTraversal() hard-codes the action to take for each node (printing it). What we want is to use the Visitor pattern, where we pass the action as a parameter to the call.

#Ease of use We've made the interface unnecessarily hard to use, by insisting that packets are passed by reference. This means that the calling code is obliged to ensure that every packet outlives the tree. If packets could be copied/moved to the tree, then it would be much easier for other code to use it.

#Clean output Is there any reason not to end the output with a newline? It's very annoying when commands leave the next shell prompt dangling halfway across the terminal.

#Portability Don't use std::system() if you can avoid it:

Don't import the standard namespace

Namespace std is a large, and growing, namespace. Do you know every identifier in it? Including the ones to be defined in C++20 and beyond? Bringing all its names into the global namespace not only eliminates the benefits of using namespaces, but also has the potential to silently and subtly change the meaning of your program in future (e.g. by supplying an unambiguously better match for one of your function calls).

Syntax errors

This doesn't compile:

Self-initialization

It's useless to initialize packet using its own (uninitialized) value here:

Memory leak

I'm not sure how you exercised the test program, but when I ran it with Valgrind, it immediately told me about this leak:

Encapsulation

Node isn't part of the public interface. If we make it a private struct within BST, then BST gets full access (not needing a friend declaration), but no other code does. That's what we really want.

Flexibility

preorderTraversal() hard-codes the action to take for each node (printing it). What we want is to use the Visitor pattern, where we pass the action as a parameter to the call.

Ease of use

We've made the interface unnecessarily hard to use, by insisting that packets are passed by reference. This means that the calling code is obliged to ensure that every packet outlives the tree. If packets could be copied/moved to the tree, then it would be much easier for other code to use it.

Clean output

Is there any reason not to end the output with a newline? It's very annoying when commands leave the next shell prompt dangling halfway across the terminal.

Portability

Don't use std::system() if you can avoid it:

Source Link

answered Jun 18, 2019 at 8:05

Toby Speight

answered Jun 18, 2019 at 8:05

Toby Speight

It's an especially bad practice in a header file, as now you're inflicting the breakage on every single user of that header file, with no way to correct it.

#Syntax errors This doesn't compile:

class BST {
 void BST::insert(Packet& packet);
 void BST::insert(Node* &p, Node *newNode);
};

Remove the extra qualification from the members.

#Self-initialization It's useless to initialize packet using its own (uninitialized) value here:

Node() : packet(packet), rlink(nullptr), llink(nullptr) {}

#Memory leak I'm not sure how you exercised the test program, but when I ran it with Valgrind, it immediately told me about this leak:

==31705== HEAP SUMMARY:
==31705== in use at exit: 144 bytes in 6 blocks
==31705== total heap usage: 20 allocs, 14 frees, 74,208 bytes allocated
==31705=わ=わ 
=わ=わ31705=わ=わ 144 (24 direct, 120 indirect) bytes in 1 blocks are definitely lost in loss record 6 of 6
==31705== at 0x4835DEF: operator new(unsigned long) (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==31705== by 0x10A276: BST::insert(Packet&) (222481.cpp:54)
==31705== by 0x10A744: main (222481.cpp:104)

If we're owning raw pointers, we need to be absolutely sure every new is paired with exactly one delete. It's much better to use the smart pointers provided in <memory> than to try to do this on our own.

#Flexibility preorderTraversal() hard-codes the action to take for each node (printing it). What we want is to use the Visitor pattern, where we pass the action as a parameter to the call.

#Clean output Is there any reason not to end the output with a newline? It's very annoying when commands leave the next shell prompt dangling halfway across the terminal.

#Portability Don't use std::system() if you can avoid it:

sh: 1: pause: not found

Even if such a program was present in my search path, how do you know what function it performs? I'm guessing it's a program that waits forever (like sleep inf on a GNU system). That sounds like a real obstruction to using the test (e.g. it will prevent make test from ever completing successfully). Is that really what's desired?

lang-cpp