#include only necessary headers
Reduce compilation time.
Stack being a template class should be defined in a header file.
Thus, irrelevant headers would needlessly get inside translation units along with Stack definition.
<iostream> and <vector> are redundant here.
#include <cstdlib> is sufficient for std::malloc and std::free.
Prefer class over struct for complex data structures
struct is useful for aggregate data structures that do not impose constraints on data.
Internal data is exposed to users, and values of mutable members may be independently changed in unpredictable ways.
Data structures featured with user defined implementation constraints (e.g. invariants) must hide implementation details with private access specifier.
Stack is such data structure.
Its data members must be accessed only through carefully designed public methods to maintain constraints that should be initially established by class constructors. In general, if a class has a constructor, one should probably use class.
class Stack {
public:
/* user available methods */
private:
/* hidden implementation details */
int top = 0;
int MAX_SIZE;
T* array;
};
Use simple and meaningful names
Don't repeat yourself.
template<typename T>
class Stack {
public:
/*...*/
int size() const { /* ... */ }
T& top() { /* ... */ }
void clear() { /* ... */ }
/*...*/
};
Restrict names visibility
Note that InsertStatus is not a scoped enum. Placed in global scope of a header file, it may cause name ambiguity problems.
#include "Stack.h"
/* enum InsertStatus { ..., OK = 0 } is available */
/* and OK is accessible as is */
static int OK = -1; // error, can't redefine in global scope
void foo() {
std::string OK{"-1"}; // hides InsertStatus::OK
cout << OK; // prints -1
}
Use scoped enum
enum class InsertStatus { /* ... */ } // separate type
/* ... */
class Stack {
/* ... */
InsertStatus pop() {
/* ... */
return InsertStatus::OK; // OK is accessible only when fully qualified
}
};
or simply put enum in the class scope.
/* ... */
class Stack {
public:
enum InsertStatus { /* ... */ }
/* ... */
private:
/* ... */
};
/* ... */
Stack<int> st(3);
if (st.pop() == Stack::FailedStackEmpty) { /* ... */ }
Stack also should be placed in a namespace to avoid possible name clash.
Use member initializer lists
Use member initializer lists in constructors to avoid redundant in-class member initialization.
class Sentence {
public:
Sentence(const std::string& s) {
/* text was default initialized */
text = s; // initial value is discarded
}
private:
std::string text;
};
With member initializer list, constructor looks like this.
Sentence(const std::string& s)
: text{s} // directly initialize text
{ }
Always test malloc return value
std::malloc returns null pointer if it fails to acquire a block of memory of required size, e.g. if MAX_SIZE is too big.
array = (T*)std::malloc(/* ... */);
if (array == nullptr)
throw std::bad_alloc("Not enough memory");
Report the problem as soon as possible.
Avoid using C-style casts
static_cast, dynamic_cast and reinterpret_cast are conspicuous and functionally limited to prevent cast-related undefined behavior.
array = static_cast<T *>std::malloc(/* ... */);
Establish constraints in constructors
Prevent using invalid objects.
Here, MAX_SIZE must be positive and array must not be null.
Stack(int capacity)
: MAX_SIZE{capacity}
{
assert(MAX_SIZE > 0);
/* ... */
assert(array != nullptr);
}
If throwing exception is not an option, invalid objects may be flagged accordingly.
class Stack {
public:
Stack(int capacity)
: MAX_SIZE{capacity}
{
/* ... */
isValid = array != nullptr;
}
/* ... */
bool valid() const {
return isValid;
}
private:
bool isValid = false;
/* ... */
};
Test your code extensively
Code that uses stackTop method fails to compile because stackTop attempts to assign a value to the const variable t.
It compiles though if stackTop is not used because compiler instantiates only used template methods.
For classes owning resources, define (copy/move) constructors, (copy/move) assignment operators and destructor
Otherwise, some of them (maybe all) will be implicitly generated by the compiler and perform trivial behavior.
Compiler generated (copy/move) constructors and (copy/move) assignment operators do simple memberwise value copy, and destructor does nothing.
Such behavior causes resource (here memory) leaks.
Throughout its lifetime, an object should maintain a valid and comprehensible state
#include "Stack.h"
int main() {
Stack<int> st(3);
st.clearStack();
// st is invalid because memory was freed
st.push(5) // error, undefined behavior
return 0; // st is destructed at the end of lifetime
}
free()probably just makes the memory available for future allocations, but you could link against a debuggingfree()that overwrites, or run your program with Valgrind to help detect such errors. \$\endgroup\$