How do you guys write `error(message: String)` messages? 1. `System.getenv("GITHUB_SHA") ?: error("...

# codingconventions

[Nick Halase]

How do you guys write error(message: String) messages?
1. System.getenv("GITHUB_SHA") ?: error("GITHUB_SHA is missing")
2. System.getenv("GITHUB_SHA") ?: error("GITHUB_SHA must exist")

[dumptruckman]

I’d probably throw something in to indicate it’s missing from the environment

[Nick Halase]

error("message") throws an IllegalStateException. I'm curious about the wording of the message. Would you state the variable is missing, or that it should exist?

[dumptruckman]

I mean, I get what you’re asking but also would recommend that you indicate where it is missing from or should exist in. I don’t really have a recommendation for which wording otherwise.

[ephemient]

rather than ?: error(message), I'd use

checkNotNull(System.getEnv("GITHUB_SHA")) { message }

regardless of what the message is

[Joffrey]

@ephemient I guess the value of the env variables are useful, so in that case using elvis would probably read much better

[efemoney]

I dont think it reads better in this case even if you were to assign it to a variable. One thing to consider is that they throw either IAE or ISE and you have to pick the exception that is correct for your use case

[Klitos Kyriacou]

Some general advice on error messages: https://wix-ux.com/when-life-gives-you-lemons-write-better-error-messages-46c5223e1a2f (there are also other sites with similar advice). However, keep in mind that exception messages are not necessarily the same as what would be shown to the end user.

[Joffrey]

@efemoney for some reason I thought checkNotNull(x) was a shortcut check(x != null), but it does in fact return the non-null value, my bad. Both checkNotNull and error throw ISE, so that is not a distinguishing factor (although I agree in case we need IAE, require variants would be nessary).

Still, in the case where ISE is needed, I do prefer this:

val sha = System.getenv("GITHUB_SHA") ?: error("GITHUB_SHA is missing")

Over this:

val sha = checkNotNull(System.getenv("GITHUB_SHA")) { "GITHUB_SHA is missing" }

Because we can more clearly see the interesting expression in the happy path, as the error case is handled separately, while using checkNotNull kinda mixes both. Similarly, a git diff showing the addition or removal of the check would be more readable (IMO) using the elvis because the whole error case is contiguous in a single piece of code.

[Joffrey]

In any case, this discussion is orthogonal to the original question, sorry @Nick Halase. Both messages look OK to me, but I would suggest giving more information in the message about why the env variable was expected to be there if it's possible. This would help debug this invalid state.

[Nick Halase]

@Joffrey thank you!

[ephemient]

@Joffrey there's no analogous function to error() for IAE, but there is requireNotNull(). that is one reason I prefer checkNotNull()