5

Which is the better style to follow for say, changing the voltage on a 4 channel PSU:

setChannelOneVoltage(voltage)
setChannelTwoVoltage(voltage)
setChannelThreeVoltage(voltage)
setChannelFourVoltage(voltage)

or

setVoltage(channel,voltage)

closed as primarily opinion-based by gbjbaanb, jwenting, BЈовић, Bart van Ingen Schenau, gnat Sep 30 '14 at 12:49

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.

  • one may argue that conceptually, this has been addressed in How would you know if you've written readable and easily maintainable code? If your peers keep complaining about your way of doing things, be it one way or another, you better change to make them feel better – gnat Sep 30 '14 at 8:34
  • 2
    The question is twofold. For the current example and the given set of circumstances, use approach 2. In a more general sense the answer would be "it depends" and there are more approaches than just two. – JensG Sep 30 '14 at 9:04
  • 2
    It should be noted that no ethically-trained software engineer would ever consent to write a DestroyBaghdad procedure. Basic professional ethics would instead require him to write a DestroyCity procedure, to which Baghdad could be given as a parameter. Nathaniel Borenstein – TRiG Sep 30 '14 at 10:52
12

I would definitely recommend the latter variant.

  1. It's more flexible. What if you adopt the code for another device which has not just 4 but 8, 16, 32 or even more channels? You would have to add more and more methods and your code would become more and more convoluted. The code could not be shared between the devices, because you would end up with invalid methods. But when your code leaves the number of channels open, you can use the same code for any device.
  2. When you need to set the voltage on multiple channels at the same time (either all to the same value or to different values read from a data structure), you can do this with a simple for-loop. The first version would require to call every single method separately.

By the way, for better code readability, you can replace the channel numbers by constants when you want to. This code might be ambiguous:

setVoltage(2, 4);  // Does this set channel 2 to 4V or channel 4 to 2V?

But this is much more readable:

setVoltage(CHANNEL_2, 4); // OK, channel 2 is now 4V, but what does channel 2 actually do?

This is even more readable:

setVoltage(FLUX_CAPACIATOR_CHANNEL, 4); // Puts 4V on the Flux Capaciator -- Captain Obvious
  • 1
    Yeah, I agree with the latter point. I initially went with the first method due to how unambiguous it was but it made my code more bloated. Thanks for the reply. – user124757 Sep 30 '14 at 8:58
  • I do not agree with the "more flexible" argument. There is nothing you can do with the second approach what you can't do with the first one. It may be a different amount of work, but we are talking about flexibility, not efficiency. – JensG Sep 30 '14 at 9:01
  • 2
    setVoltage(2, 4.0); is also clear, since you don't have floating-point channel numbers. – immibis Sep 30 '14 at 10:06
  • 1
    Tbh setVoltageFluxCapaciator(4) is the same as setVoltage(FLUX_CAPACIATOR_CHANNEL, 4), also, the first variant makes it a lot easier to find all calls to that channel. Internally these functions will be simpler also as you do not need to special case channel 2 and channel 4 in a if then else if then else structure. I would prefer less arguments in most situations, but that doesn't make 2 arguments wrong, it's still perfectly valid and usable code. – Dorus Sep 30 '14 at 11:38
  • 1
    @JensG That you can do anything in a Turing complete language if you rewrite code is a tautology. You could also say generics aren't more flexible because you can always copy-paste the code for each new type you want to work with. – Doval Sep 30 '14 at 11:41
4

Under the assumption that the implementations would be very similar, I would suggest having one function so for the sake of DRY. It is however also legitimate to use wrapper functions around this function to make your code more instantly readable

  • It's only a small function so I'm going to combine them. No point in repeating a lot of the same code as your link says. Thanks. – user124757 Sep 30 '14 at 9:11
  • 1
    You can still stick to DRY by adding a private function setVoltage(channel,voltage) that's called by the 4 other functions. That even allows you to add some special cases to all separate functions,for example not all of them might have the same max voltage. – Dorus Sep 30 '14 at 11:45
4

This depends on the context. Both options have its pros and cons and tradeoffs are to be made.

Both at once

Freedom of choice - allowing setChannelOneVoltage(220) as well as setVoltage(1, 220).

Everybody's happy!

But it invites inconsistency. One programmer will be using the former, perhaps unaware of the shortcut, another will prefer the latter.

Third programmer comes over, searches for all usages of setChannelOneVoltage (for some reason) and it may not occur to him that he's overlooking setVoltage(1... calls.

Is it a dealbreaker? No, but it should be taken into account. Every optional shortcut will be used inconsistently and therefore add a bit of noise.

A pet peeve of mine is the coexistence of collection.size() == 0 and collection.isEmpty(), for example. It's noisy. But since both options are equally valid, enforcing a consistent convention is hard, as it boils down to one's taste ultimately.

Shortcut methods only

Now we are hiding the setVoltage(x implementation and only make it accessible by our sugar shortcuts.

But what if you need to quickly set voltage on all channels? Instead of a simple loop, you are now writing:

setChannelOneVoltage(voltage)
setChannelTwoVoltage(voltage)
setChannelThreeVoltage(voltage)
setChannelFourVoltage(voltage)

Okay, we can enwrap this into another shortcut function: setAllChannelsVoltage.

But what if we need to set all channels apart from number 1? Do we follow up with another helper method: setAllChannelsButTheFirst...

Depending on the nature of the task, it may be hard to maintain.

Only the raw, parameterized method

It is now hard to find all usages of setVoltage(1.... You need to resort to text search, and it will fail in case of i = 1; setVoltage(i, 220) or some other of all the endless possibilities.

It makes code more abstract and adds to the number of parameters - if it's a choice between one or two, that's not a big deal, but if the number of parameters has grown beyond that already, you should think twice every time before adding just one more.

If there is only ever 4 channels and no more, setVoltage(int channel puts the code at risk of out of range exceptions, which you are protected from by design if you only provide shortcuts.

Code that contains setVoltage(5 will compile and crash in runtime, but it won't build at all if it contains a call to setChannelFiveVoltage as the method doesn't exist, making the error much easier to catch out.

To sum up

Chosing one of these design approaches requires us to answer certain questions.

Is setting voltage on channel 2 or 3 a special case in its own right so that we might want to identify it in code and easily distinguish from all the others?

Is it likely that we will need to iterate over these channels and set their voltage conditionally?

Or that we would be setting the voltage of a channel whose ordinal number is computed or derived from some other data?

Has the parameterized version been already in use for some time? In such case the resulting inconsistency would strike us harder, because preexisting code won't be using the shortcuts and other programmers won't be used to them either, and stick to what they know.

In other words - which of the pros and cons that I listed above applies in your situation and to what extent? Some may be relevant, while others not so much. Calculate the tradeoffs for the task at hand, there is no silver bullet here; YMMV.

  • In this case it's just those two parameters. I'm going to change to the latter for this case as the four channels would often be changed together. Thanks for the in depth reply. – user124757 Sep 30 '14 at 9:09
  • @user124757 yeah I understand. I deliberately tackled your problem in more general terms. – Konrad Morawski Sep 30 '14 at 9:12
3

If you are using an object oriented language, could it make sense to make the channel an object?

channels = new Channel[4];
channels[0] = new Channel();
...
channels[0].SetVoltage(220);
  • It's a good idea but in this case it wouldn't be worth it. Thanks though, I might do that in the future. – user124757 Sep 30 '14 at 9:12
  • 1
    You could still enwrap your array of channels in an object exposing ChannelOne, ChannelTwo etc. so this by itself does not solve the design dilemma. – Konrad Morawski Sep 30 '14 at 9:13
3

You could use an enumeration for your channel:

enum Channel
{
    C1,
    C2,
    C3,
    C4
};

then you can use a single method:

setVoltage(Channel.C1,voltage);
setVoltage(Channel.C2,voltage);
setVoltage(Channel.C3,voltage);
setVoltage(Channel.C4,voltage);

and you can use it in a loop:

for(Channel channel = Channel.C1; channel <= channel.C4; channel++)
{
    setVoltage(channel,voltage);
}
0

Both.

Use wrapper functions with few parameters to create intuitivity, call a single function with many parameters inside the wrappers to provide compact, coherent code.

  • 2
    It invites inconsistency, though. One programmer will be using setVoltage(1, 220), perhaps unaware of the shortcut, another will prefer the shortcut setChannelOneVoltage(220). Third programmer comes over, searches for all usages of setChannelOneVoltage (for some reason) and it may not occur to him that he's overlooking setVoltage(0 calls. Is it a dealbreaker? No, but it should be taken into account. Every optional shortcut will be used inconsistently and therefore add a bit of noise. – Konrad Morawski Sep 30 '14 at 8:34
  • This is true, which is why I'd make the shortcuts obligaory by making the core function privat in whatever way my language may offer or convention at least. – user3154108 Sep 30 '14 at 8:43
  • Every option has its pros and cons. I made it into an answer of my own. – Konrad Morawski Sep 30 '14 at 9:01
0

The question is twofold. For the current example and the given set of circumstances, use approach 2. In a more general sense the answer would be "it depends" and there are more approaches than just two.

Some more variants on the topic:

Approach 3

setVoltage( channel, voltage)
setMaxCurrent( channel, maxCurrent)
setMaxWorkingTime( channel, maxHours)
setChannelSettings( channel, voltage, maxCurrent = null, maxHours = null) 

That one is good if only a few args come into play. It quickly becomes ugly with more than 3 or 4 args.

Approach 4

struct settings {
  int channel
  nullable double voltage
  nullable double maxCurrent
  nullable double maxHours
  // add new fields here
}

setVoltage( List<settings> settings)
setVoltage( settings)

Easily extendable, can be made forward-compatible, but requires the caller to set up a rather complex structure for each call. The list<> version enables to set all values with one call, which may be important if the call is expensive, e.g. if it is an RPC with high latencies.

... and so on. As you see, all approaches have their own advantages and disadvantages, and the question Which option is better? should always be completed with Which option is better for that particular purpose, use case and set of restrictions that we face?.

0

Assuming that all channels are same, setChannelOneVoltage(voltage) and setChannelTwoVoltage(voltage) are doing the same, but on different channels. If you now can identify those channels easily by a function that takes the channel number as the argument, the second approach, using setVoltage(channelNo,voltage) would be the better one, mainly because of DRY and flexibility.

If identifying the channels is more complex (and perhaps different for each channel), you should consider to wrap each channel into an object. But then again, the second approach would be the better one, now with setVoltage(channelObj,voltage).

But if setting the voltage is significantly different for each channel, the first approach would be better; having a distinct function/method for each channel is better than a construct like

setVoltage( channel, voltage )
{
   switch( channel )
   {
      case 1: // do it for channel 1
        // Lengthy, complex code going here ...
      case 2: // do it for channel 2
        // Lengthy, complex code going here ...
      ...
   }
}

But for convenience, you can offer a setVoltage(channel,voltage) method even in this scenario; it calls the setChannel#Voltage(voltage) methods in a switch - or using reflection, if your programming language do provide such a feature - in C, you can implement something like that as a macro instead.

An alternative approach for an object oriented language would be to define a Channel interface class and setVoltage(voltage) as one of the methods. This would address DRY and flexibility as well as all the distinctions that the different channels may have in retrieval and/or setting the voltage.

-1

If you're using c#, make an extension method. that would allow you to do this: var channel1 = new channel(...); channel1.SetVoltage(15);

var channel2 = new channel();
channel2.SetVoltage(56);

some more info on extension methods: http://msdn.microsoft.com/en-us/library/bb383977.aspx

  • why did I get a -1 for this? I'd like to know what I did wrong :-) – Ruben Verschueren Oct 2 '14 at 12:18

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