Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

My job is to refactor an old library for GIS vector data processing. The main class encapsulates a collection of building outlines, and offers different methods for checking data consistency. Those checking functions have an optional parameter that allows to perform some process. For instance:

std::vector<Point> checkIntersections(int process_mode = 0);

This method tests if some building outlines are intersecting, and return the intersection points. But if you pass a non null argument, the method will modify the outlines to remove the intersection.

I think it's pretty bad (at call site, a reader not familiar with the code base will assume that a method called checkSomething only performs a check and doesn't modifiy data) and I want to change this. I also want to avoid code duplication as check and process methods are mostly similar.

So I was thinking to something like this:

// a private worker
std::vector<Point> workerIntersections(int process_mode = 0)
{
    // it's the equivalent of the current checkIntersections, it may perform
    // a process depending on process_mode
}


// public interfaces for check and process
std::vector<Point> checkIntersections()  /* const */
{
    workerIntersections(0);
}


std::vector<Point> processIntersections(int process_mode /*I have different process modes*/)
{
    workerIntersections(process_mode);
}

But that forces me to break const correctness as workerIntersections is a non-const method.

How can I separate check and process, avoiding code duplication and keeping const-correctness?

share|improve this question
add comment

2 Answers

up vote 0 down vote accepted

I think you're on the right track with revising the function names as you've suggested. That will allow you to clarify the intent of what is happening as well as minimizing code duplication.

I would suggest calling processIntersections() something different though. updateIntersections() or checkAndUpdateIntersections() would be two suggestions. The intent is to get the function name closer to what is actually occurring.

I'm not sure I understand the concern over 'const-correctness'. You said that workerIntersections would be a private method, so it's effectively hidden outside of that class. If the concern is about providing a read-only guarantee to callers of checkIntersections(), then I would say that putting const on checkIntersections() is a nice-to-have, but isn't necessarily required. If I had to pick between that guarantee versus minimizing duplicate code, then I'll go with less code. Given that this is an internal API used by your firm, I wouldn't worry about the guarantee.

But that having been said... If the guarantee is really important, then consider breaking the functionality of workerIntersections() apart. What you've done so far is to put a facade in front of the original function in order to clarify intent. In order to provide the const guarantee, you'll need to break the original function into a const checkOnly() function and a makeUpdates(int mode) function.

Whether or not you continue with the facade that you've suggested is really up to you. Since you already have other callers providing a mode, you might want to keep the facade or it might be just as easy to refactor those at the same time you break apart the original function.

share|improve this answer
    
The problem with const-correctness is that workerIntersections() is non-const, so it cannot be called in a const method. Thus, checkIntersections() cannot be const. –  undu Dec 11 '12 at 15:50
    
@undu - That makes your concern more clear, but I'm still closer to the camp of "so what?" I'll update my answer with some additional thoughts on this line. –  GlenH7 Dec 11 '12 at 16:04
add comment

Just change the name of checkIntersections to checkOrProcessIntersections.

On more serious note :-
You said this is an old library. So if you change the behaviour isn't the old customer will be affected? My advice will be to keep the checkIntersections. Add two more methods strictCheckIntesection or onlyCheckIntersection and processIntersections. And then from your checkIntersections call these two methods, depending on the input parameter.
Also didn't get your worry about the constness. If some function is changing the state, it is changing the state. It is not going to be const in any case.

share|improve this answer
    
This is an old library used only by our own internal tools, which will also be rewritten, so there is no problem with breaking changes (the old version of the library will be kept in maintenance mode for a short period of time). My concern with const is that with the method I propose, check cannot be const. –  undu Dec 11 '12 at 10:34
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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