Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

Include guards

Rather than relying on #pragma once, you should use an include guard in your Stack.h file instead, like this:

#ifndef STACK_TYPE_H_
#define STACK_TYPE_H_
// Original code for Stack.h goes here.
#endif

While #pragma once is supported across many compilers, there's always the chance that there is one that doesn't support it because it still isn't standard. If you feel the need to use both #pragma once and include guards, just do something like this:

#ifndef STACK_H_
#define STACK_H_
#pragma once
// Original code for Stack.h goes here.
#endif

Passing by const reference

If you never modify the value of an argument when you pass it, like in the push function:

void push(Type data) {
 ...
}

Rather than passing the value normally, you should pass it by const reference, like this:

void push(const Type& data) 
{
 ...
}

While this is a micro-optimization for "small" types, like int, or bool, when you start dealing with "bigger" types, like std::string, or a user-defined type, doing this becomes a little more important.


Nitpicks

While your test file containing main isn't such a huge deal, there are still a few bad things I want to point out about it.

First off, this line:

using namespace std;

Is as really bad habit to get into. It can also result in many bad things happening, especially if you're using Boost, which provides alternatives to some of the functions in std. Some of the bad things that can happen can be found here here.

This line here should be removed as well:

system("PAUSE");

Preferably if you need to display output for the user to see, you should use something like this instead:

std::cin.get();

system("PAUSE"); is bad, for the following reasons:

  • It's slow and un-optimal.
  • It's insecure.
  • It's very platform-dependent.

Preferably, as it is the C++ style, you should define you class and it's function/constructor signatures inside Stack.h, and then create a file named Stack.cpp where you implement the function/constructors.

Finally, your isEmpty function should be const, since it isn't modifying anything. That means that this:

bool isEmpty() {
 return length == 0;
}

Should become this:

bool isEmpty() const {
 return length == 0;
}

Include guards

Rather than relying on #pragma once, you should use an include guard in your Stack.h file instead, like this:

#ifndef STACK_TYPE_H_
#define STACK_TYPE_H_
// Original code for Stack.h goes here.
#endif

While #pragma once is supported across many compilers, there's always the chance that there is one that doesn't support it because it still isn't standard. If you feel the need to use both #pragma once and include guards, just do something like this:

#ifndef STACK_H_
#define STACK_H_
#pragma once
// Original code for Stack.h goes here.
#endif

Passing by const reference

If you never modify the value of an argument when you pass it, like in the push function:

void push(Type data) {
 ...
}

Rather than passing the value normally, you should pass it by const reference, like this:

void push(const Type& data) 
{
 ...
}

While this is a micro-optimization for "small" types, like int, or bool, when you start dealing with "bigger" types, like std::string, or a user-defined type, doing this becomes a little more important.


Nitpicks

While your test file containing main isn't such a huge deal, there are still a few bad things I want to point out about it.

First off, this line:

using namespace std;

Is as really bad habit to get into. It can also result in many bad things happening, especially if you're using Boost, which provides alternatives to some of the functions in std. Some of the bad things that can happen can be found here.

This line here should be removed as well:

system("PAUSE");

Preferably if you need to display output for the user to see, you should use something like this instead:

std::cin.get();

system("PAUSE"); is bad, for the following reasons:

  • It's slow and un-optimal.
  • It's insecure.
  • It's very platform-dependent.

Preferably, as it is the C++ style, you should define you class and it's function/constructor signatures inside Stack.h, and then create a file named Stack.cpp where you implement the function/constructors.

Finally, your isEmpty function should be const, since it isn't modifying anything. That means that this:

bool isEmpty() {
 return length == 0;
}

Should become this:

bool isEmpty() const {
 return length == 0;
}

Include guards

Rather than relying on #pragma once, you should use an include guard in your Stack.h file instead, like this:

#ifndef STACK_TYPE_H_
#define STACK_TYPE_H_
// Original code for Stack.h goes here.
#endif

While #pragma once is supported across many compilers, there's always the chance that there is one that doesn't support it because it still isn't standard. If you feel the need to use both #pragma once and include guards, just do something like this:

#ifndef STACK_H_
#define STACK_H_
#pragma once
// Original code for Stack.h goes here.
#endif

Passing by const reference

If you never modify the value of an argument when you pass it, like in the push function:

void push(Type data) {
 ...
}

Rather than passing the value normally, you should pass it by const reference, like this:

void push(const Type& data) 
{
 ...
}

While this is a micro-optimization for "small" types, like int, or bool, when you start dealing with "bigger" types, like std::string, or a user-defined type, doing this becomes a little more important.


Nitpicks

While your test file containing main isn't such a huge deal, there are still a few bad things I want to point out about it.

First off, this line:

using namespace std;

Is as really bad habit to get into. It can also result in many bad things happening, especially if you're using Boost, which provides alternatives to some of the functions in std. Some of the bad things that can happen can be found here.

This line here should be removed as well:

system("PAUSE");

Preferably if you need to display output for the user to see, you should use something like this instead:

std::cin.get();

system("PAUSE"); is bad, for the following reasons:

  • It's slow and un-optimal.
  • It's insecure.
  • It's very platform-dependent.

Preferably, as it is the C++ style, you should define you class and it's function/constructor signatures inside Stack.h, and then create a file named Stack.cpp where you implement the function/constructors.

Finally, your isEmpty function should be const, since it isn't modifying anything. That means that this:

bool isEmpty() {
 return length == 0;
}

Should become this:

bool isEmpty() const {
 return length == 0;
}
added 10 characters in body
Source Link
Ethan Bierlein
  • 15.9k
  • 4
  • 60
  • 146

Include guards

Rather than relying on #pragma once, you should use an include guard in your Stack.h file instead, like this:

#ifndef STACK_H_STACK_TYPE_H_
#define STACK_H_STACK_TYPE_H_
// Original code for Stack.h goes here.
#endif

While #pragma once is supported across many compilers, there's always the chance that there is one that doesn't support it because it still isn't standard. If you feel the need to use both #pragma once and include guards, just do something like this:

#ifndef STACK_H_
#define STACK_H_
#pragma once
// Original code for Stack.h goes here.
#endif

Passing by const reference

If you never modify the value of an argument when you pass it, like in the push function:

void push(Type data) {
 ...
}

Rather than passing the value normally, you should pass it by const reference, like this:

void push(const Type& data) 
{
 ...
}

While this is a micro-optimization for "small" types, like int, or bool, when you start dealing with "bigger" types, like std::string, or a user-defined type, doing this becomes a little more important.


Nitpicks

While your test file containing main isn't such a huge deal, there are still a few bad things I want to point out about it.

First off, this line:

using namespace std;

Is as really bad habit to get into. It can also result in many bad things happening, especially if you're using Boost, which provides alternatives to some of the functions in std. Some of the bad things that can happen can be found here.

This line here should be removed as well:

system("PAUSE");

Preferably if you need to display output for the user to see, you should use something like this instead:

std::cin.get();

system("PAUSE"); is bad, for the following reasons:

  • It's slow and un-optimal.
  • It's insecure.
  • It's very platform-dependent.

Preferably, as it is the C++ style, you should define you class and it's function/constructor signatures inside Stack.h, and then create a file named Stack.cpp where you implement the function/constructors.

Finally, your isEmpty function should be const, since it isn't modifying anything. That means that this:

bool isEmpty() {
 return length == 0;
}

Should become this:

bool isEmpty() const {
 return length == 0;
}

Include guards

Rather than relying on #pragma once, you should use an include guard in your Stack.h file instead, like this:

#ifndef STACK_H_
#define STACK_H_
// Original code for Stack.h goes here.
#endif

While #pragma once is supported across many compilers, there's always the chance that there is one that doesn't support it because it still isn't standard. If you feel the need to use both #pragma once and include guards, just do something like this:

#ifndef STACK_H_
#define STACK_H_
#pragma once
// Original code for Stack.h goes here.
#endif

Passing by const reference

If you never modify the value of an argument when you pass it, like in the push function:

void push(Type data) {
 ...
}

Rather than passing the value normally, you should pass it by const reference, like this:

void push(const Type& data) 
{
 ...
}

While this is a micro-optimization for "small" types, like int, or bool, when you start dealing with "bigger" types, like std::string, or a user-defined type, doing this becomes a little more important.


Nitpicks

While your test file containing main isn't such a huge deal, there are still a few bad things I want to point out about it.

First off, this line:

using namespace std;

Is as really bad habit to get into. It can also result in many bad things happening, especially if you're using Boost, which provides alternatives to some of the functions in std. Some of the bad things that can happen can be found here.

This line here should be removed as well:

system("PAUSE");

Preferably if you need to display output for the user to see, you should use something like this instead:

std::cin.get();

system("PAUSE"); is bad, for the following reasons:

  • It's slow and un-optimal.
  • It's insecure.
  • It's very platform-dependent.

Preferably, as it is the C++ style, you should define you class and it's function/constructor signatures inside Stack.h, and then create a file named Stack.cpp where you implement the function/constructors.

Finally, your isEmpty function should be const, since it isn't modifying anything. That means that this:

bool isEmpty() {
 return length == 0;
}

Should become this:

bool isEmpty() const {
 return length == 0;
}

Include guards

Rather than relying on #pragma once, you should use an include guard in your Stack.h file instead, like this:

#ifndef STACK_TYPE_H_
#define STACK_TYPE_H_
// Original code for Stack.h goes here.
#endif

While #pragma once is supported across many compilers, there's always the chance that there is one that doesn't support it because it still isn't standard. If you feel the need to use both #pragma once and include guards, just do something like this:

#ifndef STACK_H_
#define STACK_H_
#pragma once
// Original code for Stack.h goes here.
#endif

Passing by const reference

If you never modify the value of an argument when you pass it, like in the push function:

void push(Type data) {
 ...
}

Rather than passing the value normally, you should pass it by const reference, like this:

void push(const Type& data) 
{
 ...
}

While this is a micro-optimization for "small" types, like int, or bool, when you start dealing with "bigger" types, like std::string, or a user-defined type, doing this becomes a little more important.


Nitpicks

While your test file containing main isn't such a huge deal, there are still a few bad things I want to point out about it.

First off, this line:

using namespace std;

Is as really bad habit to get into. It can also result in many bad things happening, especially if you're using Boost, which provides alternatives to some of the functions in std. Some of the bad things that can happen can be found here.

This line here should be removed as well:

system("PAUSE");

Preferably if you need to display output for the user to see, you should use something like this instead:

std::cin.get();

system("PAUSE"); is bad, for the following reasons:

  • It's slow and un-optimal.
  • It's insecure.
  • It's very platform-dependent.

Preferably, as it is the C++ style, you should define you class and it's function/constructor signatures inside Stack.h, and then create a file named Stack.cpp where you implement the function/constructors.

Finally, your isEmpty function should be const, since it isn't modifying anything. That means that this:

bool isEmpty() {
 return length == 0;
}

Should become this:

bool isEmpty() const {
 return length == 0;
}
deleted 218 characters in body
Source Link
Ethan Bierlein
  • 15.9k
  • 4
  • 60
  • 146

It's also worth noting that your code is clearer when you place your curly braces like this:

something
{
}

Not like this:

something {
}

This isn't required though, just a tip.

Finally, your isEmpty function should be const, since it isn't modifying anything. That means that this:

It's also worth noting that your code is clearer when you place your curly braces like this:

something
{
}

Not like this:

something {
}

This isn't required though, just a tip.

Finally, your isEmpty function should be const, since it isn't modifying anything. That means that this:

Finally, your isEmpty function should be const, since it isn't modifying anything. That means that this:

added 49 characters in body
Source Link
Ethan Bierlein
  • 15.9k
  • 4
  • 60
  • 146
Loading
added 560 characters in body
Source Link
Ethan Bierlein
  • 15.9k
  • 4
  • 60
  • 146
Loading
added 139 characters in body
Source Link
Ethan Bierlein
  • 15.9k
  • 4
  • 60
  • 146
Loading
Source Link
Ethan Bierlein
  • 15.9k
  • 4
  • 60
  • 146
Loading
lang-cpp

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