Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'd like to display a vector (implemented in my custom Vector class) and have the output width of the elements be configurable. I've done this by creating a simple wrapper for the element being displayed which stores the desired width in a static field. I'm not sure if this is the best approach. Any other suggestions would be much appreciated.

/** A struct for formatting doubles with a fixed width. */
struct FixedWidthDouble {
  FixedWidthDouble(double x) : x_(x) {}
  double x_;
  static int width_;
};

/** Apply a fixed width manipulator to a double for display. */
std::ostream& operator<<(std::ostream& out,
    const FixedWidthDouble &fixedWidthDouble) {
  return out << std::setw(fixedWidthDouble.width_) << fixedWidthDouble.x_;
}

Here's an example of it being applied:

/** Convert a Vector into a stream suitable for display. */
std::ostream& Vector::Print(std::ostream& out, int width) const {
  out << "[" << std::setprecision(1) << std::fixed;
  FixedWidthDouble::width_ = width;
  std::copy(begin(), end(), std::ostream_iterator<FixedWidthDouble>(out, " "));
  return out << " ]";
}

Is there a cleaner way to do this?

Proposed Solution

Based on feedback from Begemoth this is the solution I decided to go with. Does this look reasonable?

template<int width>
struct FixedWidthDouble {
  FixedWidthDouble(double x) : x_(x) {}
  double x_;
};

/** Apply a fixed width manipulator to a double for display. */
template <int width>
std::ostream& operator<<(std::ostream& out,
    const FixedWidthDouble<width> &fixedWidthDouble) {
  return out << std::setw(width) << fixedWidthDouble.x_;
}

/** Convert a Vector into a stream suitable for display. */
template <int width>
std::ostream& Vector::Print(std::ostream& out) const {
  out << "[" << std::setprecision(1) << std::fixed;
  std::copy(begin(), end(),
      std::ostream_iterator<FixedWidthDouble<width> >(out, " "));
  return out << " ]";
}
share|improve this question
add comment

1 Answer

up vote 4 down vote accepted

There are one problem with your approach you make local information global as consequence your FixedWidthDouble class is not thread-safe. There is a simple way to solve the problem by turning width_ into a integral template argument of the class template template<int width> FixedWidthDouble.

In C++11 I'd use range-based for:

std::ostream& Vector::Print(std::ostream& out, int width) const
{
  out << "[" << std::setprecision(1) << std::fixed;
  for (auto& x : *this)
    out << std::setw(width) << x;
  return out << " ]";
}

In the old C++98 it is cleaner to use old for with iterators. Also you can create a new iterator type that will set the width:

template<typename T, typename CharT = char, typename Traits = std::char_traits<CharT> >
class width_ostream_iterator
  : public std::iterator<std::output_iterator_tag, void, void, void, void>
{
public:
  typedef CharT char_type;
  typedef Traits traits_type;
  typedef std::basic_ostream<CharT, Traits> ostream_type;

private:
  ostream_type* stream;
  const CharT* separator;
  int width;

public:
  explicit width_ostream_iterator(ostream_type& stm, int width, const CharT* separator = 0)
    : stream(&stm), separator(separator), width(width)
  { }

  width_ostream_iterator& operator=(const T& value)
  {
    *stream << std::setw(width) << value;
    if (separator) *stream << separator;
    return *this;
  }

  width_ostream_iterator& operator*()     { return *this; }
  width_ostream_iterator& operator++()    { return *this; }
  width_ostream_iterator& operator++(int) { return *this; }
};

Update: Your proposed solution is valid if the width can only be changed at compile time, if you want to change the width at the runtime (e.g. from configuration file or command line arguments) you have to use plain for loop or the width_ostream_iterator I've provided. One thing I do not like about the width_ostream_iterator — it is definitely a library entity intended to be used in multiple places but it allows you only to change the width of the field and nothing else. It is too specific for the library (it does not allow an arbitrary manipulator to a stream before outputting a value) it is too complicated for a single use.

And one more note: the Print function uses only public interface of the vector (I assume that begin and end functions are public), so it does not required to be a member function. Prefer to make such functions free not member. Why not use member function for all class operations? You can extend the interface of a class without modifying it, e.g. you can have a Vector class and stream I/O operations defined in separate headers, you can later add another I/O operations (e.g. for QDataStream) in a third header and you will not have to recompile the parts of the program that depend only on the first and second headers. Your Print function can potentially work with any class which looks and behaves like a standard sequence, just like any algorithm from the standard library. The class (just a function) must do one thing well, the Vector class stores a number of values of some type and provides constant-time random-access to the elements, it doesn't sort elements (that what std::sort algorithm is for), it does not search for elements (that what std::find and std::find_if algorithms are for), it does nit provide elementwise arithmetic operations (oops, there is no std::zip to combine several sequences into one), etc.

share|improve this answer
    
Thank you. I was think of using template<int width> as you suggested however I wasn't sure how to write operator<< for a generic FixedWidthDouble. Could you please show how this is done? –  padawan Nov 7 '12 at 9:50
    
Thank you again. Could you expand a little on why you prefer not to make such methods a member? I come from a Java background where everything belongs to a class so I always get a strange feeling when I define a global function. Could you give some advice on when global functions are the 'right' thing to do in C++? Perhaps global functions are somewhat like static member functions in Java? –  padawan Nov 7 '12 at 13:01
add comment

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.