Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"Default arguments are insane" needs explanation #116

Closed
RichardBradley opened this issue Aug 4, 2014 · 38 comments

Comments

@RichardBradley
Copy link

commented Aug 4, 2014

Please could you expand on why wartremover thinks default arguments are "insane"?

The justification for this language feature is explained somewhat at http://docs.scala-lang.org/tutorials/tour/default-parameter-values.html

This feature is widely used in practice, see for example various default argument values to express suggested sensible defaults for configuration values in spray.can.Http

@vendethiel

This comment has been minimized.

Copy link

commented Aug 4, 2014

(in your example, they're merely used as a "default config object")
Usually, you're better off with another function that does one thing, doesn't break currying, etc.

@RichardBradley

This comment has been minimized.

Copy link
Author

commented Aug 4, 2014

(in your example, they're merely used as a "default config object")

Does that make it OK?
Should the documentation be updated to say "Scala allows methods to have default arguments. This is insane, except for mere config objects."?

It will be tricky to update the wart to detect if the default argument is acceptable under that exemption.

Usually, you're better off with another function that does one thing, doesn't break currying, etc.

Usually or always? The wart and the "insane" wording strongly suggests the latter.

If this is to be pushed as a good coding practice in Scala, I think it needs some documentation to justify it, with explanation, examples and tips on how to convert to alternate styles.

For example what about:

  class MyCollection[T] {
    /**
     * Displays all elements of this list in a string using start, end, and separator strings.
     */
    def mkString(start: String = "", sep: String = "", end: String = ""): String
  }

How should that be "fixed"? The scala stdlibs use 3 separate overloads to implement the default arguments (as they were implemented before the language had default argument support). Is that what wartremover recommends? Why would that be better?

@puffnfresh

This comment has been minimized.

Copy link
Collaborator

commented Aug 4, 2014

class MyCollection[T] {
  def mkStringWithDelimiters(start: String, sep: String: end: String): String
  def mkString: String = mkStringWithDelimiters("", "", "")
}

It's better because it allows us to better treat methods as functions. See the bottom of Rob Norris' blog post on Scala's treatment of methods vs functions.

@puffnfresh

This comment has been minimized.

Copy link
Collaborator

commented Aug 4, 2014

Clarified a little in 6210285.

@vendethiel

This comment has been minimized.

Copy link

commented Aug 4, 2014

Does that make it OK?

No, I don't think it does. I think it'd be better off as an (immutable) instance you'd use as a "base", instead of a functions with 10 arguments.

"insane" wording

(seems to be removed as of said 6210285)

@RichardBradley

This comment has been minimized.

Copy link
Author

commented Aug 4, 2014

It's better because it allows us to better treat methods as functions. See the bottom of Rob Norris' blog post on Scala's treatment of methods vs functions.

I don't find that very convincing as a rationale for banning default arguments.

The blog post is about methods vs functions and notes that "Default arguments are ignored for the purposes of η-expansion". So methods will default arguments can be used as functions just fine.

I suppose it is very slightly awkward to use methods with many default arguments as functions, but they are generally used for config values and the like, so tend not to be used in meta-programming and there is an easy workaround:

scala> def tcpConnect(host: String, port: Int = 80, sslEncryption: Boolean = false, localAddress: Option[InetSocketAddress] = None, options: immutable.Traversable[Inet.SocketOption] = Nil, settings: Option[ClientConnectionSettings] = None): Unit = println("connected")
tcpConnect: (host: String, port: Int, sslEncryption: Boolean, localAddress: Option[java.net.InetSocketAddress], options: scala.collection.immutable.Traversable[akka.io.Inet.SocketOption], settings: Option[spray.can.client.ClientConnectionSettings])Unit

scala> val hosts = List("google.com", "example.com")
hosts: List[String] = List(google.com, example.com)

scala> hosts.map(tcpConnect)
<console>:23: error: type mismatch;
 found   : (String, Int, Boolean, Option[java.net.InetSocketAddress], scala.collection.immutable.Traversable[akka.io.Inet.SocketOption], Option[spray.can.client.ClientConnectionSettings]) => Unit
 required: String => ?
              hosts.map(tcpConnect)
                        ^

scala> hosts.foreach(host => tcpConnect(host))
connected
connected

Perhaps there is a case to be made that default arguments should be banned outright as they are "insane", but it hasn't been made by this wart.

My point in this issue isn't primarily to debate whether default args are insane or not, but to suggest that a linter rule which deprecates a feature really needs a) a convincing explanation of what's wrong with that feature and b) suggestions on how to convert problem code to alternatives, both of which are currently missing here.

@puffnfresh

This comment has been minimized.

Copy link
Collaborator

commented Aug 4, 2014

Many convincing explanations exist but I don't think it's WartRemover's job to repeat them.

@RichardBradley

This comment has been minimized.

Copy link
Author

commented Aug 4, 2014

Many convincing explanations exist

I can't find any: https://www.google.com/#q=scala+why+are+default+arguments+bad

I don't think it's WartRemover's job to repeat them.

I don't think it's too much to ask for a linter enforcing a rule to at least link to a convincing explanation.

@puffnfresh

This comment has been minimized.

Copy link
Collaborator

commented Aug 4, 2014

I think your example is very convincing of why default arguments are awful. If you do not agree, then you're free to use WartRemover and not enable the rule.

wartremoverErrors := Warts.allBut(Wart.DefaultArguments)
@RichardBradley

This comment has been minimized.

Copy link
Author

commented Aug 4, 2014

Have I wandered into the middle of a flamewar? That wasn't my intention at all.

I'm new to Scala. I was looking for style guidance, so I looked around for Scala static analysis / linter tools, and I've found wartremover.

wartremover seems to ban default arguments, a language feature that was added not that long ago, presumably deliberately, and presumably by people who understand Scala fairly well. The feature is in widespread use in Scala code, such as Spray (linked above).

If wartremover takes the stance that a language feature should be avoided entirely, then it is arguing against the designers of the language who chose to include that feature. I would expect that argument to be strongly put and I would be interested to hear it. When I said above that I can't find any convincing explanations, I wasn't being sarcastic.

I think your example is very convincing of why default arguments are awful.

I think a TCP connection method is a good example of where default arguments work well. There are loads of possible choices to be made, but most have sensible defaults.

I can see that the "connect" method is complex, but I think that's because of the complexity of TCP, rather than due to the use of default arguments.

How could Spray have designed their API better? What's the alternative to default arguments for spray.can.Http.Connect? A builder pattern? That would be quite a lot of boilerplate.

If you do not agree, then you're free to use WartRemover and not enable the rule.

What would be the point of a linter that only enforces guidelines that the author already agrees with? I already write all my code in a style I agree with, and I don't need a static analysis tool for that.

@vendethiel

This comment has been minimized.

Copy link

commented Aug 4, 2014

wartremover seems to ban default arguments, a language feature that was added not that long ago, presumably deliberately, and presumably by people who understand Scala fairly well. The feature is in widespread use in Scala code, such as Spray (linked above).

There's actually two different "schools" of scala, it seems. The "functional" one, that goes for purity and immutability, using scalaz and other typelevel projects, and wartremover is directed at them.
(the other school would be the java-y school, I think – sorry if that sounds judgemental – and maybe Spray is more of the second "school")

@aloiscochard

This comment has been minimized.

Copy link

commented Aug 4, 2014

@RichardBradley Hi Richard!

I used to believe in default arguments, but I have lost that belief over time :)

So I thought, it might be interesting to explain you why... my arguments are basically the same as the one @puffnfresh put forward and was referring to. I'll go in a bit more detail.

First thing, in my opinion most arguments against method overloading apply equally to default parameters... here is why!

Let's take that function with default parameter:
def foo(x: Int, y: String = "bar"): Int = x + 2

To me it's basically equivalent to:

def foo(x: Int) = foo(x, "bar")
def foo(x: Int, y: String)

Which mean that if I have now a function like this:

def bar(f: Int => Int) = f

I can not do bar(foo _), because the compiler will say that foo have a type (Int, String) => Int when he actually want a Int => Int.

So... I would have to write bar(foo(_: Int)) which is rather verbose if you use this style a lot (and here there is just one argument... think about a complex signature!).

The thing is that if you don't use high-order function a lot, you might not see this as a problem, but in the context of libraries designed with a strong emphasis on FP, it can make a huge difference!

WartRemover was created by "typelevel enthusiasts", so most of the rules where defined by people designing libraries with a strong emphasis on FP.

I heard that typesafe is working on a similar tools (which might include less "fundamentalist" rules)...

Anyway, I hope my explanation help you understanding why this rule was added in wart?

@mxswd

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2014

Hello @RichardBradley

Sorry for the lack of explanation in the README.

I have found while working on Scala projects that refactoring code with default arguments often leads to unexpected and dangerous bugs. Since there is such a simple and powerful alternative to using default arguments (a function) and default arguments can cause so much damage when refactored incorrectly, I think default arguments are a detrimental (and potentially malicious) addition to Scala.

Hope that makes sense. Thanks.

@RichardBradley

This comment has been minimized.

Copy link
Author

commented Aug 5, 2014

Thanks, that explains some of the motivation behind this rule.

Since there is such a simple and powerful alternative to using default arguments (a function)

I don't understand the proposed alternatives, sorry.

As a worked example, how could Spray rewrite their TCP connection methods without the use of default parameters (see "Connect" in spray.can.Http)?

@aloiscochard

This comment has been minimized.

Copy link

commented Aug 5, 2014

@RichardBradley not sure for spray specifically but if we look back at my example you could do:

def foo(x: Int)
def fooWithString(x: Int, y: String)

Basically you can find different name for every method shape, this approach is actually used in most haskell libraries and it work quite well.

@RichardBradley

This comment has been minimized.

Copy link
Author

commented Aug 5, 2014

not sure for spray specifically ... basically you can find different name for every method shape

The TCP connection method has 5 optional arguments, so that would be 32 different method names: "TCP.ConnectWithSslEncryptionAndSocketOptions".

This rule only seems to work for example code named "foo" ;-)

@mxswd

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2014

Why not:

  • connectHTTP(host: String)
  • connectHTTPPort(host: String, port: Int)
  • connectHTTPS(host: String) where port = 443, sslEncryption = true
  • the normal connect with all the arguments.

They seem like the 3 common use cases to me.

@aloiscochard

This comment has been minimized.

Copy link

commented Aug 5, 2014

@RichardBradley no, imo it's a bad design of spray then.

Parameters should be composed in data-structure that you can get populate with default values using certain functions.

That way you don't end in a case like the one you describe.
Once again, haskell libraries are of great inspiration to see how it work in practice....

Let me show you a concrete example:

case class FooParams(a: Int, b: Int, c: Int, d: Int)
def defaultFooParams: FooParams = ???
def foo(params: FooParams)

You can then do:

foo(defaultFooParams.copy(c = 4))
@vendethiel

This comment has been minimized.

Copy link

commented Aug 5, 2014

foo(defaultFooParams.copy(c = 4))

Btw, that was what I suggested here

an (immutable) instance you'd use as a "base", instead of a functions with 10 arguments.

Really, it's much better than a function with arguments (how do you decide for the ordering of the arguments ? Or do you just call it with named arguments ?)

@tpolecat

This comment has been minimized.

Copy link
Collaborator

commented Aug 5, 2014

... right, but then you have the awful copy method to contend with, then you add lenses, then you add phantom types to ensure that options haven't been set more than once, etc., etc., and I'm not convinced that the complexity is warranted, given the lack thus far of any convincing reason not to use default args.

So I guess I'm not really seeing the kind of slam-dunk evidence of madness that you find with things like overloading. If you have a method with a single default arg you could reasonably suggest splitting it into two methods (as you might do with a single option or boolean arg) but I'm not convinced that default args in general are bad. I would be happy to be convinced, but so far I'm not.

Maybe just say "some people find default args distasteful" and leave it at that.

@RichardBradley

This comment has been minimized.

Copy link
Author

commented Aug 5, 2014

@Nami-Doc, you do know that "copy" uses default args, right?
You are proposing changing:
TCP.Connect("example.com", socketOptions = xyz)
into
TCP.Connect(TCPConnectParams.default.copy("example.com", socketOptions = xyz))
?

How is that better?

@etorreborre

This comment has been minimized.

Copy link

commented Aug 5, 2014

Ok, I'm going to add my 2 cents to the discussion. First of all I have to say that I like overloading and default arguments. Just because I find it expressive. So if I have a call to a method like this:

val result = property.check

and I want to slightly alter the way check is performed, I like to be able to write:

val result = property.check(minTestsOk = 1000, minSize = 30)

I prefer this to

val result = property.checkWithMinTestsAndMinSize(1000, 30)

Now I understand why WartRemover has a rule against default arguments. They can be dangerous in production code. Let's say that I have a low-level operation reading data and copying it to a repository possibly compressed (this is actually 2 real-life examples modified and merged into one):

def ingest(data: Path, compressor: Option[Compressor] = None)

Here we choose None as a default value because, say, it's more convenient for tests. But for production data has to be compressed.

Now a real problem can happen if your call to ingest is deeply buried into several calls:

def parseCommandLineAndExecute(command: String): Result = {
  val mustIngest: Boolean = parseIngest(command)
  if (mustIngest) {
    val pah: Path = parsePath(command)
    val compressor: Option[Compressor] = parseCompressor(command)
    executeIngestion(path, compressor)
  }
}

def executeIngestion(path: Path, compressor: Option[Compressor]): Result = {
   prepareIngestion
   doIngestion(path, compressor)
   endIngestion
}

def doIngestion(path: Path, compressor: Option[Compressor]) = {
   log("I'm going to do it")
   ingest(path) // <------ BUG!!!!!
   log("I've done it")
}

In this situation the compiler won't tell you that you forgot to pass an argument value and will happily take the default one. But in production situations configuration values can have a tremendous impact on the whole system! So you will get a broken system and a hard to debug one because you have to manually trace all the calls to understand where and how the value to pass was left aside. And when you refactor your code, which @maxpow4h alluded to, you can introduce those bugs without realising it because the code might compile, the tests might all pass, yet the production system will be broken.

So my own conclusion is that default arguments (and overloading) still have some value (for non-critical DSLs) but you need to be very careful where you use them. For some modules you definitely want WartRemover to enforce the rule just to make sure that no one makes this kind of mistake.

@OlegYch

This comment has been minimized.

Copy link

commented Aug 5, 2014

@etorreborre i'm not sure if this refactoring problem is inherently a problem caused by default arguments, you might just as well have two functions ingest and ingestWithCompressor similarly confused

@etorreborre

This comment has been minimized.

Copy link

commented Aug 5, 2014

@OlegYch I'm not talking about overloading here, just about default arguments. Let's modify my example a little bit:

def parseCommandLineAndExecute(command: String): Result = {
  val mustIngest: Boolean = parseIngest(command)
  if (mustIngest) {
    val pah: Path = parsePath(command)
    val compressor: Option[Compressor] = parseCompressor(command)
    executeIngestion(path, compressor)
  }
}

def executeIngestion(path: Path, compressor: Option[Compressor] = None): Result = {
   prepareIngestion
   doIngestion(path, compressor)
   endIngestion
}

def doIngestion(path: Path, compressor: Option[Compressor] = None) = {
   log("I'm going to do it")
   ingest(path) // <------ BUG!!!!!
   log("I've done it")
}

In this example there are default arguments at each level in the call stack. If you add one more intermediate level and forget to pass the compressor value then None gets passed without any warning. So in this case avoiding default values forces you to always pass things explicitly down the stack.

In a recent presentation Yaron Minsky from (Jane Street](https://blogs.janestreet.com) said he would never, ever use default values in a production system. The risk of taking it instead of the "production" value is too high.

I was saying in my first comment that I liked default values. That's because they allow me to make very slight changes and alter some behaviour. Alas this is also the exact same reason for their weakness when writing robust systems.

@markhibberd

This comment has been minimized.

Copy link

commented Aug 6, 2014

I am with @maxpow4h, default arguments are a developer only convenience that lead to bugs more often than not. They are easily as insidious as overloaded methods or implicit configurations (and in some scala libraries more so - see point 1 below).

The use of default arguments in scala that I see regularly mis-used can be divided into four cases:

  1. Allocation of resources (there are even examples of this in scalaz) - which is utterly wrong. Anything that is allocated by a default argument has no reasonable lifecycle and is unlikely (or impossible) to be closed properly.
  2. Default configurations - these are a developer convenience that lead to operational bugs. There is no such thing as a "safe" default, where it could mean forgetting to set something in production leads to an incorrect value rather than an error (this is closely related to what Minsky says as mentioned by Eric above).
  3. Common arguments through delegating methods - these are representative of what @maxpow4h originally stated. That if you have multiple methods with optional arguments, it is extremely easy for incorrect code to compile by forgetting to delegate one of the arguments.
  4. Faux overloading - it is cool to hate on overloading so I will avoid it by using named arguments with defaults, ending up with the exact same situation. Code that is subtly wrong (such as forgetting to pass argument) still compiles. This is not an acceptable situation.

But the most troublesome part of this thread, is that almost all of the discussion is about what developers find "convenient" and aesthetically pleasing, when we should be asking how a language feature adds or removes from our ability to build robust, correct programs - and, as quickly as possible. When held in this light, default arguments do not hold up. They are a mere syntactic convenience - that does not help us with this goal. This might be ok, if they didn't come with risk or issues, but even the gentler arguments in this thread should be enough to highlight their use in a linting tool - especially given their inherent lack of motivation to begin with.

But yeh. Everyone gets to live in their own teams codebases. I just prefer mine without these undue risks.

@OlegYch

This comment has been minimized.

Copy link

commented Aug 6, 2014

thanks for the clarification guys
fwiw, i agree with most of what have been said here and personally refrain from using default arguments but still find them useful on rare occasion
i've seen them being used inappropriately and had hard time explaining why, and this discussion will help a lot in the future
as for the issue of explaining warts in general i'd be happy to see something like "rationale" section for each wart as seen in checkstyle or "why not" in hlint

@RichardBradley

This comment has been minimized.

Copy link
Author

commented Aug 6, 2014

OK, so there are some arguments here about how bugs can arise from default args + refactoring, but I don't see what the alternative is in many cases.

Take the Spray TCP.Connect example above: what is the alternative to default arguments here? The only one I can see is a builder pattern with 5+ property addition methods. This would require a great deal of boilerplate to satisfy this wart.

i.e. for the client:

TCP.Connect("example.com", socketOptions = xyz)

becomes:

TCP.Connect(
  TCPConnectParams("example.com")
    .withSocketOptions(xyz))

and the server code changes from:

def Connect(host: String, port: Int = 80, sslEncryption: Boolean = false, localAddress: Option[InetSocketAddress] = None, options: immutable.Traversable[Inet.SocketOption] = Nil, settings: Option[ClientConnectionSettings] = None): Unit = ...

to

case class TCPConnectParams(port: Int = 80, sslEncryption: Boolean = false, localAddress: Option[InetSocketAddress] = None, options: immutable.Traversable[Inet.SocketOption] = Nil, settings: Option[ClientConnectionSettings] = None) {

  def withSocketOptions(options: Traversable[Inet.SocketOption]) = copy(options = options)

  def withLocalAddress(localAddress: Option[InetSocketAddress]) = copy(localAddress = localAddress)

  ... many more "with" methods
}

... which is very long winded, and we haven't event avoided the use of default args (they're still in the constructor and in the "copy").

If we think the Spray TCP API is "badly designed", what would be a better alternative?

There is no such thing as a "safe" default

This is simply not true. For example, the default SocketOptions provided by the O/S are safe for most normal TCP connections. Only if you are doing something unusual (like a very high throughput server-to-server connection which would be affected by TIME_WAIT port exhaustion) do you need to override this default.

@etorreborre

This comment has been minimized.

Copy link

commented Aug 6, 2014

I think that you can do the following:

Have a parameter class where all the values need to be specified

case class TCPConnectParams(port: Int, sslEncryption: Boolean, localAddress: Option[InetSocketAddress], options: immutable.Traversable[Inet.SocketOption], settings: Option[ClientConnectionSettings])

And factory methods in the companion object for the most common scenarios:

object TCPConnectParams {
  def test = TCPConnectParams(...)
  def production = TCPConnectParams(...)
  def secure = TCPConnectParams(...)
  // possibly a method with a parameter
  def local(address: InetSocketAddress) = TCPConnectParams(localAddress = Some(address)) 
}
@lihaoyi

This comment has been minimized.

Copy link

commented Aug 6, 2014

There is no such thing as a "safe" default

Disagree =) The whole reason everyone in my office is using Windows or OSX rather than FreeBSD as their work machines is precisely because of safe defaults.

As @RichardBradley said, I'd like to see a "better" alternative before accepting that Spray's API is badly designed =P The options presented here are:

  • Fluent Builders
  • Passing 10 different None arguments every time
  • Config object which uses the .copy method
  • Pass every argument every time

The first two are basically isomorphic with defaults, except more verbose and annoying. They have the same "oops i forgot one" property, and the same "can't be auto lifted into functions but can if you manually use _" property. The third one actually uses defaults in .copy!

The last one is different, but arguably more unsafe, since you're basically asking the end-user to ad-hoc parameter-values that you already know with 99.9% certainty what they should be, and they have much less than 99.9% chance of getting it right for e.g. configuring their TCP connection. It also has the same "can't be auto-lifted into meaningful functions" property unless you regularly pass 10-tuples or functions with 10 arguments around your APIs.

The last (unstated) alternative that I can think of is to pass a Map[String, Any] around, which is the norm for people working in languages without defaults (javascript, ruby-until-recently, java to some extent for larger configs) which is obviously much worse.

Saying "Spray's API is bad" and then providing toy examples with 1-2 arguments isn't very convincing. Saying "here's 3 functions that cover what I think are the 3 most common configurations" is basically saying "the world should be simpler", which is true but doesn't help me write correct, robust software in the messy real-world we live in.

@markhibberd

This comment has been minimized.

Copy link

commented Aug 6, 2014

There is no such thing as a "safe" default

Note this is the second time someone has decided to paste half of a sentence to make a point. Please don't, that is not what was said.

Disagree =) The whole reason everyone in my office is using Windows or OSX rather than FreeBSD as their work machines is precisely because of safe defaults.

Good joke, even funnier because I run FreeBSD on my desktop. A lot easier than either of those other two, I can assure you.

Saying "Spray's API is bad"

Nobody said "Spray's API is bad" maybe don't use quotes. What is it with the internet and mis-representation.

Saying "Spray's API is bad" and then providing toy examples with 1-2 arguments isn't very convincing. Saying "here's 3 functions that cover what I think are the 3 most common configurations" is basically saying "the world should be simpler", which is true but doesn't help me write correct, robust software in the messy real-world we live in.

The discussion and point I was merely trying to make is that everyone can write code how they want, but there is value in a lint tool warning about a language feature that provides syntax value only, especially if it has risks. Reguardless, I realise now I should not of extended the conversation. Apologies to @puffnfresh and carry on. And apologies to @lihaoyi for the snark above, but it is really not ok to make things up, and you just happened to be the last person who did it.

@lihaoyi

This comment has been minimized.

Copy link

commented Aug 6, 2014

@aloiscochard totally said spray's API is bad! Wasn't referring to you specifically, sorry if it wasn't clear, but I didn't make it up =P Maybe I shouldn't have used quotes or C&Ped what I was referring to directly...

Good joke, even funnier because I run FreeBSD on my desktop. A lot easier than either of those other two, I can assure you.

We probably can just agree to disagree here =)

The discussion and point I was merely trying to make is that everyone can write code how they want, but there is value in a lint tool warning about a language feature that provides syntax value only, especially if it has risks.

I guess what @RichardBradley and I are wondering is: what is the "better" way to do things in this case? In many of the other rules, there is a clear path forward: vars can often be replaced by a fold or monad of some sort, Any and is/asInstanceOf can usually be replaced by better use of generics/typeclasses, and so on. So I'm curious what pattern could suite this use case that would remove the risks of default arguments, because if I'm missing out on something I'd want to know =)

(The reason I'm here is that somebody came across the readme at work and asked me "why are default arguments insane", and I had no answer)

@markhibberd

This comment has been minimized.

Copy link

commented Aug 6, 2014

I guess what @RichardBradley and I are wondering is: what is the "better" way to do things in this case? In many of the other rules, there is a clear path forward: vars can often be replaced by a fold or monad of some sort, Any and is/asInstanceOf can usually be replaced by better use of generics/typeclasses, and so on. So I'm curious what pattern could suite this use case that would remove the risks of default arguments, because if I'm missing out on something I'd want to know =)

I agree there isn't a clear answer it might be easier to break it down into the 4 cases I mention above.

  1. Resouce arg - the answer is easy - don't do defaults, force consumer to pass argument and handle resource allocation/deallocation.
  2. Config - Definitely no easy or prescriptive answer. I can only say that I do something very close to what @etorreborre suggested above, a companion with different (non-overloaded) functions the offer completely configured objects. One subtly that I think has been overlooked in the discussion above is that the concern isn't programmer X forgetting to call function Y with argument Z (can always happen obviously), as much as it is, programmer X does the right thing, but programmer Q changes function Y and in an incompatible way and programmer X's code still compiles. If this can be avoided it should be.
  3. Delegating defaults - Just don't do this. It is duplication (in the definition of default and even more susceptible to the issue above). Just pass arguments. Bundle them up in a reader if it gets too arduous.
  4. Faux overloading - Just don't do this. It is as bad as overloading for all the same reasons. Just name the different combinations or use a composable abstraction like a lens over a data type of your args.

I will also say that my experience says (3) happens a lot more than (2) and is way easier to avoid - so spending too much time fussing about what is largely the aesthetic of (2) seems not that important to me.

@aloiscochard

This comment has been minimized.

Copy link

commented Aug 6, 2014

@lihaoyi I apologize if I have offended you by saying that the usage of default arguments in Spray is a bad design (I'm not saying the whole API is bad... I'm actually using it quite a lot, and even collaborate with the authors trying to make it more safe: https://twitter.com/aloiscochard/status/495242711478579200)

Thanks to @markhibberd for giving a nice summary of the different alternative approach, and I hope it will more convincing than my toy function. I'm sorry I was not in the mood of write a whole essay on the subject... but on the light of the contribution of others (like @etorreborre for example) I don't see what is missing now to sound convincing.

@lihaoyi

This comment has been minimized.

Copy link

commented Aug 6, 2014

No offense was taken =) I don't actually work on spray and while using it have whined as much as anybody.

I'm probably just going to go with "it's fine to use them for configs" for now.

@puffnfresh

This comment has been minimized.

Copy link
Collaborator

commented Aug 6, 2014

@markhibberd 👍 to everything. Thank you very much.

def Connect(host: String, port: Int, sslEncryption: Boolean, localAddress: Option[InetSocketAddress], options: immutable.Traversable[Inet.SocketOption], settings: Option[ClientConnectionSettings]): Unit

def defaultPort = 80
def defaultSslEncryption = false
def defaultLocalAddress = None
def defaultOptions = Nil
def defaultSettings = None

I'd also remove the Unit (use a type for the effect) and the Boolean parameter (cause booleans suck) but those are different problems.

Now, if the problem is "people have to input lots of parameters" then that's also a different problem with a few different tools.

@RichardBradley

This comment has been minimized.

Copy link
Author

commented Apr 12, 2015

@puffnfresh -- you have closed this issue, but the README still has no explanation about what developers should use instead of default arguments: https://github.com/puffnfresh/wartremover/blob/0d20131326db4482d1babbee04c56427f2968732/README.md#defaultarguments

If this is to be pushed as a good coding practice in Scala, I think it needs some documentation to justify it, with explanation, examples and tips on how to convert to alternate styles.

Perhaps there is no consensus on this topic, but I think that if wartremover is taking the position that default arguments are bad it ought to explain what you think should be done instead, ideally covering each of @markhibberd's 4 cases above (if they should be handled differently by developers).

@dwijnand

This comment has been minimized.

Copy link
Collaborator

commented Apr 12, 2015

Although more an alternative to method overloading, but default arguments are sometimes used for method overload, the "Magnet Pattern" could be of interest as well:

@ssanj

This comment has been minimized.

Copy link
Contributor

commented May 19, 2019

This is such an interesting thread. I've tried to summarise the pros/cons of using default arguments as listed in this issue here if anyone's interested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.