OOP question: constructor has too many parameters, what to do?

I have not encountered this before since my past class designs don't have very large class defs. Now I am tackling a problem that would require a class with a couple dozen member variables/properties. If I want to assign values to all these variables with the constructor, my constructor will be extremely long with a couple dozen variables. That's not very good-looking plus it's prone to mistakes of misaligned numbers being assigned to wrong variables and hard to read. I wonder if there is a few standard ways to solve this problem.

My current thoughts: 1) create a shadow struct, fill the struct with numbers and pass the struct to constructor so the constructor extracts values from it. 2) Have no argument for the constructor and pass variables one at a time with member functions such instance1.setVal1(100); etc.

I appreciate all the help!!! Thanks in advance!

Now I am tackling a problem that would require a class with a couple dozen member variables/properties.

Doesn't sound like a very cohesive class, to me.

The best thing to do, in my opinion, is to re-think this design. But, if you are wedded to this class, make a constructor that takes only a minimum number of arguments, and assigns default values to the rest.

Provide members that can set those other fields to other than the default values.

Thanks Paul. I'll use your suggestion.

BTW, could you elaborate on the "not so cohesive" point?

What I am making is a relay class that interacts with the user. A user may set the relay to automatically cycle on and off with on and off times, get triggered by various trigger values with different modes (say, turn on relay when sensor X goes above value of Y).

Now I am tackling a problem that would require a class with a couple dozen member variables/properties.

How many of these couple of dozen member variables apply to the one relay?

I would think that you would have a method that says "toggle relay n every m seconds" and another one that says "turn relay n on if pin m goes high". I can't think how you would organize the input arguments to have one method that says "toggle relay n every m seconds unless pin o goes to state p, in which case you should...".

The RelayBoard class should perhaps have several Relay instances, instead of one monolithic class that manages all the relay behavior.

A cohesive class does one thing. It sounds like your is trying to do two things - manage a collection of relays, and manage the behavior of those relays, too. A class for the relay and a class for the board would result in more cohesive classes.

Got it Paul.

I wasn't clear before. This should be a relay control class and each instance controls one relay with rules. There is a whole zoo of logic behind how this relay should run and for how long.

The relay can be in standby mode where it does nothing and it is off If it is in the auto mode, it runs on rules as such:

1) Under cycled rule, it turns on for x minutes and off for y minutes and keeps doing it 2) Under sensor rule, it monitors the sensor sx reading and uses a rule to decide what to do. Say if sensor sx is above its alarm value, run for 5 minutes and retest the rule in 30 minutes. 3) Under schedule rule, it turns on and off per schedule

Rule 2 has sections that make up one condition, [sensor sx] is [above/below] [target/alarm] value, then run relay with [cycled/continuous/fixed time] and [retest in z sec/min/hr/no retest].

These rule details are supposed to be member variables and a .run() takes in all these and executes the rules.

So, I'd see a relay class with a constructor that takes a one argument - the pin that the relay is triggered by. It would default to standby mode. Then, I'd see various members that set, for instance, the mode to cycle mode and defines the on and off times.

PaulS:
So, I’d see a relay class with a constructor that takes a one argument - the pin that the relay is triggered by. It would default to standby mode. Then, I’d see various members that set, for instance, the mode to cycle mode and defines the on and off times.

Thanks Paul!

Functions like this list?

  void engage();                            ///< This engages the relay
  void disengage();                         ///< This disengages the relay
  void update_name(char* new_name);         ///< This updates the relay name
  void get_name(char* buffer);              ///< This fills the buffer with the relay name. Buffer needs to be 6 or more char long.
  void get_full_name(char* buffer);         ///< This fills the buffer with "Relay(number)-(name)" format. Buffer needs to be 14 or more char long.
  byte get_number();                        ///< This returns the relay serial number
  void set_number(byte b);                  ///< This sets the relay serial number
  byte get_status();                        ///< This returns the relay status, on or off
  byte get_pin();                           ///< This returns the relay pin
  void set_pin(byte b);                     ///< This sets the relay pin
  byte get_standby_status();                ///< This returns the relay standby status
  void set_standby(byte b);                 ///< This puts the relay on standby
  int get_relay_template();                 ///< This returns the relay template number
  void set_relay_template(int i);           ///< This sets the relay template number
  byte get_ctrl_by();                       ///< This returns the relay ctrl_by
  void set_ctrl_by(byte b);                 ///< This sets the relay ctrl_by
  byte get_auto_standby_status();           ///< This returns the relay auto standby status
  void set_auto_standby(byte b);            ///< This sets the relay auto standby
  int get_ctonm();                          ///< This returns the relay cycle_timer_on_min
  void set_ctonm(int i);                    ///< This sets the relay cycle_timer_on_min
  int get_ctoffm();                         ///< This returns the relay cycle_timer_off_min
  void set_ctoffm(int i);                   ///< This sets the relay cycle_timer_off_min
  int get_ctrl_sensor();                    ///< This returns the relay ctrl_sensor
  void set_ctrl_sensor(int i);              ///< This sets the relay ctrl_sensor
  byte get_sensor_above_below();            ///< This returns the relay sensor_above_below
  void set_sensor_above_below(byte b);      ///< This sets the relay sensor_above_below
  int get_sensor_driven_fixed_time();       ///< This returns the relay sensor_driven_fixed_time
  void set_sensor_driven_fixed_time(int i); ///< This sets the relay sensor_driven_fixed_time
  byte get_sensor_target_alarm();           ///< This returns the relay sensor_target_alarm
  void set_sensor_target_alarm(byte b);     ///< This sets the relay sensor_target_alarm
  byte get_sensor_driven_action();          ///< This returns the relay sensor_driven_action
  void set_sensor_driven_action(byte b);    ///< This sets the relay sensor_driven_action
  byte get_sensor_retest_unit();            ///< This returns the relay sensor_retest_unit
  void set_sensor_retest_unit(byte b);      ///< This sets the relay sensor_retest_unit
  int get_sensor_retest_delay();            ///< This returns the relay sensor_retest_delay
  void set_sensor_retest_delay(int i);      ///< This sets the relay sensor_retest_delay
  void serialize(char* buffer);             ///< This serializes the relay instance for transfer or storage
  int get_uptime();                         ///< This returns the relay uptime in minutes
  void clear_uptime();                      ///< This resets the relay uptime
  void run();                               ///< This runs the relay routine with all the rules and logic considered.

BTW, if you need to give an instance a unique id, do you set up a static member variable to hold the next id and use this variable in constructor? I never had the need for this thing :blush: :blush:

Functions like this list?

Yes. Notice that none of them take an excessive number of arguments.

BTW, if you need to give an instance a unique id, do you set up a static member variable to hold the next id and use this variable in constructor? I never had the need for this thing

Yes, that would work. Although I would think that it would be up to the user to assign a unique name, if that is necessary.

I'm having trouble figuring out why it is needed, since that breaks encapsulation. An instance of the class ought to happily exist with 0, 1, or a bazillion other instances of the class with conflict.

If the user of the class is passing the unique ID in to make the correct instance do something, the class is not properly encapsulated.

Paul,

Thanks for the help. The purpose of the unique id is for display purpose. Say I create relay 1 and then next I create is going to be relay 2. When they are listed under a select menu, they will be: relay 1-water pump relay 2-lights

It's also easy to sort them when displaying (thinking too much). Is this a bad practice? :cold_sweat:

You could have a static class member that is the number of instances that have been created. Thus each one could have a numeric ID. However if you delete one (would this happen?) then you will have gaps.

It's also easy to sort them when displaying (thinking too much). Is this a bad practice?

I would think that they should be listed in the order created, if you storing an array of them. Then, they have numbers. They would have names, I think, that the user gave them.

Not that there is anything wrong with using a static member variable to track the next available number.

[quote author=Nick Gammon link=topic=95381.msg716658#msg716658 date=1331069609] You could have a static class member that is the number of instances that have been created. Thus each one could have a numeric ID. However if you delete one (would this happen?) then you will have gaps.

[/quote]

Potentially you can delete an instance yes then there will be a gap.

PaulS: I would think that they should be listed in the order created, if you storing an array of them.

Will do that! Thanks!