Skip to content

Feedback #1

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

Closed
tonysneed opened this issue Jul 12, 2018 · 8 comments
Closed

Feedback #1

tonysneed opened this issue Jul 12, 2018 · 8 comments

Comments

@tonysneed
Copy link
Contributor

Rather than adding any code from URF.Core, what you need to do is simply add the URF.Core packages that you need.

You want to have just two projects:

  • URF.Core.Abstractions.Services.DomainModel
  • URF.Core.Services.DomainModel

Your solution should be the same name as your repo: URF.Core.DomainService.sln

Does this make sense?

@ozturkfatih
Copy link
Owner

ozturkfatih commented Jul 12, 2018

It is important for me that this solution is correct just for now. My problem is DTO for data transfer.

URF.Core can be add as a package as you mentioned. Project and Solution have the same name because this is only a sample solution in this package.

Is the solution correct?

@tonysneed
Copy link
Contributor Author

The approach looks good with the interface and class you added. But the best way to tell is for you to add a sample project that uses your solution.

Would it be too much trouble for you to refactor the project to add the URF package instead of the source code?

You should also add some unit tests.

Let me know if you need help with any of this.

@ozturkfatih
Copy link
Owner

ozturkfatih commented Jul 14, 2018

Hi Tony. Firstly, I changed the name of the project. And, I have added the necessary packages from URF.Core.

Test.DomainModelService is a unit test project. I tested Insert, Update and some methods.
Can you review for this issue . Also, What is the best solution for update method?

Attaching an existing entity to the context

@tonysneed
Copy link
Contributor Author

Terrific! I'll build and run the tests.

Say, can you please correct the links in your comments? See this.

@tonysneed
Copy link
Contributor Author

I’ll be able to give this a try sometime today.

@tonysneed
Copy link
Contributor Author

Created PR #2 with my refactoring. Please look at my changes, and I can answer any questions you have in the PR comments.

@tonysneed
Copy link
Contributor Author

Any questions abouty refactoring?

Does the Update method look OK?

@ozturkfatih
Copy link
Owner

When using the update method be careful. Because, If a property is missing on the DTO. This property is setting null when mapping DTO to the entity.

Your solution is better than the first solution. It's clean.

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

No branches or pull requests

2 participants