ResidentBiscuit wrote: |
---|
"Go judge the hell out of it" |
With pleasure :)
General Programming Practices and Design Choices
1) In "
MyVector::MyVector( )" and "
MyVector::MyVector( int capacity )", you're doing something wrong: you're setting the capacity before allocating the memory for the vector, but what happens when the allocation fails? You're going to be left with misleading information, specifically, "
_capacity" will have a valid value but no corresponding memory as "
MyVector::data" will be null. It would be best to set the capacity upon allocation success.
2) Again, just like the above point, "
MyVector::reallocate( )" suffers the same issue.
3) In function "
MyVector::get( )", you're not validating "
index"; whether this was intended I don't know because of the lack of documentation. Also, why not make "
index" constant? You seem to qualify "
index" of "
MyVector::operator []( )" with "
const".
4) The constant "
MyVector::operator []( )" is useless. If a "
MyVector" was declared constant, you wouldn't be able to add or remove any elements anyway. Therefore, the state of the object wouldn't change so there would be no need to access the elements because they'd never change anyway.
5) In regards to "
MyVector::capacity( )", two things are on my mind: why is the function non-constant? and why are you not ensuring that the capacity is valid? As I said in the first point, if an allocation fails, there's no memory and therefore is no capacity. This misleading information can lead to devastating situations.
6) In function "
MyVector::empty( )", a simple equality check would fit nicely -- the amount of "
if" statements is too excessive.
Efficiency: My Favourite Field
1) There's no explicit in-lining. Some of the function implementations are good candidates for in-lining, but you fail to even reference in-lining.
2) Your method of memory allocation is interesting given the design. "
::new T[n]" allocates a group of contiguous objects and then constructs each one if the allocation succeeds. However, because you're implementing a container, this isn't exactly a smart choice for one reason:
"
T" may well be a constant type. In order for your implementation to work, "
T" must be copy-assignable. With your allocation method, all constant objects would've been constructed at the point of allocation and any future modifications to the elements after that point would be in violation of the "
const" qualifier. Oops!
3) Allocating memory is a time-consuming process. Because your initial capacity is so low, given the usage, re-allocations would be a frequent occurrence, thereby reducing the overall speed of the vector. I would suggest upping the value, but the implementation is too general to give a suitable suggestion.
4) In function "
MyVector::reallocate( )", there's room for optimisation: the loop's body. A faster approach would be using pointers and to avoid using "
X[n + 1]".
5) If "
T" was a scalar type, you'd be losing some performance due to passing-by-reference. Passing scalar types by reference decreases performance. There's no attempt to rectify this issue.
6) Back in "
MyVector::empty( )" swapping the body with a single equality check will reduce the amount of branch predictions.
[Edit]To all who suggest the use of "
std::malloc( )":
If you're going to suggest the use of "
std::malloc( )", please make sure that you mention the possibility that "
std::malloc( )" will return a misaligned pointer and that it needs to be addressed. Also, please prefer the "
new" alternative instead:
1 2
|
void *Raw_Memory_( ::operator new( 30 * sizeof( double ) ) );
::operator delete( Raw_Memory_ );
|
Wazzak