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:
- If there are classes for everything, it would be impossible to pass a
ZoomLevelin where aWeightobject was expected, so it would be more difficult to provide the wrong arguments to a function - Likewise, some illegal operations would cause compile errors, such as adding a
GPSCoordinateto aZoomLevelor anotherGPSCoordinate - Legal operations will be easy to represent and typesafe. i.e subtracting two
GPSCoordinates would yield aGPSDisplacement
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
Yes, definitely. Functions/methods that take too many arguments is a code smell, and indicates at least one of the following:
- The function/method is doing too many things at once
- The function/method requires access to that many things because it’s asking, not telling or violating some OO design law
- 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
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.
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
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
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.