C++ Utility Functions – Is It Safe to Define for Local Use?

c

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?

Best Answer

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.

Related Topic