1

In my code, sometimes there's a long function, to avoid messed up the local variables, i would use a pair of braces to embrace the details so the local variables will not be visible to the rest of the code, e.g.:

bool pricing_deals()
{
  //prepare deal type lookup table
  vector<DealType> deal_types; // deal type lookup table
  {
    vector<DealType> deal_types_TRF; 
    vector<DealType> deal_types_DCI; 
    ...
    // code that prepare deal_types by merging deal_types_TRF and deal_types_DCI etc
  }
  // from now on deal_types_TRF and deal_types_DCI are invisible

  //prepare ccy pair lookup table
  vector<CcyPair> ccy_pairs; // ccy pair lookup table
  {
    // code that prepare ccy_pairs;
  }
  // from now on local variables preparing ccy_pairs are invisible

  // real computation starts
  ...
}

I wonder if this is a good practice, or there are other ways of doing so?

p.s. I prefer not to break it into smaller functions in this case, as the sub-logic is not likely to be reused by others, and refactoring will cause a lot of parameters to be passed over, which would increase the complexity.

closed as primarily opinion-based by Jörg W Mittag, gnat, Basile Starynkevitch, Doc Brown, Greg Burghardt Jun 4 '17 at 19:27

Many good questions generate some degree of opinion based on expert experience, but answers to this question will tend to be almost entirely based on opinions, rather than facts, references, or specific expertise. If this question can be reworded to fit the rules in the help center, please edit the question.

6

What's needed here is abstraction.

The single most limiting thing in this situation isn't whether this can be a reusable function or help a unit test. It's whether or not you can give it a good name.

Single use private helper methods (or functions or subroutines or macros) are no sin if they come with a good name.

What's a good name? One that ensures I don't have to look inside to know what it does. If I look inside after reading the name I should find pretty much what I expected.

It's only when you can't think of a good name that it makes sense not to abstract. When a good name is available abstraction should always be preferred.

5

Normally I'd suggest exactly what you say you want to avoid: break into smaller functions.

The benefit is actually not for reusability but for unit testing. Each piece can independently be tested, which usually makes it easier to write tests. It also generally makes the tests more useful, because if there's a failure it's already more focused as to where it is.

If there are in fact a large number of common variables coming over, and they are related, I'd suggest another refactor then is to put them all into a struct or class and pass that to each function.

If they're not related, then something sounds strange about having to pass a large number to each of these sub-functions. Perhaps in this case you need to come at things with a different approach: think about how to make the smallest possible units of work (functions) that can be composed together.

There's no "one" correct approach here, it's very situational and subjective.

  • what's the benefit of "put a large number of common variables all into a struct or class"? my colleagues are against so.. – athos Jun 4 '17 at 1:56
  • 1
    @athos It depends -- if they are always being passed together, it sounds like they are probably representing a 'thing' of some sort, which is a pattern that naturally lends itself to being represented as an object. I would not put things together just for the sake of having a struct or class, though. Updated my answer with a bit of detail on this. – gregmac Jun 4 '17 at 2:27

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