Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link
  1. No vertical space makes it extremely hard to read (thus maintain).
  2. None standard bracing style is also really horrible give it up.
  3. The standard containers are not designed to be inherited from.
  4. Prefer composition over inheritance Prefer composition over inheritance
  5. Your interface leaks pointers.
    Modern C++ prefers you define ownership semantics of objects rather than use pointers.
  1. No vertical space makes it extremely hard to read (thus maintain).
  2. None standard bracing style is also really horrible give it up.
  3. The standard containers are not designed to be inherited from.
  4. Prefer composition over inheritance
  5. Your interface leaks pointers.
    Modern C++ prefers you define ownership semantics of objects rather than use pointers.
  1. No vertical space makes it extremely hard to read (thus maintain).
  2. None standard bracing style is also really horrible give it up.
  3. The standard containers are not designed to be inherited from.
  4. Prefer composition over inheritance
  5. Your interface leaks pointers.
    Modern C++ prefers you define ownership semantics of objects rather than use pointers.
tyyyppos corrected
Source Link
rolfl
  • 98.1k
  • 17
  • 219
  • 419

Your structure does not allow multiple index (as you claim in the comments) as each object only contains one index. Now maybe you mean you can use multiple of your structures to achieve multi-indexing because all the members are stored as pointers and thus can easily be put into multiple versions of your container each with a different sorting arrangement (and that is true). But this exposes one of yhethe main problems with your structure (ownership semantics).

The class pvect takes pointers (or builds them dynamically) but does not seem to take ownership (they are not destroyed on destruction or when they are removed from the container (which means you have a leaky container). Not a good start. Maybe that is handled in some wrpapperwrapper class that I have not spotted. But in that case this should be a private member class so it can not be accidentally used.

I don't see the need for a GetKey class when you already have a Less class. The default version of std::less uses operator< which should work in most situations. When it does not you should define a specialized version than knows how to query the undleryingunderlying T to get the get the key you don't need an extra level of indirection.

The rest is too dense. But I have the felingfeeling I will find the same sort of issues.

Your structure does not allow multiple index (as you claim in the comments) as each object only contains one index. Now maybe you mean you can use multiple of your structures to achieve multi-indexing because all the members are stored as pointers and thus can easily be put into multiple versions of your container each with a different sorting arrangement (and that is true). But this exposes one of yhe main problems with your structure (ownership semantics).

The class pvect takes pointers (or builds them dynamically) but does not seem to take ownership (they are not destroyed on destruction or when they are removed from the container (which means you have a leaky container). Not a good start. Maybe that is handled in some wrpapper class that I have not spotted. But in that case this should be a private member class so it can not be accidentally used.

I don't see the need for a GetKey class when you already have a Less class. The default version of std::less uses operator< which should work in most situations. When it does not you should define a specialized version than knows how to query the undlerying T to get the get the key you don't need an extra level of indirection.

The rest is too dense. But I have the feling I will find the same sort of issues.

Your structure does not allow multiple index (as you claim in the comments) as each object only contains one index. Now maybe you mean you can use multiple of your structures to achieve multi-indexing because all the members are stored as pointers and thus can easily be put into multiple versions of your container each with a different sorting arrangement (and that is true). But this exposes one of the main problems with your structure (ownership semantics).

The class pvect takes pointers (or builds them dynamically) but does not seem to take ownership (they are not destroyed on destruction or when they are removed from the container (which means you have a leaky container). Not a good start. Maybe that is handled in some wrapper class that I have not spotted. But in that case this should be a private member class so it can not be accidentally used.

I don't see the need for a GetKey class when you already have a Less class. The default version of std::less uses operator< which should work in most situations. When it does not you should define a specialized version than knows how to query the underlying T to get the get the key you don't need an extra level of indirection.

The rest is too dense. But I have the feeling I will find the same sort of issues.

Post Undeleted by Loki Astari
added 3808 characters in body
Source Link
Loki Astari
  • 97.7k
  • 5
  • 126
  • 341

General Points

  1. No vertical space makes it extremely hard to read (thus maintain).
  2. None standard bracing style is also really horrible give it up.
  3. The standard containers are not designed to be inherited from.
  4. Prefer composition over inheritance
  5. Your interface leaks pointers.
    Modern C++ prefers you define ownership semantics of objects rather than use pointers.

Dis-beliefs.

Lets startYour structure does not allow multiple index (as you claim in the comments) as each object only contains one index. Now maybe you mean you can use multiple of your structures to achieve multi-indexing because all the members are stored as pointers and thus can easily be put into multiple versions of your container each with a different sorting arrangement (and that is true). But this exposes one of yhe main problems with your structure (ownership semantics).

Ownership semantics.

The class pvect takes pointers (or builds them dynamically) but does not seem to take ownership (they are not destroyed on destruction or when they are removed from the Usercontainer (which means you have a leaky container). Not a good start. Maybe that is handled in some wrpapper class that I have not spotted. But in that case this should be a private member class so it can not be accidentally used.
I

This leaking of ownership is pervasive about all your classes. As such it becomes unusable as a container in real world situations (as nobody can tell who is supposed to delete the object).

Overdesign

The requirement for a GeyKey is not required. It adds an extra layer of indirection on top of the Less type.

template <class T, class K=T,
 class Less = std::less<K>,
 class GetKey = get_key<T,K>> // Never really need this.

Lazy Design.

The comparison operator is overloaded out the wazoo.

template <class T, class K=T,
 class Less = std::less<K>, class GetKey = get_key<T,K>>
 class comparator: protected GetKey, protected Less {
public:
 const GetKey& get_key() const {
 return static_cast<const GetKey&>(*this); }
 const Less& less() const {
 return static_cast<const Less&>(*this); }
 K operator() (T* it) const {
 return get_key()(it); }
 bool operator() (K x, K y) const {
 return less()(x, y); }
 bool operator() (T* x, K y) const {
 return less()(get_key()(x), y); }
 bool operator() (K x, T* y) const {
 return less()(x, get_key()(y)); }
 bool operator() (T* x, T* y) const {
 return less()(get_key()(x), get_key()(y)); }};

There is no need for this. The comparator operator should compare two objects. This seems to suggest that you're underlying class that you are using them is designed in such a way that you were lazy in deciding which type of value you were using and you overload every single combination to get rid of the compiler errors.

While we have this class:

class comparator: protected GetKey, protected Less

Why are we inheriting from GetKey or Less? Neither of these classes have virtual interfaces where inheritance would make it sensible to inherit from them.

It is much easier to use composition in this case.

class comparator
{
 protected: // If you must make them protected sure.
 // But that seems unnesacery. Just make the whole thing public.
 GetKey getKey;
 Less lessThanTest;
 public:
 bool operator() (Value const& x, Value const& y) const
 {
 return lessThanTest(getKey(x), getKey(y));
 }
 }; // Now its readable.

Finally analysis of User (from original comments).

I find some of your decisions odd.

Lets start with the User class.
I find some of your decisions odd.

General Points

  1. No vertical space makes it extremely hard to read (thus maintain).
  2. None standard bracing style is also really horrible give it up.
  3. The standard containers are not designed to be inherited from.
  4. Prefer composition over inheritance
  5. Your interface leaks pointers.
    Modern C++ prefers you define ownership semantics of objects rather than use pointers.

Dis-beliefs.

Your structure does not allow multiple index (as you claim in the comments) as each object only contains one index. Now maybe you mean you can use multiple of your structures to achieve multi-indexing because all the members are stored as pointers and thus can easily be put into multiple versions of your container each with a different sorting arrangement (and that is true). But this exposes one of yhe main problems with your structure (ownership semantics).

Ownership semantics.

The class pvect takes pointers (or builds them dynamically) but does not seem to take ownership (they are not destroyed on destruction or when they are removed from the container (which means you have a leaky container). Not a good start. Maybe that is handled in some wrpapper class that I have not spotted. But in that case this should be a private member class so it can not be accidentally used.

This leaking of ownership is pervasive about all your classes. As such it becomes unusable as a container in real world situations (as nobody can tell who is supposed to delete the object).

Overdesign

The requirement for a GeyKey is not required. It adds an extra layer of indirection on top of the Less type.

template <class T, class K=T,
 class Less = std::less<K>,
 class GetKey = get_key<T,K>> // Never really need this.

Lazy Design.

The comparison operator is overloaded out the wazoo.

template <class T, class K=T,
 class Less = std::less<K>, class GetKey = get_key<T,K>>
 class comparator: protected GetKey, protected Less {
public:
 const GetKey& get_key() const {
 return static_cast<const GetKey&>(*this); }
 const Less& less() const {
 return static_cast<const Less&>(*this); }
 K operator() (T* it) const {
 return get_key()(it); }
 bool operator() (K x, K y) const {
 return less()(x, y); }
 bool operator() (T* x, K y) const {
 return less()(get_key()(x), y); }
 bool operator() (K x, T* y) const {
 return less()(x, get_key()(y)); }
 bool operator() (T* x, T* y) const {
 return less()(get_key()(x), get_key()(y)); }};

There is no need for this. The comparator operator should compare two objects. This seems to suggest that you're underlying class that you are using them is designed in such a way that you were lazy in deciding which type of value you were using and you overload every single combination to get rid of the compiler errors.

While we have this class:

class comparator: protected GetKey, protected Less

Why are we inheriting from GetKey or Less? Neither of these classes have virtual interfaces where inheritance would make it sensible to inherit from them.

It is much easier to use composition in this case.

class comparator
{
 protected: // If you must make them protected sure.
 // But that seems unnesacery. Just make the whole thing public.
 GetKey getKey;
 Less lessThanTest;
 public:
 bool operator() (Value const& x, Value const& y) const
 {
 return lessThanTest(getKey(x), getKey(y));
 }
 }; // Now its readable.

Finally analysis of User (from original comments).

I find some of your decisions odd.

Post Deleted by Loki Astari
added 270 characters in body
Source Link
Loki Astari
  • 97.7k
  • 5
  • 126
  • 341
Loading
Source Link
Loki Astari
  • 97.7k
  • 5
  • 126
  • 341
Loading
lang-cpp

AltStyle によって変換されたページ (->オリジナル) /