Discussion
· May 1, 2019

How Many Parameters Should a Good Method Have?

Hi Community!

Sometimes I meet a method which accepts 10+ parameters.

And often I need only the 8th parameter  to  pass. And I call the method something like:

do ##class(Some.Feature).Method(,,,,,,,"flag")

And I don't like this method when I call it like this cause, you know, often I just miss the number of commas and raise some other flag I wanted.

How do you avoid this situations? 

If you meet such a code, how do you call it and sure that you didn't miss the number of ","?

What is a good number of parameters in a method and f you need to pass more parameters in a method what do you do?

Discussion (27)7
Log in or sign up to continue

It's really a matter of taste.

If you don't like the long list of params (especially with lousy documented methods) you can

#1) use 1 single param and pass a local array byRef .  and decode it yourself eg:

par("IPadrr")="127.0.0.1")
par("Port")=1972
par(Namespace")="SAMPLES"

.....

do  ##class(my.par).method(.par)

or

#2) use the traditional style you may know from Caché I/O Device Guide  having also just 1 parameter

do  ##class(my.pieces).method("/SERVER=127.0.0.1:/PORT=1972:/NAMESPACE=SAMPLES")

I personally prefer #2) as it gives you an embedded doc on your intentions.

Look at this

Class User.Test
{

ClassMethod Test(Args...)
{
  ZWRITE Args
}

ClassMethod Test2(Arg1, Arg2, Arg3, Arg4, Arg5, Arg6, Arg7, Arg8)
{
  ZWRITE Arg1
  ZWRITE Arg2
  ZWRITE Arg3
  ZWRITE Arg4
  ZWRITE Arg5
  ZWRITE Arg6
  ZWRITE Arg7
  ZWRITE Arg8
}

}

and let's call it

USER>do ##class(Test).Test(1,2,,,,,,8)  
Args=8
Args(1)=1
Args(2)=2
Args(8)=8

and second method

USER>do ##class(Test).Test2(1,2,,,,,,8) 
Arg1=1
Arg2=2
Arg8=8

And another way 

USER>set args=8 ;just the number of the latest one                      

USER>set args(8)="test"

and calls

USER>do ##class(Test).Test(args...)
Args=8
Args(8)="test"

USER>do ##class(Test).Test2(args...)
Arg8="test"

Good question. Given our long history, we have a fair number of utility methods whose signatures grew organically and for which backwards compatibility goals have prevented us from doing significant cleanup. We're now in the process of reviewing the SQL utility methods and for the ones that really need as many parameters, are considering the use of an options string (aka compile flags), as is being used by some more modern infrastructure such as the work queue manager and/or just making separate specific methods rather than super-generic 10-argument ones. While we're at it, I'm eager to read suggestions or feedback on how others developing utility functions are dealing with this.

Some thoughts.

1. Do not use Hungarian notation.

2. Do not use ByRef params/dynamic objects/$lists/args... as an arguments. There are two exceptions:  you don't know the number of arguments or there's more than 10 arguments.

3. Method should not parse arguments, only validate arguments. If caller passed invalid input it's caller problem. Fail fast.

4. Mandatory arguments go first.

5. If several methods have similar signatures, the order of arguments should be the same.

6. Same logical arguments in different methods should have the same names.

7. Arguments should be consistent. If you pass primitives, pass primitives everywhere. If you pass dynamic objects, pass them everywhere.

8. Return types should also be consistent.

9. If it's object methods and you pass the same optional arguments in several methods extract these arguments as a property.
 

Guys, thanks for a lot of options! It looks like it is a relevant question ;)

Personally I feel that 3 parameters for a method are OK, but 4 is too much. But how much is for an average developer? Is there is a consensus about it?

If I know, that method will accept a lot of settings and not all of them are mandatory, I like the .args approach, which @Robert.Cemper mentioned and which, e.g. is introduced in e.g. in Database, Namespace and Web App management functions. E.g.:

set props("NameSpace")="Namespace"

set props("Enabled")=1

set props("DeepSeeEnabled")=1

Do ##class(Security.Applications).Create("AppName", .props)

It's readable and you don't count commas.

Also I'm curious if we can setup and pass a JSON that easily too? Would be great.

Also I'm curious if we can set up and pass a JSON that easily too? Would be great.

Certainly.

Class dc.test Extends %RegisteredObject
{

ClassMethod MethodTest(args As %DynamicObject)
{
 i $IsObject(args{
   args.%ToJSON(),!
   w:args.%IsDefined("arr"args.arr."2",!
 }else{
   "null",!
 }
}

ClassMethod Test()
{
  ;d ##class(dc.test).Test()
  ..MethodTest(),
    ..MethodTest({}),
    ..MethodTest({"arg1":($zts),"arg6":"hello","arr":["a",10,true,2.5674]})
}

}

Result:

USER>##class(dc.test).Test()
null
{}
{"arg1":"65136,19638.022","arg6":"hello","arr":["a",10,true,2.5674]}
1

Hi. The "clean code" people would recommend just one parameter max, and better would be none! But I think that's going a bit far, and agree that 3, or maybe 4, maximum should be aimed for to keep things easy to understand when reading, though there may be exceptions.

The array passing is a good idea to reduce the number for normal routines, but I think the ideal for classes is using objects. If you are truly embracing objects, and self-documenting code, then new classes are usually needed and what used to be parameters become the setting of properties, like this:

table=##class(CMT.UI.Table).%New()
table.TopLine=10,table.BottomLine=21
table.HeaderFormat="Underline"
table.DefineQuery("CMT.UI.PatchSite:MyList")
table.AddColumn(3,,"BOLD")
...etc.

table.Display()

It works well in some cases, but I have to admit that there is a tendency for the number of classes to get a bit silly if you take it to the extreme. Sometimes simple code is best.  :-)     / Mike

UPDATE: Oh, this technique has been mentioned already, either way this sample shows why to use it.

That's gonna be quite the bump, but it's worth mentioning. Since the adoption of %Dynamic classes, you can now provide a JSON object to methods, this way you get a syntax that ressembles POJOs (Plain Old JavaScript Objects):

ClassMethod GreetAndAsk(data As %DynamicObject) As %String
{
    return $$$FormatText("Hello %1! %2", data.name, data.message)
}

// Omitted for brevity.
write ..GreatAndAsk({
  "name": "Rubens",
  "message": "How are you?"
})

This is really useful if you must provide a method with several parameters that have some sort of link between them, here's a real use-case:

/// This method implements a full-sized configuration on how to handle multipart uploads.
ClassMethod TestPOSTMultipartFileUpload() As %Status
{
  set location = %frontier.Data.Workspace_"/fixtures/uploads/multiple"
  set destination = (location_"/:KEY/:FILE_NAME:EXTENSION")

  // 512 KB
  set maxFileSize = (1024**2/0.5)

  return %frontier.Files.Upload({
    "hooks": {
      "onItemUploadSuccess": "Frontier.UnitTest.Router:WriteResultToProcessGlobal",
      "onItemUploadError": "Frontier.UnitTest.Router:WriteErrorToProcessGlobal",
      "onComplete": "Frontier.UnitTest.Router:WriteResultSummaryToProcessGlobal"
    },
    "filters": {
      "verbose": true,
      "maxFileSize": { "value": (maxFileSize), "errorTemplate": "The file :FILE_NAME exceeded the max of :VALUE bytes." },
      "extensions": ["cls", "md", "txt", "pdf"]
    },
    "destinations": {
      "file_a": { "path": (destination) },
      "file_b": { "path": (destination) },
      "file_c": { "path": (destination), "optional": true },
      "file_d": { "path": (destination), "optional": true },
      "files": {
        "path": (location_"/files/:INDEX/:FILE_NAME:EXTENSION"),
        "slots": 3,
        "filters": { "maxFileSize": 500000000 }
      }
    }
  })
}

Herman 

I think you have it in COS since Strings became almost endless.
Par #2) of my answer showed it:

the traditional style you may know from Caché I/O Device Guide  having also just 1 parameter

do  ##class(my.pieces).method("/SERVER=127.0.0.1:/PORT=1972:/NAMESPACE=SAMPLES")

and the method end:

ClassMethod method(parameter as %String(MAXLEN="") {
   for par="SERVER","PORT","NAMESPACE" 
   set @par=$P($P(parameter,par_"=",2),":") 
   zw @par 
   #; now do something ...   
}
SERVER="127.0.0.1" 
PORT=1972 
NAMESPACE="SAMPLES"

Robert,

As to (simulated) named arguments approach:

(+): Good readability.

(-):  We should be pretty sure that values would not contain separators, neither "=" nor ":", although screening of them is not a very hard problem ("\=", "\:", "\\"). 

(-): Handling binary data (e.g., $list or $bit) seems to be more serious problem. In this case it seems that one argument structured as $lb($lb("name1",value1),...$lb("nameN",valueN)) is better option.

score

-1  #1 - disagree, see no valid reasoning

-2  #2 - strongly disagree

-1 #3 - disagree because of #2

+1 #4- agree

+1 #5 - agree

+1 #6 - agree . pls.send enhancement req. to engineering

+0 #7 - not clear about the message ? an oref is neither primitive nor dynamic

+0 #8 - miss imagination of inconsistent return types

+0 #9 - don't understand that message
 

1. Modern editors color variable based on scope. p/t in the beginning is unnecessary.

7. I mean if you develop an API all methods should accept either primitives or ByRefs or json. Not object arguments in one method, primitives in another, etc.

8. If you return %Status always return it. If you return $this always return self.

9. If your class has several instance methods and each has, let's say "debug" argument, remove this argument from methods and add "debug" as a class property.