1

I have a class called SolarPosition that has many member functions. I currently have a member function called CalculateSolarAzimuthAndAltitude which is a prime candidate for refactoring.

I found that the only way to refactor it to CalculateSolarAzimuth and CalculateSolarAltitude was to add an additional ten member variables, as some preliminary calculations are used by both functions.

Should I be happy with a member function with two responsibilities, or are multiple transient member variables a reasonable sacrifice in order to split it up?

François Andrieux
  • 28,148
  • 6
  • 56
  • 87
pingu
  • 8,719
  • 12
  • 50
  • 84
  • Why do you use `CalculateX` instead of `getX`? `getX` would also be `const`, no member variables would change. Unless you must cache things, which forces you to have member variables and complicated logic anyways. – nwp Mar 09 '17 at 17:18
  • Does `CalculateSolarAzimuthAndAltitude` take any arguments? – François Andrieux Mar 09 '17 at 17:49
  • 1
    It depends (Do you have a usecase that calls one of the two without ever calling the other?) and opinion. I cite `std::minmax_element` as an example of packing two different functions into one because they tend to flock together and there are significant performance benefits. – user4581301 Mar 09 '17 at 18:02
  • Sometimes it helps to have a flag that marks the current calculation results as invalid. Any changes to state can set the flag. Any request for a calculation result can return the last calculated result if the flag is false and recalculate all of the results if the flag is true. – Khouri Giordano Mar 09 '17 at 18:54
  • Does `SolarPosition` hold any state prior to calling `CalculateSolarAzimuthAndAltitude`? If there's a state, then you have all the data already, so it really matter if there's one or two methods. Otherwise, your method needs to take the same number of arguments. In any case - the class seems to be a candidate for refactoring. Maybe there's simply too much responsibility in `SolarPosition`? – Mike Wojtyna Mar 09 '17 at 22:49
  • @FrançoisAndrieux No, it doesn't. – pingu Mar 10 '17 at 10:21
  • @MikeWojtyna It does hold state, such as lat and long. I want to avoid duplication of calculations as this class has a requirement to perform fast. – pingu Mar 10 '17 at 10:24
  • @pingu So, both `CalculateSolarAzimuth` and `CalculateSolarAltitude` calculate some shared partial results? Could you provide the full contract of your `CalculateSolarAzimuthAndAltitude` method in your question (return type and arguments)? – Mike Wojtyna Mar 10 '17 at 17:39
  • @MikeWojtyna, please see https://gist.github.com/deathwishdave/8ec3c6aabfdbc86810471806b8af7cc2 (note it is a work in progress) – pingu Mar 13 '17 at 10:15

2 Answers2

4

My suggestion is to split up responsibilities into to two member functions. If that introduce ten addition transient variables, you can define a struct to group them together and define member from that struct in the SolarPosition class. that will improve readability as well.

If you still want to keep SolarPosition class much cleaner by hiding internal data variables from the class declaration, refer pimpl-idiom. for more information visit: Is the pImpl idiom really used in practice?

Community
  • 1
  • 1
curiosity
  • 66
  • 3
1

If you have that many additional variables to add, I suspect that you have an additional class hiding in your SolarPosition class. Think about extracting that class and go on from there.

Aaron
  • 874
  • 3
  • 17
  • 34