22

So I have a webservice that has something like a getAccount where it would return an identifier to the account if it got it, else throw an exception. The client will always want to create an account if an exception is thrown with the same info the get is done with.

I am creating a convenience library for clients that will be handling all of the webservice calls inside so they don't need to know how to do the calls themselves.

What I am wondering is in this library if I were to create a getAccount(accountName) that will get the account if it exists, and if it does not then create it and return the info, is that a bad thing to do? Should I leave it to the client to handle the exceptions or simply name it something like getOrCreateAccount? Does it matter?

Is it bad practice to create something in a get operation?

Improve this question
  • 10
    At the very least, I would name it getOrCreateAccount or similar. – Telastyn May 22 '12 at 19:08
  • 4
    It seems valid in the context of lazy initialization. However, it depends if you have a need to use that pattern in your library. – Chris C May 22 '12 at 19:43
  • 1
    I like the verb acquire, like acquireAccount. It doesn't have an existing meaning in any major protocols that I have encountered, and it has an imperative ring to it that suits it well. "Do whatever you have to do to acquire one of these for me. Request it, build it, fake it, steal it, I don't care, just get me one or die trying." – Dan Ross Apr 17 '16 at 11:38
  • 2
    "The client will always want to create an account" - that seems highly unusual. If I can't get the account because the user mistyped their username, I certainly don't want to create an account with the mistyped name. – gnasher729 Oct 19 '16 at 8:08
  • According to the javabean spec oracle.com/technetwork/articles/javaee/spec-136004.html getSomething() is for getters, and setSomething() is for setters. Imo anything that does something more intellectual must be called something else, i.e. fetchSomething, obtainSomething, computeSomething, or doSomethingElse etc. – ccpizza Oct 29 '16 at 13:23
34

Yes, it matters. In my opinion, it's a generally a bad practice to create something in a procedure that is not documented as having the powers of creation. Either name the procedure getOrCreate... or have a separate create... procedure and then if you really want, have a getOrCreate... that first attempts get..., and if that fails, calls create... and then calls get....

The user of the library will probably not expect the get... procedure to create if the get operation fails. If they suddenly find out that their test calls to get... created a whole tonne of data, they will probably be rather surprised. And how do they clean it up? What if they write code thinking that they will get an error if get... fails and they want to handle that their way?

Improve this answer
  • Thanks, the create actually returns the id that the get would return too so I wouldn't have to do a get... create... get... just the first two. I will talk to the client about whether they will ever require the ability to simply call a get without wanting to create – Mike May 22 '12 at 19:25
  • 6
    @Mike: I still think it should have create in the name, just to be 100% clear about what's happening. – FrustratedWithFormsDesigner May 22 '12 at 19:26
  • Yes I agree, I was planning on making it something along the lines of getOrCreate because my original thought was just to do get but I realized that it is not immediately clear that it is also creating something in the background – Mike May 22 '12 at 19:31
  • 1
    This is super late but getOrCreate has precedence in a popular web framework: docs.djangoproject.com/en/1.10/ref/models/querysets/… – tex Oct 19 '16 at 14:23
18

No, it is not 'bad practice'. So long as you and the other developers agree thats how you want it to work, its just fine. After all, it'd be returning an account, which is what you want. That the account is created 'under the hood' is irrelevant to the caller.

Improve this answer
  • 10
    Only exception is if it's a publicly available API. The larger community has already agreed that GET is indempotent and nullipotent; in other words, the same action is performed each time, and it's a safe method that doesn't change the state of the server. This is in the REST Wiki. Aside from that, if the API just exists in your own little world, do whatever is accepted by your group. – jmort253 May 22 '12 at 20:11
  • 9
    I disagree, even for a public API its fine - so long as it makes sense within the context of whatever the API is supposed to do. There's little point in creating multiple entries in the API when a single one would do the trick. – GrandmasterB May 22 '12 at 20:21
  • For the record it is an entirely internal library and I have the ability to just tell the team using it how it works – Mike May 22 '12 at 20:35
  • 1
    @jmort253: A service is nullipotent if it has no side effects visible to the client. The server can do as it pleases. – kevin cline Aug 26 '14 at 22:46
  • 1
    @kevincline, I guess I was thinking about it in terms of, let's say, charging my credit card. If the server charges my card based on a GET request but still gives me a result like "declined" every time I run it, that feels like it goes against the spirit of GET. With that said, I do see your point. The server could count the number of GET requests and lock me out after 3 queries, thereby rate limiting me, but without changing any details of my account. I do think that's an important distinction when one says the server state isn't modified. Perhaps I should say "client state on server" instead – jmort253 Aug 27 '14 at 2:23
10

If getAccount() can always return an account, then from the caller's point of view, the account does exist, and has always existed. There is no need to for getAccount() to 'create' anything. The account doesn't have to be stored anywhere until it is different from the default account.

Improve this answer
  • +1 Generally, GetOrCreate is the wrong semantic but getting an object which may "logically" exist whether or not it phyiscally exists is fine. As an example, a sparse array of mutable items may not have any storage allocated for element 1,841,533, but still allow that element to be "retrieved" by creating a new object, storing it, and returning a reference. – supercat Aug 26 '14 at 21:39
4

It makes most sense to create 3 methods:

getAccount -> Which just gets the account.

createAccount -> Creates an account.

getAccountAndCreateIfNeeded -> Choose your own naming ;)

Why separation: You have a simple method for getting and creating. That is a clear testable method for both. For getAccount it is not an exception to not find the account. So just return false or something like that, it is expected.

Then you can use that return value in your grouped function: getAccountAndCreateIfNeeded which is now also testable, it should return an account always. Whatever you ask.

All these 3 methods are clear, it is exactly clear what they do and what they return. You may make agreements now with your team but this kind of exceptions are terrible on the long run. Just make them very clear and you will have no issues.

Improve this answer
  • Also, from Clean Code: "If your method can't do what its name implies, then throw an Exception." I'd have a Try/Catch block inside 'GetOrCreateIfneeded' that throws on a failed GET and then has the 'createAccount' inside the Throw for whatever exception type comes back for the failed get. – Graham Aug 1 '12 at 19:21
  • I might suggest a fourth method: getAccountIfExists, which would either get an account or indicate that it doesn't exist without creating a new one. The getAccount method itself should presuppose that the account exists, and throw an exception if not. – supercat Dec 17 '14 at 22:44
2

It depends on the circumstances.

For example, you can use it to do lazy loading/instantiation, deferring the loading of data or the creation of an instance until it's actually needed. This is normally sensible as it saves resources that you might not need (if the class/data is never needed then it's never loaded).

However, in this particular case, I'd say that having a method called getAccount that would create a new account if it didn't exist would not be good practice. If the user has given some credentials to identify a particular account and that account couldn't be found, does that mean that the user is not yet a customer and should have an account created for them, or does it mean that the credentials were mistyped and the user needs to be asked to verify that they've entered what they meant to enter?

If you have a getAccount method that does create a new account if it couldn't identify one, then you have no choice in the matter. If you split account creation and account getting out into separate methods, then you have a lot more flexibility in deciding what to do should an attempt to get an account fails.

Improve this answer

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