17

During a recent project I've been working on, I've had to use a lot of functions that kind of look like this:

static bool getGPS(double plane_latitude, double plane_longitude, double plane_altitude,
                       double plane_roll, double plane_pitch, double plane_heading,
                       double gimbal_roll, double gimbal_pitch, double gimbal_yaw, 
                       int target_x, int target_y, double zoom,
                       int image_width_pixels, int image_height_pixels,
                       double & Target_Latitude, double & Target_Longitude, double & Target_Height);

So I want to refactor it to look something like this:

static GPSCoordinate getGPS(GPSCoordinate plane, Angle3D planeAngle, Angle3D gimbalAngle,
                            PixelCoordinate target, ZoomLevel zoom, PixelSize imageSize)

This appears to me to be significantly more readable and safe than the first method. But does it make sense to create PixelCoordinate and PixelSize classes? Or would I be better off just using std::pair<int,int> for each. And does it make sense to have a ZoomLevel class, or should I just use a double?

My intuition behind using classes for everything is based on these assumptions:

  1. If there are classes for everything, it would be impossible to pass a ZoomLevel in where a Weight object was expected, so it would be more difficult to provide the wrong arguments to a function
  2. Likewise, some illegal operations would cause compile errors, such as adding a GPSCoordinate to a ZoomLevel or another GPSCoordinate
  3. Legal operations will be easy to represent and typesafe. i.e subtracting two GPSCoordinates would yield a GPSDisplacement

However, most C++ code I've seen uses a lot of primitive types, and I imagine there must be a good reason for that. Is it a good idea to use objects for anything, or does it have downsides that I am not aware of?

9
  • 8
    "most C++ code I've seen uses a lot of primitive types" - two thoughts come to mind: a lot of C++ code may have been written by programmers familiar with C which has weaker abstraction; also Sturgeon's Law Mar 28, 2013 at 8:19
  • 3
    And I think it's because primitive types are simple, straightforward, fast, sleek and efficient. Now in the OP's example, it's definitely a good idea to introduce some new classes. But the answer to the title question (where it says "everything") is a resounding no!
    – Mr Lister
    Mar 28, 2013 at 10:24
  • 1
    @Max typedeffing the doubles does absolutely nothing to solve the OP's problem.
    – Mr Lister
    Mar 28, 2013 at 10:35
  • 1
    @CongXu: I had almost the same thought, but more in the sense "a lot of code in any language may have been written by programmers having problems with building abstractions at all".
    – Doc Brown
    Mar 28, 2013 at 12:22
  • 1
    As the saying goes "if you have a function with 17 parameters, you probably missed a few". Mar 28, 2013 at 15:19

5 Answers 5

24

Yes, definitely. Functions/methods that take too many arguments is a code smell, and indicates at least one of the following:

  1. The function/method is doing too many things at once
  2. The function/method requires access to that many things because it's asking, not telling or violating some OO design law
  3. The arguments are actually closely related

If the last one is the case (and your example certainly suggests so), it's high time to do some of that fancy-pants "abstraction" that the cool kids were talking about oh, just some decades ago. In fact, I'd go further than your example and do something like:

static GPSCoordinate getGPS(Plane plane, Angle3D gimbalAngle, GPSView view)

(I am not familiar with GPS so this is probably not a correct abstraction; for instance, I don't understand what zoom has to do with a function called getGPS.)

Please prefer classes like PixelCoordinate over std::pair<T, T>, even if the two have the same data. It adds semantic value, plus a bit of extra type safety (compiler will stop you from passing a ScreenCoordinate to a PixelCoordinate even though both are pairs.

2
  • 1
    not to mention you can pass the larger structs by reference for that little bit of micro-optimization all the cool kids are trying to do but really shouldn't Mar 28, 2013 at 9:05
  • Pair<int, int> is actually quite useless compared to two ints. Two ints can be given names. With a pair of two ints, you address them as “first” and”second” with no reason why one would be the first and one the second.
    – gnasher729
    Aug 31, 2022 at 9:15
6

Disclaimer: I prefer C to C++

Instead of classes, I would use structs. This point is pedantic at best, since structs and classes are nearly identical (see here for a simple chart, or this SO question), but I think there is merit in the differentiation.

C programmers are familiar with structs as blocks of data that have some greater meaning when grouped, whereas when I see a clasas, I assume there's some internal state and methods to manipulate that state. A GPS coordinate doesn't have state, just data.

From your question, it seems this is exactly what you're looking for, a general container, not a stateful structure. As the others have mentioned, I wouldn't bother putting Zoom into a class of its own; I personally hate classes built to hold a data type (my professors love creating Height and Id classes).

If there are classes for everything, it would be impossible to pass a ZoomLevel in where a Weight object was expected, so it would be more difficult to provide the wrong arguments to a function

I don't think this is a problem. If you reduce the number of parameters by grouping the obvious things like you've done, the headers should be enough to prevent those problems. If you're really worried, separate the primatives with a struct, which should give you compile errors for common mistakes. It's easy to swap two parameters next to each other, but harder if they're farther apart.

Likewise, some illegal operations would cause compile errors, such as adding a GPSCoordinate to a ZoomLevel or another GPSCoordinate

Good use of operator overloads.

Legal operations will be easy to represent and typesafe. i.e subtracting two GPSCoordinates would yield a GPSDisplacement

Also good use of operator overloads.

I think you're on the right track. Another thing to consider is how this fits into the rest of the program.

3

Using dedicated types has the advantage that it's much harder to screw up the order of arguments. The first version of your example function, for example, begins with a chain of 9 doubles. When someone uses this function, it is very easy to screw up the order and pass the gimbal angles in place of the GPS coordinates and vice versa. This can lead to very strange and hard to trace bugs. When you pass only three arguments with different types instead, this error can be catched by the compiler.

I would in fact consider to go a step further and introduce types which are technically identical but have a different semantic meaning. For example by having a class PlaneAngle3d and a separate class GimbalAngle3d, even though both are a collection of three doubles. This would make it impossible to use one as the other, unless it's intentional.

I would recommend using typedefs which are just aliases for primitive types (typedef double ZoomLevel) not just for that reason, but also for another: It easily allows you to change the type later. When you get the idea one day that using float could be just as good as double but could be more memory- and CPU-efficient, you just have to change this in one place, and not in dozends.

4
  • +1 for the typedef suggestion. Gets you the best of both worlds: primitive types and objects. Mar 28, 2013 at 13:03
  • It's no harder to change the type of a class member than a typedef; or did you mean compared to primitives?
    – MaHuJa
    Oct 28, 2013 at 14:09
  • It’s much easier to avoid screw ups if you have named parameters.
    – gnasher729
    Aug 31, 2022 at 9:17
  • @gnasher729 C++ doesn't support named parameters, though.
    – Philipp
    Aug 31, 2022 at 11:35
2

Fine answers here already, but there is one thing I would like to add:

Using PixelCoordinate and PixelSize class makes things very specific. A general Coordinate or Point2D class may probably better suit your needs. A Point2D should be a class implementing the most common point/vector operations. You could implement such a class even generically (Point2D<T> where T may be int or double or something else).

2D point/vector operations are so very common for lots of 2D geometry programming tasks that to my experience such a decision will increase highly the chance of reusing existing 2D operations. You will avoid the need for re-implementing them for each PixelCoordinate, GPSCoordinate, "Whatsover"-Coordinate class again and again.

3
  • 1
    you can also do struct PixelCoordinate:Point2D<int> if you want proper type safety Mar 28, 2013 at 13:32
  • What is a pixel size supposed to be? (0.5, 0.5) points on my Mac, or (0.6666, 0.6666) on my not quite retina screen? And point2D<int> is actually quite horrible, should be double.
    – gnasher729
    Aug 31, 2022 at 9:19
  • @gnasher729: in the OPs example, a PixelSize object does represent the size of a pixel, but the size of an image in pixels, didn't you notice? If you don't like that name, tell them, not me.
    – Doc Brown
    Aug 31, 2022 at 14:42
0

So I want to refactor it to look something like this:

static GPSCoordinate getGPS(GPSCoordinate plane, Angle3D planeAngle, Angle3D gimbalAngle,
                        PixelCoordinate target, ZoomLevel zoom, PixelSize imageSize)

This appears to me to be significantly more readable and safe than the first method.

It is [more readable]. It is also a good idea.

But does it make sense to create PixelCoordinate and PixelSize classes?

If they represent different concepts, it makes sense (that is, "yes.").

Or would I be better off just using std::pair for each.

You could start by doing typedef std::pair<int,int> PixelCoordinate, PixelSize;. This way, you cover the semantics (you're working with pixel coordinates and pixel sizes, not with std::pairs in your client code) and if you need to extend, you simply replace the typedef with a class and your client code should require minimal changes.

And does it make sense to have a ZoomLevel class, or should I just use a double?

You could typedef it as well, but I would use a double until I'd need functionality associated with it that doesn't exist for a double (for example, having a ZoomLevel::default value would require that you define a class, but until you have anything like that, keep it a double).

My intuition behind using classes for everything is based on these assumptions [omitted for brevity]

They are strong arguments (you should always strive to make your interfaces easy to use correctly and difficult to use incorrectly).

However, most C++ code I've seen uses a lot of primitive types, and I imagine there must be a good reason for that.

There are reasons; in lots of cases though, those reasons are not good. One valid reason to do this may be performance, but that means that you should see this kind of code as an exception, not as a rule.

Is it a good idea to use objects for anything, or does it have downsides that I am not aware of?

It is generally a good idea to make sure that if your values always appear together (and make no sense apart) you should group them together (for the same reasons you mentioned in your post).

That is, if you with coordinates that always have x, y, and z, then it's much better to have a coordinate class than transmit three parameters everywhere.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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