Best practice for passing structure to a function and adding new fields when returning it

This is more a style question than anything else. I'm passing a structure to a function, adding some fields to the structure, and returning the structure. Which of the following is considered best practice?

Option 1: defining the fields you want to add as function outputs, passing the structure to the function, then adding each field from the function outputs.

[structa.multiplied, structa.divided, ...
    structa.added, structa.subtracted, someExtraVar] = testFunction(structa, n);

function [multiplied, divided, added, subtracted, someExtraVar] = testFunction(structa,num)
    multiplied=structa.mat1.*structa.mat2;
    divided=structa.mat3./structa.mat4;
    added=structa.mat5+structa.mat6;
    subtracted=structa.mat6-structa.mat6;  
    someExtraVar=length(structa.mat6)*num;
end

or option 2: modifying the structure within the function

[structa, someExtraVar] = testFunction(structa);

function [structa, someExtraVar] = testFunction(structa, num)
    structa.multiplied=structa.mat1.*structa.mat2;
    structa.divided=structa.mat3./structa.mat4;
    structa.added=structa.mat5+structa.mat6;
    structa.subtracted=structa.mat6-structa.mat6;   
    someExtraVar=length(structa.mat6)*num;
end

Option 1 seems better to me, since the second option is modifying globals locally within function. The first option also tells you exactly what outputs the function returns without having to actually read the function. But I do have several functions which add multiple fields to a structure and it gets cumbersome to add four different fields from function outputs to a structure. It also does affect readability when a single line of code goes on forever.

Is there an accepted best practice for this/does anyone have any opinions or suggestions?

(This is dummy code, my actual structures and functions are more complex and I didn't want to distract from the question!)

Edit: a very simple snippet from my codebase because I think the above example isn't super clear about my actual usecase (I don't love the if switch, I might rewrtie that part later)

Option 1:

function [dat.filteredSignal,dat.filteredAtEvents,dat.muFSNew,...
    dat.sigmaFSNew,dat.muFSAtEvents,dat.sigmaFSAtEvents, ...
    dat.muFS, dat.sigmaFS] = normalizeFilteredSignals(dat,params)

    assert(or(params.normalizeFS==0,params.normalizeFS==1), 'normalize FS parameter must exist and be 1 or 0');

    filteredSignal=convertVarLengthCells2Mat(dat.filteredSignalEnsemble);
    filteredAtEvents=convertVarLengthCells2Mat(dat.filteredSignalAtEventEnsemble);

    [muFS,sigmaFS] = meanAndSigma(filteredSignal);
    
    if params.normalizeFS==1
        normFunc = @(x) (x-muFS)/sigmaFS ;
        filteredSignal=normFunc(filteredSignalAll);
        filteredAtEvents=normFunc(filteredAtEvents);

        [muFSNew,sigmaFSNew] = meanAndSigma(filteredSignal);
    else
        muFSNew=muFS;
        sigmaFSNew=sigmaFS;
    end
    
    [muFSAtEvents,sigmaFSAtEvents] = meanAndSigma(filteredAtEvents);

end

function [meanDat,sigmaDat] =   meanAndSigma(inputVec)
    meanDat=mean(inputVec,'omitnan');
    sigmaDat=std(inputVec,0,'all','omitnan');
end

Option 2:

function [dat] = normalizeFilteredSignals(dat,params)

    assert(or(params.normalizeFS==0,params.normalizeFS==1), 'normalize FS parameter must exist and be 1 or 0');

    filteredSignal=convertVarLengthCells2Mat(dat.filteredSignalEnsemble);
    filteredAtEvents=convertVarLengthCells2Mat(dat.filteredSignalAtEventEnsemble);

    [dat.muFS,dat.sigmaFS] = meanAndSigma(filteredSignal);
    
    if params.normalizeFS==1
        normFunc = @(x) (x-muFS)/sigmaFS ;
        dat.filteredSignal=normFunc(filteredSignalAll);
        dat.filteredAtEvents=normFunc(filteredAtEvents);

        [dat.muFSNew,dat.sigmaFSNew] = meanAndSigma(filteredSignal);
    else
        dat.muFSNew=muFS;
        dat.sigmaFSNew=sigmaFS;
        dat.filteredSignal=filteredSignal;
        dat.filteredAtEvents=filteredAtEvents;
    end
    
    [dat.muFSAtEvents,dat.sigmaFSAtEvents] = meanAndSigma(filteredAtEvents);

end

function [meanDat,sigmaDat] =   meanAndSigma(inputVec)
    meanDat=mean(inputVec,'omitnan');
    sigmaDat=std(inputVec,0,'all','omitnan');
end

[Torsten]

I'd prefer this (i.e. making changes to the structure only in the calling program), but it's a matter of taste.

structa.mat1 = 23;
structa.mat2 = 3;
structa.mat3 = 12;
structa.mat4 = -3;
structa.mat5 = -pi;
structa.mat6 = exp(1);
num = 10;

[multiplied, divided, added, subtracted, someExtraVar] = testFunction(structa,num);
structa.multiplied = multiplied;
structa.divided = divided;
structa.added = added;
structa.subtracted = subtracted;

structa

structa = struct with fields:
          mat1: 23
          mat2: 3
          mat3: 12
          mat4: -3
          mat5: -3.1416
          mat6: 2.7183
    multiplied: 69
       divided: -4
         added: -0.4233
    subtracted: 0

function [multiplied, divided, added, subtracted, someExtraVar] = testFunction(structa,num)
    multiplied=structa.mat1.*structa.mat2;
    divided=structa.mat3./structa.mat4;
    added=structa.mat5+structa.mat6;
    subtracted=structa.mat6-structa.mat6;  
    someExtraVar=length(structa.mat6)*num;
end

[Sam Chak]

Whether Style 1 (expansive) or Style 2 (compact) is preferable depends on user preferences and the requirements of the project. Is testFunction() a standalone function, or are its outputs intended to be updated or reused in a parent function or a main script containing loops?

Style 1 generally requires users to remember the order of the output arguments. For one-time execution involving only a few outputs, Style 1 may be more convenient for most users. Even so, I often confuse the azimuth and elevation angles when using [out1, out2] = view() to readjust the camera’s line of sight with view(out1 + value1, out2 + value2).

When there are many computed outputs, Style 2 is usually more practical. For example, simOut = sim(model) contains all the data logged during the simulation, as well as metadata describing the simulation. However, you can write a wrapper function based on Style 1 to determine how the data should be handled before making changes to the structure.

You might also want to combine the advantages of both styles by using nargout, which returns the number of output arguments requested by the caller of the currently executing function.

sys = tf([1], [4 3 2 1])

sys =
 
             1
  -----------------------
  4 s^3 + 3 s^2 + 2 s + 1
 
Continuous-time transfer function.

% Style 1
[out1, out2, out3, out4] = margin(sys)    % [Gm, Pm, Wcg, Wcp] = margin(sys)

Warning: The closed-loop system is unstable.

out1 = 0.5000

out2 = -25.3875

out3 = 0.7071

out4 = 0.7966

% Style 2
structA = stepinfo(sys)

structA = struct with fields:
         RiseTime: 2.4001
    TransientTime: 46.8491
     SettlingTime: 46.8491
      SettlingMin: 0.6750
      SettlingMax: 1.4493
        Overshoot: 44.9337
       Undershoot: 0
             Peak: 1.4493
         PeakTime: 6.3852

[Stephen23]

"Is there an accepted best practice for this/does anyone have any opinions or suggestions?"

There is no single "best" approach to this. Like everything to do with code, "it depends" is the the correct answer.

"since the second option is modifying globals locally within function"

Do not let some abstract rule-of-thumb override your concrete needs and requirements. A rule of thumb is only a rule of thumb, if you can justify why your task is better suited for some other design then use that other design.

Remember that correctness is always your first priority: if this is easier to justify with option 2, then that overrides some abstract rule of thumb that might provide 3ns runtime improvement vs. failing to ensure correctness which costs you hours/days/weeks of your actual progress instead.

Personally I would favor simplicity in your situation. Often when you try something you find that several other parts of your design simplify and refactor into neater code too, which is usually a good indications that you have found a good abstraction of the task at hand. So, try option 2 and see what happens.

[Oshani]

@Sam Chak , testFunction is something I made up! I edited my original post to include a simple example of what I'm talking about from my codebase. These functions are all being used inside a parent function. The parent function has zero inputs in the actual function handle (the user is prompted to load their data using a file dialog), and returns only figures as output.

That's useful information about nargout, thank you, will look into it!

[Oshani]

@Stephen23 thank you for this! I'm pretty new to coding anything more complicated than a 10-line script so I do struggle to figure out which 'rules' I can break and which I can't.

[dpb]

"'... I'm passing a structure to a function, adding some fields to the structure, and returning the structure..."

Your sample code doesn't show the use of the function to judge, but I'm not at all in favor of the above logic/function behavior. I would strongly recommend that the struct should have its fields defined in the calling routine and the function only populate those with the proper results.

This would then have the syntax looking like Option 2, but the fields would be known a priori in the caller; they could be hardcoded as here (in which case there's no reason at all not to do so) or created dynamically if the user is to be granted the ability to define some specific statistic. But, if that were to be the case, there would have to be a way for the user to select or define what that other statistic should be and there doesn't appear to be anything of that nature being provided.

As a general stylistic note, I find the very long variable names extremely distracting and making for very difficult code to read and comprehend; this is particularly true when they all begin with a long string that is the same so it is only the ending that is different.

One minor efficiency, if you have the Statistics TB, then could do the z-statistic along with mean, std as

[muFS,sigmaFS] = meanAndSigma(filteredSignal);
if params.normalizeFS==1
    normFunc = @(x) (x-muFS)/sigmaFS ;
    filteredSignal=normFunc(filteredSignalAll);
    filteredAtEvents=normFunc(filteredAtEvents);
    [muFSNew,sigmaFSNew] = meanAndSigma(filteredSignal);
else
    muFSNew=muFS;
    sigmaFSNew=sigmaFS;
end

is

[filteredSignal,muFS,sigmaFS]=zscore(filteredSignalAll);
filteredAtEvents=zscore(filteredAtEvents);
[muFSNew,sigmaFSNew] = [0  1];

because you have to return the z-score statistic anyway or else it is undefined in the struct and unless you've got unshown initialization code in the production code not in the sample code, it will error with Option 1 on having undefined/uncalculated outputs.

As @Stephen23 points out, "simplify, simplify!". Something that was one of Einstein's notable maxims can't be wrong advice. :>)

[Steven Lord]

One minor efficiency, if you have the Statistics TB, then could do the z-statistic

Or if you're using release R2021a or later, you could call normalize with three outputs.

x = randn(1, 1e6);
[xnorm, c, s] = normalize(x, 'zscore');
format longg
[c, s; mean(x), std(x); c-mean(x), s-std(x)]

ans = 3×2

0.000605846420643827 0.999823644855965 0.000605846420643827 0.999823644855965 0 0

<mw-icon class=""></mw-icon>

<mw-icon class=""></mw-icon>

isequal((x-c)/s, xnorm)

ans = logical
   1

How does this compare with zscore?

[filteredSignal,muFS,sigmaFS]=zscore(x);
isequal(filteredSignal, xnorm)

ans = logical
   1

isequal(muFS, c)

ans = logical
   1

isequal(sigmaFS, s)

ans = logical
   1

[dpb]

"...using release R2021a or later, you could call normalize..."

Very good point; I know it's there now but I'm so ingrained with past I never remember it having been added.

Answer 1

[Steven Lord]

With option 1, the user of your code needs to know and respect the order in which you have your function return its outputs. Suppose that instead of what you wrote:

[structa.multiplied, structa.divided, ...
    structa.added, structa.subtracted, someExtraVar] = testFunction(structa, n);

they felt that addition and subtraction were more "fundamental" operations than multiplication and division and so wrote:

[structa.added, structa.subtracted, ...
    structa.multiplied, structa.divided, someExtraVar] = testFunction(structa, n);

How long would it take before they detected that their answers didn't make sense? Would they detect that their answers didn't make sense? "Silent wrong answer" bugs are the most severe category of bugs at MathWorks; crashes are as severe but in some ways they're not as bad because if MATLAB crashes you know for a fact that there's a problem.

Option 1 also has a problem with extensibility. Suppose you realized later that you needed testFunction to also return the result of raisedtopower = structa.mat9.^structa.mat10. With option 1, as which output argument do you return structa.raisedtopower? If you return it anywhere prior to the sixth output you break existing code that was written with the first five outputs having definite purposes. If you return it as the sixth output, now you have someExtraVar in the middle of outputs representing fields and it's likely new users of your code will swap the fifth and sixth output argument (to group all the "structa.<something>" outputs together).

Of course, "Option 1 seems better to me just because the second is modifying globals locally within functions, but I do have several functions which add multiple fields to a structure and it does get cumbersome." suggests that your functions might be violating the Single Responsibility principle, trying to do too much at once.

[Oshani]

This was a super helpful POV thank you!

Of course, "Option 1 seems better to me just because the second is modifying globals locally within functions, but I do have several functions which add multiple fields to a structure and it does get cumbersome." suggests that your functions might be violating the Single Responsibility principle, trying to do too much at once.

I think the second example of code better explains why I have multiple outputs!

[Steven Lord]

Looking at your somewhat more realistic (though not syntactically legal) option 1:

%{
function [dat.filteredSignal,...
          dat.filteredAtEvents,...
          dat.muFSNew,...
          dat.sigmaFSNew,...
          dat.muFSAtEvents,...
          dat.sigmaFSAtEvents, ...
          dat.muFS, ...
          dat.sigmaFS] = normalizeFilteredSignals(dat,params)
%}

Do you guard against your user calling normalizeFilteredSignals with fewer than 8 output arguments? If not, and your user does call it with fewer than 8 outputs, some of the fields won't be updated.

You're also imposing a requirement on your user to call normalizeFilteredSignals with outputs that assign values to the fields of the same struct array that you specified as the first input argument. Nothing prevents your user from calling:

%{
[dat.filteredSignal,...
 dat.filteredAtEvents,...
 dat.muFSNew,...
 dat.sigmaFSNew,...
 dat.muFSAtEvents,...
 dat.sigmaFSAtEvents, ...
 dat.muFS, ...
 dat.sigmaFS] = normalizeFilteredSignals(notdat,params) % notdat ~= dat
%}

or accidentally typing dar.muFSNew instead of dat.muFSNew as the third output argument (a typo) which would prevent the data from being changed in/added to the same struct as the rest of the outputs.

You've also separated the muFSNew and muFS outputs as well as the sigmaFSNew and sigmaFS outputs. Would users of your function expect those to be adjacent outputs (both muFS* ouputs as the third and fourth, possibly with muFSAtEvents remaining as the fifth and the sigmaFS* outputs being sixth through eighth)?

As an aside:

%    assert(or(params.normalizeFS==0,params.normalizeFS==1), 'normalize FS parameter must exist and be 1 or 0');

For this you could use the mustBeMember function.

params.normalizeFS = 0;
mustBeMember(params.normalizeFS, [0 1]) % passes
params.normalizeFS = 2;
mustBeMember(params.normalizeFS, [0 1]) % fails

Value must be a member of this set:
0
1

Answer 2

[Matt J]

Which of the following is considered best practice?

I would say the most common (and therefore perhaps the best) practice is Option 2. It's essentially what you're always doing in object-oriented programming. There is little difference between struct field modification in a function and object property modification inside a class method.

Option 1 seems better to me, since the second option is modifying globals locally within function. The first option also tells you exactly what outputs the function returns without having to actually read the function.

As a preliminary remark, I don't know what "modifying globals" means in this context. The fields of a struct do not behave like global variables in any way that is intuitive to me.

Generally speaking though, the very purpose of a function is to hide the details of what it is doing from the calling routine, and the purpose of a struct is to hide the variables that it carries. You want to do that to reduce code clutter, and Option 1 works against that. If you need visual reminders of what a function call is doing, that is normally accomplished using code comments and help documentation.

Further visuals cues can come from choosing variable and function names that are suggestive of what the variable contains and what a function call is doing. In particular, reusing the name structa for the output of,

[structa, someExtraVar] = testFunction(structa, num);

is awkward, since the output structa has new fields and therefore substantially different composition from the input. Instead, I might do,

[augmentedStruct, someExtraVar] = augmentMatStruct( structa, num);

[Oshani]

Wish I could accept two answers because this was also a really helpful POV, thank you!