14

In my team, we work closely with a few software architects. They approve all design decisions of our projects, do some code reviews etc.

Our projects consist mainly of backend functionality implemented in PHP using the Symfony 2 framework. So syntactically, the code, naming conventions and project structure look almost identical to what Java would look like (Symfony 2 encourages such structure). I'm mentioning this because Java-specific conventions also apply in our case (if possible).

Recently, they suggested something that I find very strange: all methods should have conjunctions in their name e.g. getEntityOrNull, setValueOrException etc.

Such a naming convention feels very wrong to me, but I can't come up with any concrete arguments or online articles/pages that specifically challenge this.

The only things I came up with is:

  • such information should be present in the method's annotations, like @return or @throws
  • the use of conjunctions ("and", "or" etc.) in method names usually suggest that the Single Responsibility Principle is not properly respected

What are some other concrete arguments against this naming convention?

Karl Bielefeldt
  • 146,727
  • 38
  • 279
  • 479
Radu Murzea
  • 1,810
  • 2
  • 18
  • 24
  • generally, [you can't](http://en.wikipedia.org/wiki/Compare-and-swap "there is a whole set of programming idioms involving conjunctions, for example compare-and-swap (CAS)") – gnat Sep 08 '14 at 08:07
  • 7
    `the use of conjunctions ("and", "or" etc.) in method names usually suggest that the Single Responsibility Principle is not properly respected` This is not the case for the examples you listed, where the conjunction is used to clarify the mechanism used to handle failures, not to indicate it may do one thing or another. Even the most narrowly-defined function may have legitimate failure conditions, e.g. popping an empty stack. – Doval Sep 08 '14 at 15:04
  • 4
    Consider: (1) using "optional" rather than "or null". "GetOptionalEntity" sounds better to my ear than "GetEntityOrNull". (2) the .NET base class library uses the convention that if you have two methods that differ only in what they throw, name the one that cannot throw "TryBlah". For example, `Int32.TryParse` and `Int32.Parse` -- both parse a string into an integer, but the former returns a Boolean indicating success and the latter throws on failure. – Eric Lippert Sep 08 '14 at 17:04
  • I wouldn't use a suffix for the throwing variant, but I would use one for the null returning variant. Could be `Try...`, `...OrNull`, `...OrDefault`. @EricLippert That's not the only convention in .net. Consider `Single` vs. `SingleOrDefault`, which is very close to the `OrNull` the OP suggested. – CodesInChaos Sep 09 '14 at 14:26

4 Answers4

14

They are probably doing so because of naming conflicts. I assume that you cannot have two methods named getEntity, one which possibly throws an exception and one which returns null. Therefore, you have to name them accordingly.

I, for one, dislike the practice of having many different ways of calling the same method unless it is performing some alteration that only that particular class can perform.

In other words, if getValueOrNull is simply calling getValueOrException, capturing the exception, and in that case returning null, this can be performed by the caller. It clutters the class with methods that don't really contribute anything useful. Rather I would prefer the getValue, and know that it throws the exception. Better still, I would prefer to know that all get methods potentially throw exceptions, and none of them return null instead so that behavior is uniform throughout my project, or inversely, all potentially return null, and know that the caller would have to throw an exception if that were desired.

However, it is also true that you're not in charge of that. My advice is to bring it up, but don't sweat the petty things. Ultimately, the responsibility of such naming conventions falls on their shoulders, regardless of whether or not they take your advice, and because it is their asses on the line, I believe it is their prerogative to say no, in my humble opinion.

Neil
  • 22,670
  • 45
  • 76
  • If you had to choose only one, it's better to return `null` (or better yet, an Optional/Maybe type) since throwing is relatively expensive. Writing a helper that checks if a value is invalid and throws doesn't add much overhead. However when you want the no-throw version, swallowing the exception won't give you back the performance hit you took throwing it. – Doval Sep 08 '14 at 18:05
  • 1
    @Doval Good point, and perhaps more importantly, exceptions should be *exceptional*. If you're often throwing exceptions, you're not using them right. Only problem with returning `null` is that `null` may actually be a valid return in some instances and so you would have to deviate from the norm and throw an exception in such instances. – Neil Sep 08 '14 at 20:11
4

Although the use of conjunctions often indicates a violation of the SRP, in this case it is just indicating the return value of a poor man's algebraic data type that can be one of two values, either null or a "success" value.

They are using a kind of Hungarian Notation to compensate for weaknesses in the type system, namely a lack of non-nullable types. In other words, there is no way to specify in your @return annotation that the function will never return null, and conversely, there's no way to specify in your @return annotation that the function may possibly return null.

Also, I believe that @throws annotations are not mandatory in php, so the absence of the annotation doesn't indicate the absence of an exception, although that's better solved by making the annotation mandatory in your style guide.

Given those limitations of the language you're using, it's not an entirely unreasonable style.

Karl Bielefeldt
  • 146,727
  • 38
  • 279
  • 479
2

In my code I sometime create method pairs with names like getEntity() and getEntityOrNull(). The name makes the expected behavior clear. If getEntity() finds no entity then an exception is thrown. getEntityOrNull() will return a null.

By doing this, the calling code becomes a bit clearer. If the calling code has to have an entity then getEntity will do the trick. If entity is some sort of optional thingee then getEntityOrNull is the method of choice.

The same thing could be accomplished with a single method but that then shifts some of the burden to the calling program. You always need to test for a null or you need a try/catch block. Either way it's extra code that needs to be duplicated each time the method is called.

If your architects are not using these sorts of method pairs then yes, I would question the need for the suffix.

Never really saw a setValueOrException method. Can't think of a good common use case for that.

You might consider asking the architects why. The ones I know are always glad to explain their decisions. Often in great (and sometimes excruciating) detail.

Cerad
  • 598
  • 4
  • 9
1

Your two arguments are sound. But if they're the ones in charge of deciding the naming convention, try not to feel too bad about it being something you don't agree with.

While you're at it, you should convince them not to use the "get" and "set" portions unless those methods actually do directly set or get a member variable.

Eben
  • 141
  • 3