Is it safe to define a utility function for local use in my .cpp file?

Question

The code listed below is intended to calculate some rotation data which will later get used in my rendering code.

renderer.cpp:

rendered_image renderer::common_tasks(render_info const& info) {
 //info.radius is a float, info.min_image_size is an int calculated by taking the minimum
 //of info.image_width and info.image_height
 cl_float raw_delta = 2.f * info.radius / info.min_image_size;
 //cl_float4 is functionally a wrapper around float[4].
 //info.rotation is a float value representing the rotation in degrees
 cl_float4 final_deltas{
 cos(to_radians(info.rotation)) * raw_delta,
 sin(to_radians(info.rotation)) * raw_delta,
 -sin(to_radians(info.rotation)) * raw_delta,
 cos(to_radians(info.rotation)) * raw_delta
 };
 //Other, irrelevant code
 /*...*/
}

But of course, I wanted to clean up the code a bit, so I rewrote it like this:

renderer.cpp:

cl_float4 get_deltas(render_info const& info) {
 //info.radius is a float, info.min_image_size is an int calculated by taking the minimum
 //of info.image_width and info.image_height
 cl_float raw_delta = 2.f * info.radius / info.min_image_size;
 //cl_float4 is functionally a wrapper around float[4].
 //info.rotation is a float value representing the rotation in degrees
 cl_float4 final_deltas{
 cos(to_radians(info.rotation)) * raw_delta,
 sin(to_radians(info.rotation)) * raw_delta,
 -sin(to_radians(info.rotation)) * raw_delta,
 cos(to_radians(info.rotation)) * raw_delta
 };
 return final_deltas;
}
rendered_image renderer::common_tasks(render_info const& info) {
 cl_float4 final_deltas = get_deltas(info);
 //Other, irrelevant code
 /*...*/
}

Now, however, I have a globally declared function get_deltas that only exists in this .cpp file. As-is, this code compiles and runs normally, but are there any significant risks to me writing code like this? Should I be nesting the function in a namespace or declare it private in the renderer class? I know for a fact that I will never need to use get_deltas anywhere else in my code: does that change whether this is a good or bad use of this code style?

Answer 1

As a matter of principle private or local methods should always either be private members of the class or static local functions. Declaring a function as static it becomes file scope and the linker will throw an error if it is attempted to be used outside of the scope of the file that it is declared in.

You can also use the anonymous namespace to achieve the same thing which is more verbose but some places prefer.

One of the major reasons is that, while you may never intend to use get_deltas elsewhere someone else might at sometime in the life of the program, possibly as a typo, and then there is a reasonable chance that the program will compile and link without errors, (you should get a warning), but will have an obscure and hard to locate bug.

• Xirema: I went with the anonymous Namespace for my code. All of this was good information though.
• Pete Kirkham: It's only more verbose if you only have the one function in it.

Answer 2

In addition to Steve Barnes answer:

1. Making the function a private method is the clean OO way; I would prefer this.

2. Using an anonymous namespace is the "new" C++ way; I'm using it time and again, e. g. for local internals (constants etc.) that may not leak to the outside, and are never ever thought to be used by derived classes. It's still not clean OO, though.

   namespace {
    const int myMagicNumber = 0xdeadbeef;
   }
   

3. Using static is the old C way; in my projects, depending on the context, I usually move these facilities into an anonymous namespace or make them a member when I find one.

• Sjoerd: It doesn't matter whether it's clean OO or not - C++ is a multi-paradigm language. Either 1) or 2) is fine, mostly depending on personal taste.
• gnasher729: Just using "static" changes it from awful to somehow reasonable. So it's much better to just add static then to think about the other (better) methods and not doing them out of laziness.
• Phil1970: Well using an anonymous namespace could be useful to reduce recompilation... but it would make sense only if you don't need much access to other members...
• Murphy: @Phil1970 Indeed, anonymous namespace and static would be simple (and IMHO awkward) ways to emulate some of the advantages of the Pimpl idiom (aka. D-Pointer).
• Pete Kirkham: You need to explain why it is 'clean OO' to make a function which takes a render_info and returns a cl_float4 and has nothing to do with a renderer a method of renderer which breaks encapsulation (a change to its signature requires recompilation of client code).

Answer 3

In general my answer would be the same as Steve Barnes' answer, with one addition:

If there is a future need to perform unit-testing on the sub-function that you have now extracted (e.g. with GTest), you will need to make that callable from the outside. This means the function prototype need to be declared in a *.h file. Furthermore, it cannot be a private function inside a class, and it cannot be inside an anonymous namespace.

The question as to whether you should be unit-testing the publicly callable function or the now-extracted utility function is best left to you, since you have the best knowledge of your project. There are general guidelines which are covered in most unit-testing textbooks.

• Steve Barnes: One "trick" that I have seen for testable code that also respects scope limiters such as private is to have them marked PRIVATE_TESTABLE where this is defined as blank/public in in unit test builds and private/static otherwise.