patterncppMinor
Vector implementation - simple replacement
Viewed 0 times
implementationsimplevectorreplacement
Problem
This is intended to be a simpler replacement of
It should be as fast or faster than
It should also be simple to use.
I am looking for a review on the following:
Here is the header file Vector.hpp
And the implementation Vector.cpp, excluded from the build and included from Vector.hpp since it is a template.
```
template void Vector::AllocSpace()
{
if(Capacity == 0U)
Reserve(2U);
else if(Capacity == Length)
Reserve(Capacity void Vector::DestroyAll()
{
Iterator it = Begin();
Iterator end = End();
while(it ~ItemType();
}
template Vector::Vector() : VecPtr(NULL), Capacity(0U), Length(0U) {}
template Vector::
std::vector, written for entertainment.It should be as fast or faster than
std::vector, but does not need to have as many features.It should also be simple to use.
I am looking for a review on the following:
- Bugs
- Performance
- Class/Functions/Variables Naming
- Useful features to add
- Code structure
- Anything else
Here is the header file Vector.hpp
#pragma once
#include "../Types.hpp"
#include "../System/Memory/Memory.hpp"
namespace Core
{
namespace DataStruct
{
template class Vector
{
ItemType* VecPtr;
UInt Capacity;
UInt Length;
void AllocSpace();
void DestroyAll();
public:
typedef ItemType* Iterator;
typedef ItemType const * ConstIterator;
Vector();
~Vector();
void Reserve(UInt Capacity);
void Add(ItemType const & Value);
void Insert(UInt Position, ItemType const & Value);
void Remove(UInt Position);
void Clear();
void Free();
UInt GetCapacity() const;
UInt GetLength() const;
ItemType operator[](UInt Position) const;
Iterator Begin();
Iterator End();
ConstIterator Begin() const;
ConstIterator End() const;
ConstIterator CBegin() const;
ConstIterator CEnd() const;
};
#include "Vector.cpp"
}
}And the implementation Vector.cpp, excluded from the build and included from Vector.hpp since it is a template.
```
template void Vector::AllocSpace()
{
if(Capacity == 0U)
Reserve(2U);
else if(Capacity == Length)
Reserve(Capacity void Vector::DestroyAll()
{
Iterator it = Begin();
Iterator end = End();
while(it ~ItemType();
}
template Vector::Vector() : VecPtr(NULL), Capacity(0U), Length(0U) {}
template Vector::
Solution
The first and most obvious disadvantage of your
The lack of proper constructors/destructor and assignment operator will cause crashes and leaks every time your
The next obvious drawback of your implementation is that one can only get copies of the elements stored in the
This returns a reference to the stored element instead, so its possible to modify it in place if we have a non-const
Another important drawback is that the
The naming convention is strange to C++, it seems like something brought from a different language, perhaps C#. But the semantics are not the same, so as a result this
You said that This is intended to be a simpler replacement of
Vector is that it does not satisfy the standard requirements for a Sequence. That means that your Vector cannot be used with any of the standard algorithms nor with 3rd party algorithms designed to work with Sequences. It is only useful to store elements, and nothing else.The lack of proper constructors/destructor and assignment operator will cause crashes and leaks every time your
Vector is used. There is a lot to say on that subject, please search for a basic C++ rule known as The Rule of Three which will let you know how you should write those special member functions to achieve the proper semantics.The next obvious drawback of your implementation is that one can only get copies of the elements stored in the
Vector. This not only inconvenient, its also really inefficient. One has to make a copy of the element just to read it; and in order to write it one has to make a copy, modify said copy, remove the element from the Vector and then insert the modified copy. As said, this is both inconvenient and really inefficient. This would be a better approach for your subscript operator:ItemType& operator[](UInt Position){ ... };
ItemType const& operator[](UInt Position) const{ ... };This returns a reference to the stored element instead, so its possible to modify it in place if we have a non-const
Vector.Another important drawback is that the
Vector as implemented will only work with standard-layout types. Simply moving or copying the underlying memory for any other kind of type may result in garbled internal values, broken invariants, or even crashes and undefined behavior.The naming convention is strange to C++, it seems like something brought from a different language, perhaps C#. But the semantics are not the same, so as a result this
Vector will look odd for a C++ coder, and it will seem familiar to a C# coder which would be inclined to think the semantics are the ones from C#.You said that This is intended to be a simpler replacement of
std::vector. But if I were to replace std::vector with Vector the code would not even compile, and even if it would then the observed behavior would greatly differ. As a general rule, when you code in C++ you should stick to C++ conventions. The same applies for any other language as well.Code Snippets
ItemType& operator[](UInt Position){ ... };
ItemType const& operator[](UInt Position) const{ ... };Context
StackExchange Code Review Q#20243, answer score: 3
Revisions (0)
No revisions yet.