###Reserved Identifiers:
An identifier with a leading underscore followed by a capitol letter is reserved for use by the system in all scopes.
Thus the following are bad:
inline void _Reserve(MaxSizeType size)
inline void _CopyData(const DataArray& dArray)
inline void _Delete_m_aData()
inline void _Delete_Data(DataType* begin, DataType* end)
###Use constructors to initialize all members
DataArray(MaxSizeType size) :m_iDataSize(0){
_Reserve(size);
}
Looks good. But _Reserve()
does not always initialize all the remaining members!! (If size == 0) the two remaining members are left undefined.
Also doing this is bad practice because you are assuming that somebody that modifies _Reserve()
knows the contract it has with the constructor (ie that it must initialize those two members). The contract is not specified anywhere...
###Dead code should be removed
void reserve(MaxSizeType size){
}
Pass values by reference.
void push_back(DataType element){
/// ^^^ parameter copied
Resizing for more than you need.
if (m_iDataCapacity <= m_iDataSize)
{
_Reserve(m_iDataCapacity + 1);
}
This is very inefficient if you add lots of elements in a row. Once you reach the initial size every time you push back you are reserving more capacity. When you hit the limit reserve more than you need so that you don't have to reserve every time.
Provide const versions of some methods:
DataType& operator[](MaxSizeType id)
There should be two version of this method. This will allow your object to use in const contexts (thus allowing you to build const correct code).
DataType& operator[](MaxSizeType id);
DataType const& operator[](MaxSizeType id) const;
Operator [] is non checking
if (id < 0 || id >= size())
throw std::invalid_argument("Not enought memory");
For efficiency operator[]
does not check bounds (as after testing you have fixed all those bugs). For code where the index is calculated at run time and needs to be validated you usually provide the method at(MaxSizeType id)
.
###That's not the correct exception:
if (id < 0 || id >= size())
throw std::invalid_argument("Not enought memory");
This is an out of range access. There is already an exception specifically for this. Alos it has nothing to do with "enought" memory.
Prefer to use the copy and swap idiom
This assignment constructor is not exception safe.
If _Reserve
throws an exception then the objects data is not in a consistent state. Because you have called the destructor on all the members (but not reset the size of the array and not copied new values into their place).
You should never destroy old data if there is a possibility of failure.
DataType& operator=(const DataArray& dArray)
{
if (this == &dArray)
return *this;
_Delete_m_aData();
_Reserve(dArray.m_iDataCapacity); // If this throws your out of luck.
_CopyData(dArray); // Because this is not called
return *this; // to set the correct members.
}
These look like it should be const methods.
MaxSizeType size(){
return m_iDataSize;
}
MaxSizeType Capacity(){
return m_iDataCapacity;
}
There is a bug in _Reserve.
If this is called from the assignment operator after all the elements have been deleted. Now if there is a realoc failure. The you are calling _Delete_m_aData()
which calls _Delete_Data()
which calls the destructor on the objects again. Even though they have previously been destroyed.
else{
_Delete_m_aData();
throw std::invalid_argument("Not enought memory");
}
###Different types of brace style
Be consistent.
DataArray(const DataArray& dArray)
{
_Reserve(dArray.m_iDataCapacity);
_CopyData(dArray);
}
void reserve(MaxSizeType size){
}
Move Semantics not supported.
Your class does not provide move semantics. That will make your class a whole bunch more efficient it a lot of citations.