Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I'm trying to achieve, for lack of a better term, a "view" pattern in C++. It's probably most comparable to "views" in the Database world, where one can make a query, and then perhaps aggregate the data, or turn otherwise interact with it without actually duplicating the data.

I've tried to reduce it down to a simple example, where the Data is any integer, and the View is the opposite of that integer (x, -x). This example is pretty contrived, but when we're talking about matrices or other complex forms of data, I could see this pattern become quite useful, especially with respect to generic programming

The code compiles, and fundamentally, works. Unfortunately, there are alternative implementations of this pattern, and that's why I'm asking for a code review. In the long term, are there any reasons that I should be concerned about using this design pattern in my code? Perhaps with respect to:

  1. Avoiding, Identifying Bugs
  2. Class Flexibility
  3. Code Readability

Some alternative implementations I can think of:

  1. Using raw pointers instead of const references for the Data class
  2. Using inheritance and implementing the "view" functions within the Data class

There are some important things to note about this code as well. The View class can not be copied or moved, meaning that its lifespan is identical to that of the Data class. There is also the flexibility of being able to mutate the View class when Data::view() is called, for example, setting a scalar. The two classes are mutual friends, but the Data only really has a use for the constructor of the View.

Data.h

#ifndef DATA_H
#define DATA_H

#include "View.h"

class Data {
public:
  Data(const int value) :
    view_(*this),
    value_(value)
  {}

  const View& view() const { return view_; }

  int value() const { return value_; }

private:
  friend View;

  const int value_;

  const View view_;

};

#endif // DATA_H

View.h

#ifndef VIEW_H
#define VIEW_H

class Data;

class View {
public:
  View() = delete;
  View(const View&) = delete;
  View(View&&) = delete;
  View operator=(const View&) = delete;
  View operator=(View&&) = delete;

  int value() const; 

private:
  friend class Data;

  const Data& data_;

  View(const Data& data) : data_(data) {}

};

#endif // VIEW_H

View.cpp

#include "View.h"

#include "Data.h"

int View::value() const {
  return -data_.value_;
}

Example.cpp

#include "Data.h"

#include <iostream>

int main() {
  Data data{ 5 };

  const View& view = data.view();

  std::cout << data.value() << std::endl;
  std::cout << view.value() << std::endl;
}
share|improve this question
    
Welcome to Code Review! This is a very nice first question. – Alex A. yesterday
    
Thank you, I'll definitely keep an eye out if I need to clarify anything – matt_fabina yesterday
    
I don't have enough points at this part of the stackexchange network, but if I did, I would revert your edit, @matt_fabina. You aren't supposed to edit your code at this site in response to the answers you have received. In a real world code review, the code undergoing review is not changed during the course of the review. (Afterwards, yes.) The review is of the code at the time the review started, warts and all. The same concept applies at this site. – David Hammen yesterday
    
@DavidHammen oops, I figured it was more of an issue of "this won't compile unless I add this in" rather than taking a specific piece of advice and applying it to my code. – matt_fabina yesterday
up vote 1 down vote accepted

Lesser item first:

class View {
 public:
   View() = delete;
   View(const View&) = delete;
   View(View&&) = delete;
   View operator=(const View&) = delete;
   View operator=(View&&) = delete;
   ...
 private:
   ...
   Data& data_;
   View(const Data& data) : data_(data) {}
 };

You have provided a non-default constructor. This means the default constructor is not implicitly declared. You also have a data member that is of a reference type. Even if you hadn't provided a non-default constructor, the default constructor would have implicitly defined as deleted. Bottom line: You don't need to say View() = delete.

There's also no need for defining all four of the copy and move constructors and assignment operators as deleted. The very nice capability of defining a special member function as deleted did not exist in older versions of C++. One insteadhad to use the ugly trick of declaring that special member special member function as private but never providing an implementation.

You are obviously using C++11 (or perhaps even C++14), so you have this nice capability available (and you are using it). You should also understand the rules by which the compiler implicitly declares and defines special member functions that you have not explicitly declared.

  • The move constructor and move assignment operator are not implicitly declared if the code explicitly declares any one of a copy constructor, a copy assignment operator, or the destructor. Declaring any one of those C++03 special member functions for a class essentially turns off move semantics for that class. This is essential in making C++11 backward compatible with C++03 code.

  • The move constructor is not implicitly declared if the code explicitly declares a move assignment operator. The same applies to the move assignment operator if a move constructor is explicitly declared.

  • The copy constructor and assignment operator are implicitly declared and defined as deleted if the code explicitly declares a move constructor or a move assignment operator.

What this means is that defining the move constructor (or the move assignment operator) as deleted implicitly knocks out the other three.

All you need is

 class View {
 public:
   View(View&&) = delete;
   ...
 private:
   ...
   Data& data_;
   View(const Data& data) : data_(data) {}
 };

Since the non-default constructor is private, specifying View() = delete is okay for readability reasons. On the other hand, when I see a slew of XXX = delete definitions where only one is needed is a sign to me that the author of the code doesn't know the language and that I should poke deeply into that code during review.


Regarding how you are using views

There are multiple schools of thought on what "views" are and how they should be implemented. One is that the thing being viewed should know all about the view. Another is that the thing being viewed should know nothing at about the view. Both can be correct. I'll give two concrete examples to illustrate.

One example is JSON. There are seven types of JSON values: An object, an array, a string, a number, and the three special values true, false, and null. The two boolean values can obviously be collapsed to one type, and a numerical value can be expanded to two types (floating point or integral). It helps very much to have different views on a value that implement the details. Most reasonable C++ implementations of JSON do exactly that. Since a specific JSON value can be of one and only one of those types, most implementations contains an enum value that indicates the type and a union of views, only one of which is active, the view corresponding to the enum value).

The other example is very large multidimensional arrays of numerical data. There are a number of different packages that address this problem, and in a variety of ways. Very few use standard library containers. A std::vector<std::vector<std::vector< ... std::vector<double>...>>> is a phenomenally bad idea. Sometimes these arrays are so very large they will not fit into the memory of the largest of computers. Suppose you have found such an array of interest, representing years or even decades of data collected around the globe, but you are only interested in a couple of days worth of data over a tiny portion of the Earth. What you want is a view on that huge array that easily lets you slice and dice that huge array down to a very small size. In this case, the underlying huge array does not need to know that the view even exists. On the other hand, the view needs to know all about that huge array.

You have obviously taken the first approach. Whether that is the correct approach depends very much on what you are trying to view and why you are trying to view things that way.

share|improve this answer
    
Thank you for really clarifying the deleted constructor situation. My follow up question would be, is there a C++ concept name for such objects (stroustrup.com/C++11FAQ.html#concepts)? They're certainly not Regular. Additionally, is there a standard way to approach them in terms of syntax (i.e. only deleting the move constructor)? – matt_fabina yesterday
    
Also, thank you for touching on the View pattern. I'm very interested in some real world examples, or write-ups about it in general. I'm familiar with Eigen::Map and std::string_view, but that's about it. – matt_fabina yesterday
1  
@matt_fabina - Eigen is a very nice scientific library. A number of their expression templates would be called "views" in another setting. Another nice scientific library for C++ is Armadillo. There are a lot of not so nice ones that are thin wrappers over the C version (which in turn is often a thin wrapper over the Fortran version). For non-scientific programming, a number of JSON implementations are implemented with view. One I like (the name is a killer!) is github.com/tgockel/json-voorhees . – David Hammen 20 hours ago
1  
Regarding those unmovable, unassignable objects -- there are a few of those in the C++11 library. std::mutex, for example. – David Hammen 20 hours ago
  • The intentions are clearly expressed, the code is clean and easily understood. As far as I can see there is no obvious problems.

  • clang-600.0.57 failed to compile the example:

    ./view.h:15:9: error: unknown type name 'Data'
      const Data& data_;
            ^
    ./view.h:16:14: error: unknown type name 'Data'
      View(const Data& data) : data_(data) {}
                 ^
    

    I don't know what Standard mandates here. Fix is simple: add a forward declaration class Data; to view.h.

  • In general I am not sure that this is a right approach. Data must be aware of View anyway (for example, to add another View you need to modify Data). So implementing views as member functions seems more direct.

share|improve this answer
    
I think MSVC14 (what I used) interprets "friend class Data;" as a forward declaration, but I'll keep an eye out for that in the future. msdn.microsoft.com/en-us/library/465sdshe.aspx – matt_fabina yesterday
    
I also don't like that Data is dependent on View, I think that's one of the biggest drawbacks. But there is some benefit in having the function implementations of the view separate from the main class: e.g. similarly-named functions could do different things in different views – matt_fabina yesterday
    
@matt_fabina That was my understanding as well, but Standard has the last word. (Un)fortunately I am not a C++ lawyer. – vnp yesterday

Seems completely fine, depending on your use-case. I think you've over-engineered your trivial example a little bit; e.g., you provide a View.cpp containing nothing but a single one-line function. You could have inlined it into View.h (making View a header-only class), and in practice you probably would.

I can think of two things that might be useful in a less trivial example — or, they might be further examples of overengineering.


If you're worried about dangling pointers, perhaps you should have the View contain a shared_ptr<Data> instead of a raw Data&.

class Data;

class View {
    View(std::shared_ptr<Data> p) : data_(std::move(p)) {}
    int value() const;
    std::shared_ptr<Data> data_;
    friend class Data;
};

class Data : std::enable_shared_from_this<Data> {
    int value_;
  public:
    View view() { return View(this->shared_from_this()); }
};

inline int View::value() const { return -data_->value_; }

However, this assumes that your Data objects will basically only ever be allocated on the heap, via std::make_shared<Data>() or the like. You'd pretty much have to commit to the idea that at least in this part of your codebase, you'll be doing a lot of reference-counting and heap-allocating.

(In exchange, you'll be 100% free of memory leaks and dangling pointers. It might be worth the tradeoff.)


Orthogonal to the first idea, here's another one. Right now you have View knowing about Data and vice versa, which is pretty ugly if your attitude is that Data is the "real" object and View is just an auxiliary. (Think string versus string_view, for example.) (It might be different if you're coming from a Model-View-Controller world, and you think of Data and View as co-equals.)

Anyway, if you think of View as a second-class citizen, it's annoying that View.h has to include Data.h, isn't it? Well, you could get around that (and turn View into a header-only class) by turning View into a class template:

template<class D>
class View {
    View(D *p) : data_(p) {}
    int value() const { return -data_->value_; }
    D *data_;
    friend class D;
};

class Data {
    int value_;
  public:
    View<Data> view() { return View<Data>(this); }
};

inline int View::value() const { return -data_->value_; }

You could also introduce a helper class Viewable<D> implementing the view() method, and have Data inherit that method from Viewable<Data>. That's called the Curiously Recurring Template Pattern (CRTP), and you've already seen it in action, in std::enable_shared_from_this above. This would be useful if you're going to have the same kind of view on many different kinds of data:

class IntegerData : NegViewable<IntegerData> { ... };
class RealData : NegViewable<RealData> { ... };
class NaturalData : NegViewable<NaturalData> { ... };
class PositiveData : NegViewable<PositiveData> { ... };

and if you give that view() method a more descriptive name, like neg_view(), then you could also implement several different kinds of views, e.g. a RecipViewable<D> class template that provides reciprocal_view().

class NaturalData : NegViewable<NaturalData> { ... };
class PositiveData : NegViewable<PositiveData>, ReciprocalViewable<PositiveData> { ... };

Here NaturalData provides the neg_view() method, whereas PositiveData provides both neg_view() and reciprocal_view().

However, as I said before, all this CRTP stuff is unlikely to be useful if your code operates in terms of "one kind of Data, one kind of View".

share|improve this answer
    
Unfortunately the View class can't be header-only, because that would create a cyclical dependency with the #include macros. The CPP file allows me to circumvent that cyclical dependency. I agree the function could be inlined though. I did visit the shared_ptr idea as well, but thought the heap restriction was far too limiting. – matt_fabina yesterday

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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