39

I am in a predicament. I am working in a repository that makes a lot of database changes within its functions.

The function I am dealing with returns responseids (but from a transformation, not from any database action). However, as a side effect, it adds an object containing those responseIds to the database.

So should I name it:

  • getResponseIds: This highlights the return values. It is a very functional way of thinking, but obviously if I have function postToDB it makes no sense to use getStatusOfPost
  • addResponseIdToDB: This highlights the side effect, although I truly think that many of my functions just operate on the database (and tend not to return anything)
  • getAndAddResponseIdsToDB: Very informative, but very long.

What are the pros and cons on the suggestions above? Or can you make a better suggestion yourself?

  • 3
    How about persistResponseIds? – Kain0_0 Jan 24 at 4:30
  • 5
    You might be suprised how far the functional crowd has gotten with the equivalent of getStatusOfPost :) – Luaan Jan 24 at 12:45
  • getAndSaveResponseIds – gnasher729 Jan 24 at 15:17
  • responseIds = createResponseIds() seems clear and concise. You get() something that didn't exist before, so create is the appropriate verb. The newly created stuff is what you'd expect a create()function to return. Or maybe I'm missing something obvious? – Bass Jan 25 at 11:52
  • 4
    @fattie I agree that code should be self-commenting. Comments should not tell you what the code does. Comments should tell you why the code exists. If refactoring the code means refactoring it's comments you have bad comments. I agree that names can be long full sentences. A good name should keep looking inside a function from surprising you. It shouldn't tell you how to implement it. It should tell you what the client is expecting. Do that and you have a good worthwhile abstraction. – candied_orange Jan 25 at 14:46
56

A function name that contains and is at the wrong level of abstraction.

I lean towards addResponseIdToDB() because otherwise the ‘side effect’ is a complete surprise. However:

responseIds = addResponseIdToDB();

doesn’t leave anyone surprised.

The command query responsibility segregation principle argues that this should not be the only way to get the responseId object. There should also be a query that does not change the DB that can get this object.

Contrary to Bertrand Meyer, I don't believe this means the add has to return void. Just means an equivalent pure query should exist and be easy to find so the DB doesn't get needlessly abused by use of state changing queries.

Given that getResponseIds() should exist and not talk to the database, the best name for a method that does both is actually addToDB(getResponseId()). But that's just if you want to get all functional composition about it.

  • 37
    Adding to what has been said, but adding a word that hasn't been used, I would say that "and" is appropriate when a function does something that is atomic. If there is a benefit to be gained from two things happening atomically/together/at once, then being explicit with what those two things are in the function name is good. So I agree with your first statement in general, but disagree without any qualifications. – J. Dimeo Jan 24 at 1:52
  • 11
    "...doesn’t leave anyone surprised.". I disagree, I'm left surprised that a boolean variable is called responseIds and I then have to do a mental shift to realise that it adds and gets all in one. But I disagree with @tintintong that getAndAddResponseIdsToDB is too long; it's around half the length of a great many of my function names. If a function does two things, then say so in its name. – David Arno Jan 24 at 7:44
  • 7
    @Luaan OK, what are you going to rename getAndIncrement to then? otherIncrement? – Malcolm Jan 24 at 13:12
  • 7
    @Luaan So which value does Increment return, the value pre-increment or post-increment? It's not clear which it would be without diving into docs. I think incrementAndGet and getAndIncrement are far better method names, which I believe makes a good case for "AND" in method names, at least in the specific case of methods which have side effects which could impact the return value.. – xtratic Jan 24 at 13:49
  • 6
    @Luaan When you say "Increment returns the incremented value" you mean the pre-increment value? Gotcha, misunderstood you at first.. which is actually a great example for why getAndIncrement and incrementAndGet are better names than just increment, even when considered individually – xtratic Jan 24 at 15:03
13

As others have mentioned, using and in a function name, automatically implies that a function is doing at least two things, which usually indicates that it's doing too much or is doing something at the wrong place.

Using the and clause in a function name however, in my experience, can make sense when there are some steps in a certain flow that need to be performed in sequence or when the whole computation becomes meaningful.

In numerical computing this can make a lot of sense:

normalizeAndInvert(Matrix m)

I mean, who knows, right? If you need to compute some... say, lighting trajectories in computer graphics and at a certain stage you need to normalize and invert a certain matrix, and you find yourself constantly writing:

m = normalize(m), followed by invert(m), just as a simple example, introducing the abstraction of normalizing and inverting, can be good from a readability perspective.

Speaking semantically, stuff like divideAndConquer() etc, is probably discouraged to be explicitly written out, but, well, the And there is basically necessary.

  • Functions regularly do more than one thing, but those should be little things add up to one big thing that's meaningful at a higher level. normalizeAndInvert could be computeLighting and divideAndConquer could be applyMachiavelli – Robin Bennett Jan 24 at 14:12
  • Obviously :) Just stating that first creating a function with an "and" in the name might come as a natural abstraction based on a particular context. Refactoring via the "Rename method" should come immediately after ;) – Bruno Oliveira Jan 24 at 15:07
  • @BrunoOliveira Except sometimes the two steps are themselves a component of many things, rename isn't the right approach. And should be rare but never using it means you're repeating yourself. – Loren Pechtel Jan 26 at 19:56
9

I would say it's fine in general, but isn't acceptable in all cases.

For example, one could argue against and in a function name based on the single responsibility principle aka the S in SOLID - because the and could imply multiple responsibilities. If the and really does mean the function is doing two things, then you probably want to think carefully about what's happening.

An example of a library that utilizes and in function names can be found in Java's concurrency and here and is actually a very important part of what's going on, and closely matches what you describe in your post where the state is changed and state is returned. So clearly there are some people (and some use cases) where it's deemed acceptable.

  • "the and could imply multiple responsibilities". And in this case it does indeed indicate that. The function gets some response IDs from somewhere, writes them to a database and returns them. The need for that "and" should indeed at least raise the idea with the OP that two functions are needed: one to get the IDs and one to write them to the database. – David Arno Jan 24 at 8:55
  • 2
    While I agree that the and is a code smell, the basic behavior seems legit: It is generally not a bad idea to return some additional value(s) from a method that happen to be necessary (intermediate) results of carrying out its main function. If the main function of this method is to add the response IDs to the database, additionally returning the IDs it has added to the caller is just a neat, effortless usability feature of the method. – cmaster - reinstate monica Jan 24 at 9:32
  • 1
    I don't think this necessarily violates SRP. You will always need functions that do multiple things. The important part is to have these functions call another function for every thing they want to do instead of containing the required code directly. – Tim Pohlmann Jan 24 at 11:53
  • 1
    If the function does two things then the name should reflect it. – gnasher729 Jan 24 at 15:18
2

Your function does two things:

  1. Gets a set of IDs via a transform and returns them to the caller,
  2. Writes that set of IDs to a database.

Remember the single responsibility principle here: you should aim for a function to have one responsibility, not two. You have two responsibilities, so have two functions:

getResponseIds - Gets a set of IDs via a transform and returns them to the caller
addResponseIdToDB - Takes a set of IDs and writes them to a database.

And the whole issue of what to call a single function with multiple responsibilities goes away and the temptation to put and in the function names goes away too.

As an added bonus, because the same function is no longer responsible for two unrelated actions, getResponseIds could be moved out of your repo code (where it doesn't belong as it's not doing any DB-related activity), into a separate business-level class/module etc.

  • 4
    That's surely true, but you find yourself repeating a snippet using these two SRP methods many times, then you probably should write a trivial method doing this so you DRY, shouldn't you? – maaartinus Jan 24 at 10:47
  • 1
    @maaartinus, if you find yourself calling these two methods in many places, then you likely have a problem with a lack of cohesion in your code. Creating a "DRY" function then just masks this lack of cohesion. – David Arno Jan 24 at 11:24
1

Having a composite function name is acceptable, so which naming scheme you use depends on your context. There are purists who will tell you to break it up, but I will argue otherwise.

There are two reasons to try to avoid such things:

  • Purity - A function which does two things should be converted to two functions which do one thing each.
  • Lexical size - a bigUglyCompositeFunctionName gets hard to read.

The purity argument is typically the one that is focused on. As a vague general heuristic, breaking up functions is a good thing. It helps compartmentalize what is going on, making it easier to understand. You are less likely to do "clever" tricks with sharing of variables which lead to coupling between the two behaviors.

So the thing to ask yourself is "should I do it anyways?" I say breaking things up is a vague heuristic, so you really only need a decent argument to go against it.

One major reason is that its beneficial to think of the two operations as one. The end-all-ultimate example of this is found in atomic operations: compare_and_set. compare_and_set is a fundamental cornerstone of atomics (which are a way of doing multithreading without locks) which is successful enough that many are willing to think of it as the cornerstone. There's an "and" in that name. There's a very good reason for this. The entire point of this operation is that it does the comparison and the setting as one indivisible operation. If you broke it up into compare() and set(), you'd defeat the entire reason the function existed in the first place, because two compares() could occur back to back before the set()s.

We also see this in performance. My favorite example is fastInvSqrt. This is a famous algorithm from Quake which calculates 1/sqrt(x). We combine the "inverse" and "square root" operations because doing so gives us a massive performance boost. They do a Newton's approximation which does both operations in a single step (and happens to do it in integer math rather than floating point, which mattered in the era). If you were to do inverse(sqrt(x)), it would be much slower!

There are some times where the result is more clear. I've written several APIs involving threading where one has to be dreadfully careful about things like deadlocks. I didn't want my user to be aware of the internal implementation details (especially since I might change them), so I wrote the API with a few "and" functions which prevented the user from ever having to hold a lock for more than one function call. This means they never needed to know how I was handling the multithreaded synchronization under the hood. In fact, most of my users weren't even aware they were in a multithreaded environment!

So while the general rule is to break things up, there can always be a reason to couple them together. For example, can your user break your architecture by adding to a database multiple times without the "gets" inbetween? If each of the responses logged in the DB needs to have a unique identifier, you could get in trouble if the user got to do them willy nilly. Likewise, would you ever want to let a user "get" without logging the results in the DB? If the answer is "yes" then break them up so that the user can access the "get" function. However, if the security of your application is broken if the user can "get" without the result being logged in the DB, you really should keep the functions together.

Now as for the lexical issues, your function name should describe what the user needs to know about it. Let's start with the "get" part. Its pretty easy to remove the "get" from a function name, because all of your examples will show an assignment: int id = addResponseIdToDB()". In all places where the function is used, you will end up documenting the fact that the function returned a value.

Likewise "add" can be optional. We use the term "side effect" as an all encompassing term, but there's different shades of it. If the DB entries are merely a log, there may be no reason to highlight it. We never see functions like playMusicAndSendPersonalInformationToOurCorporateServers. It's just playMusic, and if you're lucky, the documentation may mention something about a socket connection over the internet. On the other hand, if users are expected to call this function for the purpose of adding things to the DB, then "add" is essential. Don't take it out.

Now, I've written a lot of reasons to do every possible combination of what you are asking. I've intentionally not answered the question because there's a choice to be made. It's not a rule to follow. You're crafting an API.

That being said, my instinct is that addResponseIdToDB is likely to be the best answer. In most cases the "get" portion is an obvious enough side effect that it doesn't earn the extra noise caused by typing it everywhere. However, there's a few places where I might consider it important:

  • If the "get" is expensive -- If you have to do a "get" that involves fetching something over the internet and then adding it to a local cache DB, by all means the "get" is important. It is the thing the user is trying to do.
  • If you need to make it clear that the user needs to pay attention to the value. If the user wants to use the variable, they'll realize the API returns it, and they'll use it. However, you want to watch out for the user that doesn't know they need it. For example, if you have to "close" this operation by ID later to free up memory, you may want to draw attention to the fact that you're doing something. In these cases, you might want to look at a verb other than "get." "get" often implies idempotent functions (calling it again doesn't do anything). If it returns resources, other verbs like "create" or "acquire" are nice. This particular failure mechanism is a major issue in C when doing exception handling. C lacks the catch/throw mechanism of C++, so it relies on return codes. Developers are famous for simply failing to check these return codes and getting into really bad situations like buffer overflows because of it.
  • Symmetry -- Sometimes you design an API to have a lexical symmetry to it. You set up the words so that they come in pairs, or other patterns, and craft the commands so that it is easy to visually identify whether the pattern has been followed or not. I'd say this is rare, but its not unheard of. There's a reason closing XML tags repeat the tag name (such as <foo> and </foo>).
0

What is the ultimate intent of this method? Why add these transformed ids to the DB, is it for the purpose of caching? I'll work under that assumption.

It sounds like the intent of the method is really just to get the transformed response ids (either doing the transform or getting them from a cache), and so there's your method name:

getTransformedResponseIds or less verbosely getResponseIds()

A method with this name could cache or maybe not, and that can be documented, but your method name shouldn't tie you to a specific implementation; What if you decide to stop using a DB and instead cache them elsewhere such as just temporarily in memory?

Side effects should be just that, side effects, they should probably be documented but they shouldn't really impact the core intent (or success) or name of the function. If getting the value from the cache fails (or if you configure to skip caching) that shouldn't be a critical issue, the method should just transparently re-calculate, cache (or not), and return the new value.

Sometimes using an "AND" is helpful to convey intent such as with the Java methods getAndIncrement and incrementAndGet so that's certainly not off the table.

-2

You say that the function returns something "from a transformation, not from any db action".

This suggests that the function behaves more like a constructor than a getter. However, constructors also shouldn't cause side-effects (thanks for the link, @MechMK1).

Factory methods, on the other hand, already presuppose some level of messiness. For example, one of the motivations for using factory methods is that a factory method:

Allows for more readable code in cases where multiple constructors exist, each for a different reason. - wikipedia

Likewise,

If you need to do some work with side effects, a factory method can be named in such a way that it's more clear what those side effects are. - ruakh

Consider making the function a factory method, using the term "logged" to alert programmers to the database storage:

  • Declaration: createLoggedResponseid(...) {...} // log object to db
  • Call: responseid = createLoggedResponseid(...);

Not the answer you're looking for? Browse other questions tagged or ask your own question.