5
\$\begingroup\$

I have written a (dynamic) TemplateArray class, for purposes to be included as a baseline for a library I am working on, and I would like feedback on any of the following:

  • How the code can be improved (even if it's just your subjective opinion)?
  • How the code can be made more efficient?
  • How the functions of the class can be made more intuitive and user-friendly (does it operate like how you anticipate it to)?

namespace TL
{

template<typename TemplateItem>
class TemplateArray
{

protected:

        TemplateItem *Array;
        SIZE_TYPE Size;

        void Clear(){Array = NULL; Size = 0;}

    public:

        const bool IsEmpty() const { return ((Array == NULL) && (Size == 0)); }
        const bool IsValid() const { return ((Array != NULL) && (Size != 0)); }

        const bool operator!() const {return IsEmpty();}
        operator const bool () const {return IsValid();}

        const SIZE_TYPE GetSize() const {return Size;}

        const bool SetSize(const SIZE_TYPE S)
        {
            ERRORCLASSFUNCTION(TemplateArray,SetSize,S < 1,RETURN_BOOL)
            if(!IsEmpty())
            {
                Close();
            }
            ERRORCLASSFUNCTION(TemplateArray,SetSize,!CREATEB(Array,S),RETURN_BOOL)
            Size = S;
            return true;
        }

        const bool CopyArray(const TemplateItem Arr[], const SIZE_TYPE S)
        {
            ERRORCLASSFUNCTION(TemplateArray,CopyArray,S < 1,RETURN_BOOL);
            ERRORCLASSFUNCTION(TemplateArray,CopyArray,S > Size,RETURN_BOOL);

            SIZE_TYPE Temp = 0;
            while(Temp < S)
            {
                Array[Temp] = Arr[Temp];
                Temp++;
            }
            return true;
        }

        const bool SetArray(const TemplateItem Arr[], const SIZE_TYPE S)
        {
            ERRORCLASSFUNCTION(TemplateArray,SetArray,S < 1,RETURN_BOOL)
            ERRORCLASSFUNCTION(TemplateArray,SetArray,!SetSize(S),RETURN_BOOL)
            SIZE_TYPE Temp = 0;
            while(Temp < S)
            {
                Array[Temp] = Arr[Temp];
                Temp++;
            }
            return true;
        }

        const bool SetArray(const TemplateArray &ItemCopy)
        {
            ERRORCLASSFUNCTION(TemplateArray,SetArray,!ItemCopy,RETURN_BOOL)
            ERRORCLASSFUNCTION(TemplateArray,SetArray,!SetArray(ItemCopy.Array,ItemCopy.Size),RETURN_BOOL)
            return true;
        }

        const bool SetArray(const std::vector<TemplateItem> &ItemCopy)
        {
            ERRORCLASSFUNCTION(TemplateArray,SetArray,ItemCopy.empty(),RETURN_BOOL)
            ERRORCLASSFUNCTION(TemplateArray,SetArray,!SetArray(&ItemCopy[0],ItemCopy.size()),RETURN_BOOL)
            return true;
        }

        TemplateItem * const GetArray() { return Array; }
        const TemplateItem * const GetArray() const { return Array; }

        void TransferFrom(TemplateArray &ItemCopy)
        {
            Close();
            Array = ItemCopy.Array;
            Size = ItemCopy.Size;
            ItemCopy.Array = NULL;
            ItemCopy.Size = 0;
        }

        const bool Reverse()
        {
            ERRORCLASSFUNCTION(TemplateArray,Reverse,!IsValid(),RETURN_BOOL)
            TemplateArray<TemplateItem> Temp;
            ERRORCLASSFUNCTION(TemplateArray,Reverse,!Temp.SetSize(Size),RETURN_BOOL)
            TemplateItem *C_Ptr = Array+(Size-1), *C_Ptr_2 = Temp.GetArray();

            while(C_Ptr != Array)
            {
                *C_Ptr_2 = *C_Ptr;
                C_Ptr--; C_Ptr_2++;
            }
            *C_Ptr_2 = *C_Ptr;
            TransferFrom(Temp);
            return true;
        }

        const bool Compare(const TemplateItem Arr[], const SIZE_TYPE S)
        {
            if(!IsValid())
            {
                return false;
            }

            ERRORCLASSFUNCTION(TemplateArray,Compare,S < 1,RETURN_BOOL)
            SIZE_TYPE Temp = 0;

            while(Temp < S)
            {
                if(Array[Temp] != Arr[Temp]){return false;}
                Temp++;
            }

            return true;
        }

        const bool Compare(const TemplateArray &ItemCopy)
        {
            if(Size != ItemCopy.Size)
            {
                return false;
            }

            if(IsEmpty())
            {
                return true;
            }

            return Compare(ItemCopy.Array,ItemCopy.Size);
        }

        const bool Compare(const std::vector<TemplateItem> &ItemCopy)
        {
            if(ItemCopy.size() != Size)
            {
                return false;
            }

            if(ItemCopy.empty())
            {
                return true;
            }

            return Compare(&ItemCopy[0],ItemCopy.size());
        }

        const bool Append(const TemplateItem Data[], const SIZE_TYPE S)
        {
            ERRORCLASSFUNCTION(TemplateArray,Append,Data == NULL,RETURN_BOOL)
            ERRORCLASSFUNCTION(TemplateArray,Append,S < 1,RETURN_BOOL)
            TemplateArray<TemplateItem> Temp;
            ERRORCLASSFUNCTION(TemplateArray,Append,!Temp.SetSize(Size+S),RETURN_BOOL)
            SIZE_TYPE Temp2 = 0;
            while(Temp2 < Size)
            {
                Temp.Array[Temp2] = Array[Temp2];
                Temp2++;
            }

            Temp2 = 0;
            while(Temp2 < S)
            {
                Temp.Array[Temp2+Size] = Data[Temp2];
                Temp2++;
            }
            TransferFrom(Temp);
            return true;
        }

        //Tested
        const bool Append(const TemplateArray &ItemCopy)
        {
            ERRORCLASSFUNCTION(TemplateArray,Append,!ItemCopy.IsValid(),RETURN_BOOL)
            return Append(ItemCopy.GetArray(),ItemCopy.GetSize());
        }

        const bool Append(const std::vector<TemplateItem> &ItemCopy)
        {
            ERRORCLASSFUNCTION(TemplateArray,Append,ItemCopy.empty(),RETURN_BOOL)
            ERRORCLASSFUNCTION(TemplateArray,Append,!Append(&ItemCopy[0],ItemCopy.size()),RETURN_BOOL)
            return true;
        }

        const bool Append(const TemplateItem &ItemCopy)
        {
            return Append(&ItemCopy,1);
        }

        const bool Remove(const TemplateItem Data[], const SIZE_TYPE S)
        {
            ERRORCLASSFUNCTION(TemplateArray,Append,Data == NULL,RETURN_BOOL)
            ERRORCLASSFUNCTION(TemplateArray,Remove,S < 1,RETURN_BOOL)
            if(!IsValid()){return false;}
            if(S > Size){return false;}

            SIZE_TYPE Temp = 0, Temp2 = 0;

            do
            {
                Temp2 = 0;
                while(Temp < Size)
                {
                    if(Array[Temp] == Data[Temp2])
                    {
                        break;
                    }
                    Temp++;
                }

                while(Temp < Size)
                {
                    if(Array[Temp] != Data[Temp2])
                    {
                        break;
                    }

                    Temp++; Temp2++;

                    if(Temp2 == S)
                    {
                        break;
                    }
                }

                if(Temp2 == S)
                {
                    TemplateArray<TemplateItem> DataTemp;

                    ERRORCLASSFUNCTION(TemplateArray,operator-=,!DataTemp.SetSize(Size-S),RETURN_BOOL)
                    Temp2 = Temp - S;
                    Temp = 0;

                    while(Temp < Temp2)
                    {
                        DataTemp.Array[Temp] = Array[Temp];
                        Temp++;
                    }

                    Temp2 += S;

                    while(Temp2 < Size)
                    {
                        DataTemp.Array[Temp] = Array[Temp2];
                        Temp++; Temp2++;
                    }

                    TransferFrom(DataTemp);
                    return *this;
                }

            }while(Temp < Size);

            return *this;
        }

        const bool Remove(const std::vector<TemplateArray> &ItemCopy)
        {
            ERRORCLASSFUNCTION(TemplateArray,Remove,ItemCopy.empty(),RETURN_BOOL)
            ERRORCLASSFUNCTION(TemplateArray,Remove,!Remove(&ItemCopy[0],ItemCopy.size()),RETURN_BOOL)
            return true;
        }

        const bool Remove(const TemplateArray &ItemCopy)
        {
            ERRORCLASSFUNCTION(TemplateArray,Remove,!IsValid(),RETURN_BOOL)
            ERRORCLASSFUNCTION(TemplateArray,Remove,!Remove(ItemCopy.Array,ItemCopy.Size),RETURN_BOOL)
            return *this;
        }

        const bool Remove(const TemplateItem &ItemCopy)
        {
            return Remove(&ItemCopy,1);
        }

        void Reset()
        {
            if(Array != NULL)
            {
                DELETEB(Array);
            }
            Open();
        }
        void Open(){ Clear(); }
        void Close()
        {
            if(Array != NULL)
            {
                DELETEB(Array);
            }
            Clear();
        }

        //Tested
        TemplateArray(){Open();}

        ~TemplateArray(){Close();}

        //Tested
        TemplateArray(const TemplateItem Data[], const SIZE_TYPE S)
        {
            Open();
            ERRORCLASSFUNCTION(TemplateArray,TemplateArray,!SetArray(Data,S),)
        }

        //Tested
        TemplateArray(const TemplateArray &ItemCopy)
        {
            Open();
            ERRORCLASSFUNCTION(TemplateArray,TemplateArray,!SetArray(ItemCopy),)
        }

        TemplateArray(const std::vector<TemplateItem> &ItemCopy)
        {
            ERRORCLASSFUNCTION(TemplateArray,TemplateArray,!SetArray(ItemCopy),RETURN_VOID)
        }

        operator const SIZE_TYPE () const {return Size;}

        operator const std::vector<TemplateItem> () const
        {
            std::vector<TemplateItem> Temp;
            if(!IsValid())
            {
                return Temp;
            }

            try
            {
                Temp.resize(Size);
            }
            catch(...)
            {
                std::vector<TemplateItem> Temp2;
                ERRORCLASSFUNCTION(TemplateArray,std::vector,true,return Temp2;)
            }

            SIZE_TYPE Temp2 = 0;

            while(Temp2 < Size)
            {
                Temp[Temp2] = Array[Temp2];
                Temp2++;
            }

            return Temp;
        }

        const bool operator!=(const TemplateArray &ItemCopy){return !((*this) == ItemCopy);}
        const bool operator==(const TemplateArray &ItemCopy)
        {
            return Compare(ItemCopy);
        }

        const bool operator==(const std::vector<TemplateItem> &ItemCopy)
        {
            return Compare(ItemCopy);
        }

        TemplateArray &operator+=(const TemplateItem &ItemCopy)
        {
            ERRORCLASSFUNCTION(TemplateArray,operator+=,!Append(ItemCopy),RETURN_THIS)
            return *this;
        }

        //Tested
        TemplateArray &operator+=(const TemplateArray &ItemCopy)
        {
            ERRORCLASSFUNCTION(TemplateArray,operator+=,!Append(ItemCopy),RETURN_THIS)
            return *this;
        }

        //Tested
        TemplateArray &operator+=(const std::vector<TemplateItem> &ItemCopy)
        {
            ERRORCLASSFUNCTION(TemplateArray,operator+=,!Append(ItemCopy),RETURN_THIS)
            return *this;
        }

        TemplateArray &operator-=(const TemplateItem &ItemCopy)
        {
            ERRORCLASSFUNCTION(TemplateArray,operator-=,!Remove(ItemCopy),RETURN_THIS)
            return *this;
        }

        TemplateArray &operator-=(const std::vector<TemplateItem> &ItemCopy)
        {
            ERRORCLASSFUNCTION(TemplateArray,operator-=,!Remove(ItemCopy),RETURN_THIS)
            return *this;
        }

        TemplateArray &operator-=(const TemplateArray &ItemCopy)
        {
            ERRORCLASSFUNCTION(TemplateArray,operator-=,!Remove(ItemCopy),RETURN_THIS)
            return *this;
        }

        TemplateArray &operator=(const std::vector<TemplateItem> &ItemCopy)
        {
            ERRORCLASSFUNCTION(TemplateArray,operator=,!SetArray(ItemCopy),RETURN_THIS)
            return *this;
        }

        TemplateArray &operator=(const TemplateArray &ItemCopy)
        {
            ERRORCLASSFUNCTION(TemplateArray,operator=,!SetArray(ItemCopy),RETURN_THIS)
            return *this;
        }

        TemplateItem &operator[](const SIZE_TYPE S){ return Array[S]; }
        const TemplateItem &operator[](const SIZE_TYPE S) const { return Array[S]; }
};

}

A working example you can paste directly into your compiler is also available here.

The class has to support the following:

To minimise bug tracing times:

  • Vocal error reporting.
  • Traceable error reporting.
  • Fail-safes (quit on first problem attitude).
  • Functions that primarily return bool where possible.

For compatibility with other classes:

  • Return and accept a standard data-type (in this case, std::vector).

The class should support the following requirements:

  • User-friendliness. User should not have to think when using the class.
  • Compatibility with future, potentially unknown classes and sub-classes.

Please ignore the macro, printf usage and the fact the functions are inside the class. It's just my personal preference. Please avoid suggesting using std::string/std::vector (this is a really old cliche now), as standard classes are generic and don't offer the specific functionality a custom built class can (i.e. bool returning, error-reporting/tracing functions), although it does offer compatibility with such classes.

\$\endgroup\$
  • 2
    \$\begingroup\$ Wouldn't it be easier to simply do this as a wrapper around std::vector? And I don't see the point of many of the bool returns? Some are functions that should never fail (and coincide with std::vector). The trace-ability could be easily done as a wrapper arounds std::vector. \$\endgroup\$ – ronag Oct 17 '11 at 15:59
  • 3
    \$\begingroup\$ I think you're mistaken about the supposed inability of a generic to provide "bool returning, error-reporting/tracing functions". At least from my perspective, the fact that you've failed to provide begin, end, rbegin, rend, etc., means your class is essentially unusable. IMO, your class also has an almost incredibly bloated external interface. OTOH, your requirement at the end seem (to me) to be saying: "Please tell me how to improve this without fixing any of its problems." \$\endgroup\$ – Jerry Coffin Oct 17 '11 at 16:44
  • 4
    \$\begingroup\$ There is a reason it is a cliche. Its because it works. This on the other hand I am not convinced about. \$\endgroup\$ – Martin York Oct 17 '11 at 17:06
  • 1
    \$\begingroup\$ en.wikipedia.org/wiki/Not_Invented_Here \$\endgroup\$ – ronag Oct 17 '11 at 17:15
  • 2
    \$\begingroup\$ @SSight3: Your comments are just silly at this point. 2002!!! Accept the criticisms of your class make it better or ignore; then move on. Its not that big of a deal everybody makes a mess now and then. \$\endgroup\$ – Martin York Oct 20 '11 at 16:17
6
\$\begingroup\$

How the code can be improved (even if it's just your subjective opinion)?

Replace the macros with readable code.
They make it much harder to read and understand than it needs to be.

Your object does not provide any strong guarantees (which is what I would expect from a container). Either the operation succeeded or the operation fails and the object is left in its original state. Your array does not provide this. If it fails the original state is usually lost.

This breaks your own rule of User-friendliness. User should not have to think when using the class. Now they do have to think. O crap that last operation just lost all my data I can;t even dump my current state for debugging.

You don't seem to check for self assignment (not that I can see I could be wrong but the code is very hard to read). Thus you will break as you delete the array you are copying (yourself) and then read from the now dead array illiciting undefined behavior.

How the code can be made more efficient?

That will take a long time.
But it looks like you have done a basic job of getting it working.

It may improve the use of the swap method which can be useful in a lot of situations and the default implementation of swap is not very efficient for any container.

You do not re-use the array. You always delete and allocate. That can be very in-efficient. It may be nicer to check if the array being copied over this one is smaller and if so re-use the array just move/copy the elements into this array.

You don't pre-allocate memory for possible expansion in the near future and thus fall into the trap of deleting and re-allocating the array for every push onto the back no matter how big it is.

How the functions of the class can be made more intuitive and user-friendly (does it operate like how you anticipate it to)?

This is where your class is horrible. It is basically the most unfriendly container class I have seen and can not be used with any of the standard algorithms, which makes it practically unusable.

Generally all-round friendly coder advice to a non-expert.

  • Get rid of the macros replace them with function calls.
  • Implement iterators. (so you can use the standard algorithms)
  • Provide better protection for your user.
    • Operations that fail should leave the object unchanged.

I will go to the cliche of saying I don't see any benefit of using this over std::vector (and I do see lots of negatives: Two of the big ones are ease of use and efficiency (Both contradicting your own design goals)).

\$\endgroup\$
  • \$\begingroup\$ This is the criticism I was looking for. It will be all taken onboard. \$\endgroup\$ – SSight3 Oct 19 '11 at 15:43
  • \$\begingroup\$ This is where your class is horrible. It is basically the most unfriendly container class I have seen and can not be used with any of the standard algorithms, which makes it practically unusable. Could you clarify what it is you don't like so I could improve? \$\endgroup\$ – SSight3 Oct 19 '11 at 16:21
  • \$\begingroup\$ in the section: Generally all-round friendly coder advice. The line: Implement iterators and the second line Provide better protection for your user. Both of these are essential. The first is easy the second will be near imposable with your current design (it needs a major re-write). \$\endgroup\$ – Martin York Oct 19 '11 at 17:44
1
\$\begingroup\$

All your methods have corresponding functionality with std::vector. I suggest you write your class as a wrapper around std::vector.

   template<typename TemplateItem>
class TemplateArray
{
    public:
        // std::vector::empty
        const bool IsEmpty() const { return ((Array == NULL) && (Size == 0)); }

        // unnecessary
        const bool IsValid() const { return ((Array != NULL) && (Size != 0)); }
        const bool operator!() const {return IsEmpty();}
        operator const bool () const {return IsValid();}

        // std::vector::size
        const SIZE_TYPE GetSize() const {return Size;}

        // std::vector::resize
        const bool SetSize(const SIZE_TYPE S)

        // std::copy
        const bool CopyArray(const TemplateItem Arr[], const SIZE_TYPE S)
        const bool SetArray(const TemplateItem Arr[], const SIZE_TYPE S)
        const bool SetArray(const TemplateArray &ItemCopy)
        const bool SetArray(const std::vector<TemplateItem> &ItemCopy)
        TemplateItem * const GetArray() { return Array; }
        const TemplateItem * const GetArray() const { return Array; }

        // move constructor
        void TransferFrom(TemplateArray &ItemCopy)

        // std::reverse
        const bool Reverse()

        // std::vector::operator==
        const bool Compare(const TemplateItem Arr[], const SIZE_TYPE S)
        const bool Compare(const TemplateArray &ItemCopy)
        const bool Compare(const std::vector<TemplateItem> &ItemCopy)

        // std::vector::push_back
        const bool Append(const TemplateItem Data[], const SIZE_TYPE S)
        const bool Append(const TemplateArray &ItemCopy)
        const bool Append(const std::vector<TemplateItem> &ItemCopy)
        const bool Append(const TemplateItem &ItemCopy)

        // std::vector::erase + std::remove.
        const bool Remove(const TemplateItem Data[], const SIZE_TYPE S)
        const bool Remove(const std::vector<TemplateArray> &ItemCopy)
        const bool Remove(const TemplateArray &ItemCopy)
        const bool Remove(const TemplateItem &ItemCopy)

        // std::vector::clear
        void Clear()
        void Reset()

        // A bit wierd, but i guess you want something like std::vector::clear + std::vector::shrink_to_fit
        void Open()
        void Close()

        // Nice to have i guess, std::vector::push_back
        TemplateArray &operator+=(const TemplateItem &ItemCopy)

        // std::vector::operator[]
        TemplateItem &operator[](const SIZE_TYPE S){ return Array[S]; }
        const TemplateItem &operator[](const SIZE_TYPE S) const { return Array[S]; }
};
\$\endgroup\$
  • \$\begingroup\$ Does vector even support move semantics...? And no low-level trace, and ignoring my request. You are not the first programmer to suggest this, you won't be the last (programmers have a nigh tendency to bleat std::string/std::vector etc etc as if it's a be-all, end-all solution - if I mention it, I already know it). Appeal to tradition and appeal to popularity. If vector does not support this functionality, I see no reason to expand it's obviously decrepit nature. \$\endgroup\$ – SSight3 Oct 17 '11 at 16:31
  • \$\begingroup\$ std::vector supports move semantics. I'm not saying that you should not use this class, I'm saying that you can more easily implement it as a wrapper over std::vector. \$\endgroup\$ – ronag Oct 17 '11 at 16:41
  • \$\begingroup\$ In some ways it could be more efficient, but I have no idea what vector does under the covers (and it varies from implementation to implementation). I do not see the move semantics from this list. \$\endgroup\$ – SSight3 Oct 17 '11 at 16:50
  • \$\begingroup\$ You are looking at the C++03 reference, not C++11 (msdn.microsoft.com/en-us/library/zzw4bwhd.aspx). What do you need to know of what it does under the covers? Most (everything?) of the things you should need to know is in the specification. It seems to me the only thing that is interesting for you is what exceptions it can throw, which it says in the reference. \$\endgroup\$ – ronag Oct 17 '11 at 17:02
  • \$\begingroup\$ I'm using 03 not 11 (11 has lots of nice touches but it's fairly new and not widely supported?). The fact it had to be updated kinda demonstrates my point on it being decrepit in nature. \$\endgroup\$ – SSight3 Oct 17 '11 at 17:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.