Moves resource classes into separate files #4
Conversation
code/job.py
Outdated
| 'salary': data['salary'], | ||
| 'status': data['status'] | ||
| } | ||
| jobs.append(job), 201 |
There was a problem hiding this comment.
Ah, REST, how you are a monumental pain in the arse! I have no idea why the spec for REST states that you should use POST to create new items, but PUT is for updating them... unless you want to let it also create new items!
Personally, I would leave PUT for just updating existing items, as this then gives the PUT command itself more meaning. You are saying that this is for updates only, so then you can add checks to the method (return a 400 if the item doesn't exist). It also stops the accidental creation of new items because of a typo in the title.
You then end up with a smaller method that is doing the absolute minimum amount of work, so should theoretically be easier to understand.
Alternatively (though I wouldn't advise it), if you insist on the PUT method doing both things, then you might as well sack off the POST method, because what's the point of it?
There was a problem hiding this comment.
Not sure if it's a terrible idea, but did think the PUT could return POST method if the job doesn't already exist? i.e.:
... if jobs.get(title):
jobs[title].update(data), 200
return jobs[title]
else:
return self.post(title)
There was a problem hiding this comment.
Is it a terrible idea? Not really. However, it feels a lot cleaner to have the method only do one thing.
My argument here would be, so why doesn't the POST method call the PUT method if the job already exists, rather than return an error? Sure, most definitions of REST state that the POST method should only create new items, not update existing ones, but why the difference with PUT then? I fear that REST descends in to ideological wankery far too easily; any protocol that it is deemed ok to end a discussion with "But that's not in accordance with the principles of REST" needs to have a long, hard look at itself!
There was a problem hiding this comment.
I agree it feels a lot cleaner to have each method only doing one thing, and I definitely didn't like the repetition needed for creation in both methods. I don;t think my alternative (calling POST within PUT) isn't great as it's a bit misleading.
So simplifying PUT to just update or return an error (item not found) seems the cleanest option!
There was a problem hiding this comment.
It's not just cleaner, it's safer. I wouldn't want an update method where I can send the data up incorrectly (and mangle, or miss out, the id), and suddenly I've got a new entity in my data store. And I won't know that until I look in the data store.
| jobs = {} | ||
|
|
||
| class Job(Resource): | ||
| parser = reqparse.RequestParser() |
There was a problem hiding this comment.
This might be a perfectly valid Python thing, but how come the parser is being set up as what would be called a static member in C#? i.e. rather than putting it in the init method and assigning it to self.parser.
The main reason I'm asking is that this class feels difficult to write unit tests for. I don't know how much of a thing this is in the Flask world, but you can use this package:
https://github.com/alecthomas/injector
to add the ability to inject dependencies in to url handlers. So, here you could have the parser injected in the constructor, which would then give you the ability to mock it in a unit test, and therefore be able to write unit tests much more easily.
There was a problem hiding this comment.
To expand on this some more; I know that what you've got here is a very simple example ( no data store, for example), but I would be looking to split the data store (jobs) and the code that interacts with it, out in to a separate service class. This service class would then be injected in to this class. So this class would consist of the code to deal with getting the fields from the request, and the service class would deal with that data.
Does that make sense?
This commit does the following: