Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added Method PUT #27

Merged
merged 2 commits into from
May 4, 2018
Merged

Added Method PUT #27

merged 2 commits into from
May 4, 2018

Conversation

amadeu01
Copy link
Contributor

Implement other HTTP verbs #8
Added PUT verb

@codecov-io
Copy link

codecov-io commented Feb 12, 2018

Codecov Report

Merging #27 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #27   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          23     24    +1     
  Lines         278    246   -32     
  Branches       11      0   -11     
=====================================
- Hits          278    246   -32
Impacted Files Coverage Δ
Sources/Frisbee/Requestables/NetworkPut.swift 100% <100%> (ø)
...ces/Frisbee/Factories/ResultGeneratorFactory.swift 100% <0%> (ø) ⬆️
Sources/Frisbee/Entities/Result.swift 100% <0%> (ø) ⬆️
Sources/Frisbee/Adapters/QueryItemAdapter.swift 100% <0%> (ø) ⬆️
Sources/Frisbee/Adapters/EncoderJSONAdapter.swift 100% <0%> (ø) ⬆️
Sources/Frisbee/Adapters/URLQueryAdapter.swift 100% <0%> (ø) ⬆️
Sources/Frisbee/Adapters/DecoderJSONAdapter.swift 100% <0%> (ø) ⬆️
Sources/Frisbee/Interactors/DataTaskRunner.swift 100% <0%> (ø) ⬆️
...e/Factories/Adapters/EncodableAdapterFactory.swift 100% <0%> (ø) ⬆️
Sources/Frisbee/Factories/URLSessionFactory.swift 100% <0%> (ø) ⬆️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a4744a...c3153ab. Read the comment docs.

Copy link
Owner

@ronanrodrigo ronanrodrigo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I just made a request-change about access control and unit tests at NetworkPut.

Thanks for the support @amadeu01

@@ -0,0 +1,17 @@
import Foundation

protocol Putable {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this public

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -0,0 +1,70 @@
import Foundation

public final class NetworkPut: Putable {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you make unit tests for this guy?

Copy link
Owner

@ronanrodrigo ronanrodrigo Mar 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to write this comment 🤪, sorry

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure. I will

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@ronanrodrigo ronanrodrigo mentioned this pull request Mar 12, 2018
3 tasks
@amadeu01
Copy link
Contributor Author

@ronanrodrigo I forgot to update my PR, sorry. I'll do it soon

@amadeu01
Copy link
Contributor Author

Hi @ronanrodrigo

I will update my PR soon, sorry for my delay

@amadeu01
Copy link
Contributor Author

amadeu01 commented May 4, 2018

@ronanrodrigo o put é muito parecido com o post

POST:

Used to modify and update a resource

 POST /questions/<existing_question> HTTP/1.1
 Host: www.example.com/

Note that the following is an error:

POST /questions/<new_question> HTTP/1.1
Host: www.example.com/
If the URL is not yet created, you should not be using POST to create it while specifying the name. This should result in a 'resource not found' error because <new_question> does not exist yet. You should PUT the <new_question> resource on the server first.

You could though do something like this to create a resources using POST:

POST /questions HTTP/1.1
Host: www.example.com/

Note that in this case the resource name is not specified, the new objects URL path would be returned to you.

PUT:

Used to create a resource, or overwrite it. While you specify the resources new URL.

For a new resource:

PUT /questions/<new_question> HTTP/1.1
Host: www.example.com/

To overwrite an existing resource:

PUT /questions/<existing_question> HTTP/1.1
Host: www.example.com/ 

O que acaba acontecendo que os metodos entre put e post são iguais basicamente, o que vai mudar é que o usuário vai poder usar um, ou outro. Mas, vai parecer codigo duplicado.

@unnamedd
Copy link

unnamedd commented May 4, 2018

Actually, POST is usually used to create new resources and PUT to update some existing resource. Yes, they are really similar but it is a semantic point. If you want, you can use GET to create new resources (or update), but you should not use.

@ronanrodrigo
Copy link
Owner

May appear to be a similar code. But they do different things. And because of this they are different code. Maybe some refactor to aggregate them come up one day. But first, we must to understand if that code repetition are accidental or no. And after this we can proceed with some strategy to make the refactor. Baby steps 🚼

By accidental duplication:

“Duplication is generally a bad thing in software. We don’t like duplicated code. When code is truly duplicated, we are honor-bound as professionals to reduce and eliminate it.

But there are different kinds of duplication. There is true duplication, in which every change to one instance necessitates the same change to every duplicate of that instance. Then there is false or accidental duplication. If two apparently duplicated sections of code evolve along different paths—if they change at different rates, and for different reasons—then they are not true duplicates. Return to them in a few years, and you’ll find that they are very different from each other.

Now imagine two use cases that have very similar screen structures. The architects will likely be strongly tempted to share the code for that structure. But should they? Is that true duplication? Or it is accidental?

Most likely it is accidental. As time goes by, the odds are that those two screens will diverge and eventually look very different. For this reason, care must be taken to avoid unifying them. Otherwise, separating them later will be a challenge.

When you are vertically separating use cases from one another, you will run into this issue, and your temptation will be to couple the use cases because they have similar screen structures, or similar algorithms, or similar database queries and/or schemas. Be careful. Resist the temptation to commit the sin of knee-jerk elimination of duplication. Make sure the duplication is real.”

Excerpt From: Robert C. Martin. “Clean Architecture: A Craftsman's Guide to Software Structure and Design (Robert C. Martin Series).” iBooks.

@amadeu01
Copy link
Contributor Author

amadeu01 commented May 4, 2018

@ronanrodrigo
Well, it seems that I have done everything you asked to be fixed. So, there is anything else I can do to the PR is accepted?

@ronanrodrigo
Copy link
Owner

@amadeu01 I just need to finish the review. I didn't start yet :)

Copy link
Owner

@ronanrodrigo ronanrodrigo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job @amadeu01 thanks for your contribution. And we will keep talking about the refactor 👍

@ronanrodrigo ronanrodrigo merged commit ad59ed9 into ronanrodrigo:master May 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants