-2

I have a program that uses something I made called "SortableVector," with a parent called SimpleVector. The Simple Vector is fine, and so is most of SortableVector, but the problem is the sort function in SortableVector. It sorts numbers just fine, but I can't get it to sort strings. Here's what I originally had:

template <class T>
void SortableVector<T>::sort()
{
   T temp = 0;
   for(int i = 0; i < this->size(); i++)
   {
      for (int count = i+1; count < this->size(); count++)
      {
          if(this->operator[](0) == int)
          {
             if (this->operator[](count) < this->operator[](i))
             {
                temp = this->operator[](count);
                this->operator[](count) = this->operator[](i);
                this->operator[](i) = temp;
                count = i+1;
             }
          }
      }
   }

}

Then I tried to use the sort(begin, end) function, but that didn't work either:

template <class T>
void SortableVector<T>::sort()
{
    sort(this->operator[].begin(), this->operator[].end();
}

EDIT: To help people understand the problem, here's the whole file:

#ifndef SORTABLEVECTOR_H
#define SORTABLEVECTOR_H

using namespace std;
#include "SimpleVector.h"
#include <algorithm>
#include <fstream>
#include <string>

template <class T>
class SortableVector : public SimpleVector<T>
{
public:
SortableVector(int s) : SimpleVector<T>(s)
{}

SortableVector(SortableVector &);

SortableVector(SimpleVector<T> &obj):
        SimpleVector<T>(obj)
{}
void sort();
};

 template <class T>
SortableVector<T>::SortableVector(SortableVector &obj):SimpleVector<T>(obj)
{
}

template <class T>
void SortableVector<T>::sort()
{
std::sort(this->operator[].begin(), this->operator[].end());

T temp = 0;
    for(int i = 0; i < this->size(); i++)
    {
        for (int count = i+1; count < this->size(); count++)
        {
            if(this->operator[](0) == int)
            {
                if (this->operator[](count) < this->operator[](i))
                {
                    temp = this->operator[](count);
                    this->operator[](count) = this->operator[](i);
                    this->operator[](i) = temp;
                    count = i+1;
                }
            }
        }
    }

}

#endif

And this is SimpleVector:

// SimpleVector.h

#ifndef SIMPLEVECTOR_H
#define SIMPLEVECTOR_H

#include <iostream>
#include <cstdlib>
using namespace std;

template <class T>
class SimpleVector
{
private:
T *aptr;
int arraySize;  
void subError();                          // Handles subscripts out of range
public:
SimpleVector()                            // Default constructor
   { aptr = 0; arraySize = 0;}
SimpleVector(int);                        // Constructor
SimpleVector(const SimpleVector &);       // Copy constructor
~SimpleVector();                          // Destructor
int size() { return arraySize; }
T &operator[](int);                       // Overloaded [] operator
void print() const;                       // outputs the array elements

void push_back(T);                        // newly added function (implemention needed)
T pop_back();                             // newly added function (implemention needed)
};

//****************************************************************
//          Constructor for SimpleVector class                   *
// Sets the size of the array and allocates memory for it.       *
//****************************************************************
template <class T>
SimpleVector<T>::SimpleVector(int s)
{
arraySize = s;
aptr = new T [s];
}

//*********************************************
// Copy Constructor for SimpleVector class    *
//*********************************************
template <class T>
SimpleVector<T>::SimpleVector(const SimpleVector &obj)
{
arraySize = obj.arraySize;
aptr = new T [arraySize];
for(int count = 0; count < arraySize; count++)
    *(aptr + count) = *(obj.aptr + count);
}

// *************************************
// Destructor for SimpleVector class   *
// *************************************
template <class T>
SimpleVector<T>::~SimpleVector()
{
if (arraySize > 0)
    delete [] aptr;
}

//************************************************************
//               SubError function                           *
// Displays an error message and terminates the program when * 
// a subscript is out of range.                              *
//************************************************************
template <class T>
void SimpleVector<T>::subError()
{
cout << "ERROR: Subscript out of range.\n";
exit(0);
}

//***********************************************************
//             Overloaded [] operator                       *
// This function returns a reference to the element         *
// in the array indexed by the subscript.                   *
//***********************************************************
template <class T>
T &SimpleVector<T>::operator[](int sub)
{
if (sub < 0 || sub >= arraySize)
    subError();
return aptr[sub];
}

//********************************************************
// prints all the entries is the array.                  *
//********************************************************
template <class T>
void SimpleVector<T>::print( ) const
{
for (int k = 0; k < arraySize; k++ )
  cout << aptr[k] << "  ";
cout << endl;  
}

//***************************************************************
//                   (1) push_back(T val)                       *
// The push_back function pushes its argument onto the back     *
// Of the vector.                                               *                                      
//***************************************************************

template <class T>
void SimpleVector<T>::push_back(T val)
{
aptr[arraySize] = val;
arraySize++;
}

// *****************************************************
//                (2) pop_back()                       *
// The pop_back function removes the last element      *
// Of the vector. It also returns that value.          *
// *****************************************************

template <class T>
T SimpleVector<T>::pop_back()
{
arraySize--;
T temp = aptr[arraySize];
return temp;
}





#endif

The SimpleVector was provided by the instructor.

  • `this->operator[].begin()` ?! Seriously? You can't expect a random concatenation of language keywords to "do something". I'd start here: http://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list – sehe May 07 '14 at 23:01
  • Anytime you're building DIY generic containers in C++ you are knocking on doorstep of STL and Boost. I know it wasn't the point of your question, but and are your friends :) –  May 07 '14 at 23:03
  • What does "didn't work" mean? – Code-Apprentice May 07 '14 at 23:06
  • Re. the edit: the edit is most unuseful, because we still have no clue how the storage is implemented. By the way `this->operator[](count)` could be `(*this)[count]`, e.g. – sehe May 07 '14 at 23:07
  • I think the original sort would be close to working if you took out the lines `if(this->operator[](0) == int)` and `count = i + 1;`. And `std::swap` says hi. – M.M May 08 '14 at 01:59

2 Answers2

1
  • You can do without the repeated this-> 99% of the time

  • You can just invoke operators, instead of invoking them as functions

  • You can just use std::sort from <algorithm>

Imagining some of the code you didn't show:

#include <vector>
#include <algorithm>

template <typename T>
struct SimpleVector {
    std::vector<T> data;

    virtual ~SimpleVector() {}
};


template <typename T>
struct SortableVector : public SimpleVector<T> {
    void sort() {
        std::sort(this->data.begin(), this->data.end());
    }
};

#include <iostream>

int main()
{
    SortableVector<std::string> sv;
    sv.data = {"one","two","three","four"};

    sv.sort();
    for(auto& s : sv.data)
        std::cout << s << ' ';
}

DISCLAIMER This seems a very non-c++ way to design classes. Algorithms and containers are traditionally separated, for good reason. The exception being that member functions sometimes perform operations in more optimal ways (e.g. std::set<>::find instead of std::find)

sehe
  • 374,641
  • 47
  • 450
  • 633
  • When I do that, it says that "left end of .begin() must have class/struct/union" and the same thing for .end() – user3609314 May 07 '14 at 22:57
  • "When I do _that_" - the problem is, of course, "*that*". It needs to be the right thing. **[Live On Coliru](http://coliru.stacked-crooked.com/a/0de3d0d930f19f16)** – sehe May 07 '14 at 22:59
  • "When I do that"...what is "that"? – Code-Apprentice May 07 '14 at 23:07
  • 1
    @Code-Guru use `std::sort`, obviously. Thing is, he didn't do it right. I showed a working example. Let's wait a bit. – sehe May 07 '14 at 23:08
0

It should be std::sort(aptr, aptr + arraySize);. Also you should check arraySize > 0 before doing this, if you're intending to catch errors like that instead of having undefined behaviour.

begin and end aren't magic; they rely on the object either being an array (not a pointer), or the object implementing functions .begin() and .end() that return iterators.

You could even define those functions yourself, e.g. SimpleVector<T>::begin() { return aptr; } etc. , but maybe this is not a good idea as it bypasses your bounds-checking. You'd have to actually write an iterator class instead of returning a pointer, which is more trouble than you're ready for :)

NB. Everywhere you write this->operator[](foo), it would be clearer to write (*this)[foo]

M.M
  • 138,810
  • 21
  • 208
  • 365