3
\$\begingroup\$

Background

I wrote my_length class and some code. my_length can be stringified using << operator, and main() tests this.

Questions

  1. I think operator<< can be refactored. How can I rewrite operator<< of my_length, and make it better code?
  2. How can I make rest of code better?

Code

#include <vector>
#include <string>
#include <cstdint>
#include <iostream>

class my_length {
  int id;
  std::string name;
  uint64_t v;     // unit is 'mm'
  bool fmt_auto;

  public:
  my_length (int id, std::string name, uint64_t v)
    : id{id}, name{name}, v{v}, fmt_auto(false) {}
  friend std::ostream &operator<<(std::ostream &out, const my_length &l) {
    out << l.id << "/" << l.name << " : ";
    if (!l.fmt_auto) {
        return out << l.v << "mm";
    } else {
        if (l.v<10)
            return out << l.v << "mm";
        else if (l.v<1000)
            return out << double(l.v)/10 << "cm";
        else if (l.v<1000000)
            return out << double(l.v)/1000 << "m";
        else
            return out << double(l.v)/1000000 << "km";
    }
  }
  void set_fmt_auto(bool b) {fmt_auto = b;}
};

bool need_fmt_auto(int x) {
  return (x%2) != 0;
}

int main() {
  std::vector<my_length> lengths;
  my_length l{0, "a", 1};
  lengths.push_back(l);
  uint64_t val = 17;
  for (int i=1; i<10; i++) {
    my_length l{i, std::string(1, 'a'+i), val};
    lengths.push_back(l);
    val *= 10;
  }
  for (int i=0; i<lengths.size(); i++) {
    if (need_fmt_auto(i)) {
        lengths[i].set_fmt_auto(true);
        std::cout << "fmt_auto: ";
    }
    std::cout << lengths[i] << "\n";
  }
}
\$\endgroup\$
  • \$\begingroup\$ What's the purpose of this class? Once it's initialized you can't even access v, just switch fmt_auto. In which context would one want to create an instance? Why not just use a function std::string length_with_unit(uint64_t v, bool fmt_auto=false) or something similar? \$\endgroup\$ – Yurim Dec 12 '18 at 7:06
  • \$\begingroup\$ I updated my code to clarify the purpose of my_length. \$\endgroup\$ – H. Jang Dec 12 '18 at 8:13
  • \$\begingroup\$ @Yurim Maybe std::string length_with_unit() is a good choice. But I think this function can be used only with my_length. And << operator does not create string instance unlike length_with_unit(), so it makes little advantage on performance. Because of this two reasons, I write this as << operator. \$\endgroup\$ – H. Jang Dec 12 '18 at 8:20
  • 2
    \$\begingroup\$ I still fail to imagine a use case where I would want to create an instance of my_length because all I can do with it is to write it to an std::ostream. Why would I want to write my_length l(42,name,567); l.set_fmt_auto(true); std::cout << l; instead of std::cout << length_with_unit(42, name, 567, true);` or even std::cout << 42 << '/' << name << " : " << length_with_unit(567, true);? What's your high-level motivation to write this class? \$\endgroup\$ – Yurim Dec 12 '18 at 8:33
  • \$\begingroup\$ @Yurim It is just a toy program. I wrote this code to study c++. I just wondered "my << operator is good enough" \$\endgroup\$ – H. Jang Dec 13 '18 at 8:09
4
\$\begingroup\$

I see a number of things that may help you improve your program.

Gather constants together

The relationship among the numerical values that determine which unit is used and the divisor and the actual unit name is contained within a single function, which is good, but it could be more clear if expressed as a structure. For example, one could use this:

struct Units {
  int limit;
  double divisor;
  std::string name;
};
static const std::array<Units,4> units; 

Then outside of the class, we supply the values:

const std::array<my_length::Units,4> my_length::units = {{
  { 10, 1, "mm" },
  { 1000, 10, "cm" },
  { 1000000, 1000, "m" },
  { 0, 1000000, "km" },
}};

This brings things into much closer proximity so that it can be more easily seen and understood. We can do a little better with C++17 by using std::string_view instead of std::string for the unit name.

Isolate concerns

It's usually better to have each function just do one thing. So insted of deciding on units and also printing, I'd suggest splitting those into two, using a private member function:

const Units& selectUnit() const {
  auto ret{units.cbegin()};
  if (fmt_auto) {
      while (ret->limit && v >= ret->limit) {
          ++ret; 
      }
  }
  return *ret;
}

Now the operator<< looks like this:

friend std::ostream &operator<<(std::ostream &out, const my_length &l) {
  const Units u{l.selectUnit()};
  return out << l.id << "/" << l.name << " : " 
      << l.v/u.divisor << u.name;
}

Use standard algorithms

The test code starts with this:

std::vector<my_length> lengths;
my_length l{0, "a", 1};
lengths.push_back(l);
uint64_t val = 17;
for (int i=1; i<10; i++) {
    my_length l{i, std::string(1, 'a'+i), val};
    lengths.push_back(l);
    val *= 10;
}

I'd suggest that one could use std::iota instead. I'll leave it to you to create the details of that.

Reconsider the design

Once constructed, there is no capability to do anything with the my_length object except to print it. If all that's needed is printing, then it may be better to use a freestanding function in a namespace instead of an object, as suggested in the comments. If not, then it makes sense to provide those other operations within the class.

\$\endgroup\$
  • \$\begingroup\$ You beat me to the punch on that one. The only difference I would have made is rather than the units be decided by the class get the user to define the units by allowing a manipulator that is applied first. \$\endgroup\$ – Martin York Dec 13 '18 at 20:24
  • \$\begingroup\$ You are right; that would be an improvement. \$\endgroup\$ – Edward Dec 14 '18 at 0:31

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.