patterncppModerate
Vector implementation, which received unhelpful negative feedback from professor
Viewed 0 times
fromunhelpfulreceivednegativewhichimplementationfeedbackprofessorvector
Problem
I've just finished a homework assignment to implement a simple Vector class.
-Kindly ignore the following two paragraphs if you don't want to read someone whining about their professor
Yadda, yadda, same old stuff. Here's where it gets interesting though; My professor is a bit... strange in his preferences. He's a retired cabinet maker who learned programming in his 50's, and decided that teaching at a local Community College would be a great way to make a decent amount of money without working.
I'm a 'do your own research' kinda guy, and the more I read (here on stackexchange, as well as in various paper books), the more I notice that some of his instructions/requirements are suboptimal to say the least. For example:
This is where you kind folks come in. I really, really want to learn how to be a solid programmer. I aspire to one day have header files void of any
-Whining is over, relevant information starts here.
The assignment was to implement a Vector class using the function definitions that my professor provided. Unfortunately, I can't change those and I'm also unable to add any public functions that were not included in his list of definitions. Additionally, I'm not allowed to
#ifndef _VECTOR_H_
#define _VECTOR_H_
#include
template class Vector
{
int _index_offset;
u
-Kindly ignore the following two paragraphs if you don't want to read someone whining about their professor
Yadda, yadda, same old stuff. Here's where it gets interesting though; My professor is a bit... strange in his preferences. He's a retired cabinet maker who learned programming in his 50's, and decided that teaching at a local Community College would be a great way to make a decent amount of money without working.
I'm a 'do your own research' kinda guy, and the more I read (here on stackexchange, as well as in various paper books), the more I notice that some of his instructions/requirements are suboptimal to say the least. For example:
using namespace std in every header file he requires us to implement without modifying. Typical feedback on my assignments have been "too many errors to list" and "This code is... odd." I've been getting A's on exams, and solid 70% C's on my assignments. Unfortunately, he doesn't allow us to email him regarding assignments and I'm a full-time student/full-time fast-food worker, so his 5 office-hours per week are really hard to pencil into my schedule.This is where you kind folks come in. I really, really want to learn how to be a solid programmer. I aspire to one day have header files void of any
using namespace whatsoever. That's why I'd love some input into how I can improve this particular assignment, as well as good practices in general that I can adopt.-Whining is over, relevant information starts here.
The assignment was to implement a Vector class using the function definitions that my professor provided. Unfortunately, I can't change those and I'm also unable to add any public functions that were not included in his list of definitions. Additionally, I'm not allowed to
#include anything besides `.
My Vector.h is as follows:
``#ifndef _VECTOR_H_
#define _VECTOR_H_
#include
template class Vector
{
int _index_offset;
u
Solution
I understand that some of this class is predicated by your professor; However I will review the code as a whole and you can then choose what can be changed and what cannot be changed.
Interface/High-level comments
Use of leading underscore
The C and C++ standards reserve the use of certain patterns of leading underscores for implementation purposes. If you use leading underscore in a way that is reserved for the implementation you may get undefined behavior. Unless you are 100% certain that every one working on the project know these rules by heart. I suggest to simply avoid leading underscore in identifiers.
The code has undefined behavior on the first line:
Speaking of which, the include guard as written it is susceptible to name collision as it is rather short and
As a personal side note, I personally dislike the style of trailing underscores as well. I use
Ordering of
This is of course very much up to personal preference but I prefer to always have the different visibility fields ordered as:
Add member type definitions used
Typically in template container classes we add a few type definitions, normally I'd expect at least the following:
But prefer to have most of the member types of for example
The reason is that for example if you do:
what is the correct type to put into
Yes, you could use iterators and probably should but it's not always possible. But we can use
Naming
It is very unfortunate that the class shares it's name with
The name
Single responsibility
A class should do one thing and do it well, see Single Responsibility Principle. Your class has two responsibilities, manage a dynamic memory region AND add keep track of an offset.
In fact this class should really be split into two. There should be one class that manages the dynamic memory, this would just be a clone of
and you would use it like so:
Managing raw memory
I understand that this is probably an exercise in handling memory and remembering to implement the rule of three but in practice you would use
Don't abuse operator overloads
Operator overloads are easy to abu
Interface/High-level comments
Use of leading underscore
_ has severe limitations.The C and C++ standards reserve the use of certain patterns of leading underscores for implementation purposes. If you use leading underscore in a way that is reserved for the implementation you may get undefined behavior. Unless you are 100% certain that every one working on the project know these rules by heart. I suggest to simply avoid leading underscore in identifiers.
The code has undefined behavior on the first line:
#ifndef _VECTOR_H_Speaking of which, the include guard as written it is susceptible to name collision as it is rather short and
VECTOR is kind of a common name for a file. If a name collision happens the build will in best case break or in worst case whatever header happened to have the same include guard may provide a class with identical signatures but different behavior (unlikely I know, but has happened to me) and you could have crashes during runtime. A much better include guard would be #ifndef GUARD_MYPROJECT_VECTOR_H. As a personal side note, I personally dislike the style of trailing underscores as well. I use
m_ as a prefix for mutable members and c_ for constant members but other conventions are just as good.Ordering of
public, protected, privateThis is of course very much up to personal preference but I prefer to always have the different visibility fields ordered as:
public, protected, private. The reason is that the code will by definition be used more often than it is written (hopefully!) and by that reason you should write the code to be as easy as possible to read and use. For this reason I sort the fields in order of interest to the user. The user is most often interested in the public API, then possibly the protected stuff if they'll inherit from the class. And rarely are they interested in the private implementation.Add member type definitions used
Typically in template container classes we add a few type definitions, normally I'd expect at least the following:
using value_type = T;
using size_type = std::size_t;But prefer to have most of the member types of for example
std::vector. The reason is that for example if you do:
std::vector v;
for(??? i = 0; i < v.size(); ++i){
....
}what is the correct type to put into
???? Well in this case we know that it is size_t because this is what is std::vector is specified to return from size(). But what if it wasn't std::vector but some other template class we have no idea of, or what if the class itself is a template? Like so:template
void foo(){
Container c;
for(??? i = 0; i < c.size(); ++i){
}
}Yes, you could use iterators and probably should but it's not always possible. But we can use
std::vector::size_type or Container::size_type to always get the proper type for this and other similar purposes. Naming
It is very unfortunate that the class shares it's name with
std::vector, has the same purpose and similar signatures. But different semantics. For example if I hadn't read your question properly I would have believed that Vector v = Vector(32); would create a dynamic Vector with initial size of 32 elements. The class should be named OffsetVector or something to that tune instead to avoid confusion. The name
Delete is also very unfortunate as it carries the connotation of keyword delete it just looks wrong. Typically we use Erase or Remove for this type of method. Also Delete() is typically called Clear. Single responsibility
A class should do one thing and do it well, see Single Responsibility Principle. Your class has two responsibilities, manage a dynamic memory region AND add keep track of an offset.
In fact this class should really be split into two. There should be one class that manages the dynamic memory, this would just be a clone of
std::vector. And another class that adds an offset to a container. Maybe something like this:template
class OffsetContainer{
public:
using value_type = Container::value_type;
using reference_type = Container::reference_type;
using size_type = Container::size_type;
// add others
OffsetContainer(Container&& c, size_type offs)
: container(std::move(c)), offset(offs)
{}
reference_type operator [](size_type i){
return container[i + offset];
}
private:
Container container;
size_type offset;
};and you would use it like so:
OffsetContainer> c(offset);Managing raw memory
I understand that this is probably an exercise in handling memory and remembering to implement the rule of three but in practice you would use
std::unique_ptr instead.Don't abuse operator overloads
Operator overloads are easy to abu
Code Snippets
#ifndef _VECTOR_H_using value_type = T;
using size_type = std::size_t;std::vector<X> v;
for(??? i = 0; i < v.size(); ++i){
....
}template<typename Container>
void foo(){
Container c;
for(??? i = 0; i < c.size(); ++i){
}
}template<typename Container>
class OffsetContainer{
public:
using value_type = Container::value_type;
using reference_type = Container::reference_type;
using size_type = Container::size_type;
// add others
OffsetContainer(Container&& c, size_type offs)
: container(std::move(c)), offset(offs)
{}
reference_type operator [](size_type i){
return container[i + offset];
}
private:
Container container;
size_type offset;
};Context
StackExchange Code Review Q#160171, answer score: 14
Revisions (0)
No revisions yet.