HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppModerate

Vector implementation, which received unhelpful negative feedback from professor

Submitted by: @import:stackexchange-codereview··
0
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: 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 _ 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, private

This 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.