Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

The point of the include guards is to prevent multiple inclusion of anything. So always put them at the top of the file:

#include <vector>
#include <list>
#ifndef GRAPH_IMPL
#define GRAPH_IMPL

Because these are not at the top you include vector/list every time. This is just more work that does not need to be done.

Never put using statements in a header file (in global context).

using std::vector;
using std::list;

Anybody that uses your header file now implicitly pulls vector/list into the global namespace. This can have undesirable effects with name resolution. Don't do this in a header file. If you must be lazy then use scope so you are not forcing your requirements onto others. There is always a typedef to make things easier.

Comments (or well named members) would be an idea.

 struct node
 {
 int v,w;
 };

No idea what that will be used for.
If the user of your code library should not be seeing this then it should also be contained inside the private part of a classs so that they don't accidentally use it.

vertex_impl

  • Not sure what vertex_impl is for?
  • Is it exposed externally?
  • What is it implementing (its name contains impl)?
  • Why can it have more than two nodes (To me a vertex is an edge that connects two nodes)?
  • Why does the constructor not fully initialize the object (index is set externally).
  • What is masking for?
  • What is index for?

When I build objects that contain containers I abstract the container used:

list<node> adj;

Here I would have gone:

typedef std::list<node> Container;
Container adj;

Then I would derive the other iterator types from Container. This means if the code changes there is only one location where the code needs to change.

Having an explicit rbegin() and rend() implies that there is some ordering to the nodes. personally I would leave them out as their does not seem to be any way to logically define the order of traversal of a graph. If the user wants to iterate in the reverse order then can use std::reverse_iterator.

###graph

graph

Basically the same comments as vertex_impl

The point of the include guards is to prevent multiple inclusion of anything. So always put them at the top of the file:

#include <vector>
#include <list>
#ifndef GRAPH_IMPL
#define GRAPH_IMPL

Because these are not at the top you include vector/list every time. This is just more work that does not need to be done.

Never put using statements in a header file (in global context).

using std::vector;
using std::list;

Anybody that uses your header file now implicitly pulls vector/list into the global namespace. This can have undesirable effects with name resolution. Don't do this in a header file. If you must be lazy then use scope so you are not forcing your requirements onto others. There is always a typedef to make things easier.

Comments (or well named members) would be an idea.

 struct node
 {
 int v,w;
 };

No idea what that will be used for.
If the user of your code library should not be seeing this then it should also be contained inside the private part of a classs so that they don't accidentally use it.

vertex_impl

  • Not sure what vertex_impl is for?
  • Is it exposed externally?
  • What is it implementing (its name contains impl)?
  • Why can it have more than two nodes (To me a vertex is an edge that connects two nodes)?
  • Why does the constructor not fully initialize the object (index is set externally).
  • What is masking for?
  • What is index for?

When I build objects that contain containers I abstract the container used:

list<node> adj;

Here I would have gone:

typedef std::list<node> Container;
Container adj;

Then I would derive the other iterator types from Container. This means if the code changes there is only one location where the code needs to change.

Having an explicit rbegin() and rend() implies that there is some ordering to the nodes. personally I would leave them out as their does not seem to be any way to logically define the order of traversal of a graph. If the user wants to iterate in the reverse order then can use std::reverse_iterator.

###graph

Basically the same comments as vertex_impl

The point of the include guards is to prevent multiple inclusion of anything. So always put them at the top of the file:

#include <vector>
#include <list>
#ifndef GRAPH_IMPL
#define GRAPH_IMPL

Because these are not at the top you include vector/list every time. This is just more work that does not need to be done.

Never put using statements in a header file (in global context).

using std::vector;
using std::list;

Anybody that uses your header file now implicitly pulls vector/list into the global namespace. This can have undesirable effects with name resolution. Don't do this in a header file. If you must be lazy then use scope so you are not forcing your requirements onto others. There is always a typedef to make things easier.

Comments (or well named members) would be an idea.

 struct node
 {
 int v,w;
 };

No idea what that will be used for.
If the user of your code library should not be seeing this then it should also be contained inside the private part of a classs so that they don't accidentally use it.

vertex_impl

  • Not sure what vertex_impl is for?
  • Is it exposed externally?
  • What is it implementing (its name contains impl)?
  • Why can it have more than two nodes (To me a vertex is an edge that connects two nodes)?
  • Why does the constructor not fully initialize the object (index is set externally).
  • What is masking for?
  • What is index for?

When I build objects that contain containers I abstract the container used:

list<node> adj;

Here I would have gone:

typedef std::list<node> Container;
Container adj;

Then I would derive the other iterator types from Container. This means if the code changes there is only one location where the code needs to change.

Having an explicit rbegin() and rend() implies that there is some ordering to the nodes. personally I would leave them out as their does not seem to be any way to logically define the order of traversal of a graph. If the user wants to iterate in the reverse order then can use std::reverse_iterator.

graph

Basically the same comments as vertex_impl

Source Link
Loki Astari
  • 97.7k
  • 5
  • 126
  • 341

The point of the include guards is to prevent multiple inclusion of anything. So always put them at the top of the file:

#include <vector>
#include <list>
#ifndef GRAPH_IMPL
#define GRAPH_IMPL

Because these are not at the top you include vector/list every time. This is just more work that does not need to be done.

Never put using statements in a header file (in global context).

using std::vector;
using std::list;

Anybody that uses your header file now implicitly pulls vector/list into the global namespace. This can have undesirable effects with name resolution. Don't do this in a header file. If you must be lazy then use scope so you are not forcing your requirements onto others. There is always a typedef to make things easier.

Comments (or well named members) would be an idea.

 struct node
 {
 int v,w;
 };

No idea what that will be used for.
If the user of your code library should not be seeing this then it should also be contained inside the private part of a classs so that they don't accidentally use it.

vertex_impl

  • Not sure what vertex_impl is for?
  • Is it exposed externally?
  • What is it implementing (its name contains impl)?
  • Why can it have more than two nodes (To me a vertex is an edge that connects two nodes)?
  • Why does the constructor not fully initialize the object (index is set externally).
  • What is masking for?
  • What is index for?

When I build objects that contain containers I abstract the container used:

list<node> adj;

Here I would have gone:

typedef std::list<node> Container;
Container adj;

Then I would derive the other iterator types from Container. This means if the code changes there is only one location where the code needs to change.

Having an explicit rbegin() and rend() implies that there is some ordering to the nodes. personally I would leave them out as their does not seem to be any way to logically define the order of traversal of a graph. If the user wants to iterate in the reverse order then can use std::reverse_iterator.

###graph

Basically the same comments as vertex_impl

lang-cpp

AltStyle によって変換されたページ (->オリジナル) /