refactor: refactor domain and repositories #42
Conversation
WalkthroughThe updates predominantly involve a shift in the handling of customer identification, transitioning from string-based IDs to Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Out of diff range and nitpick comments (2)
src/FIAP.TechChallenge.ByteMeBurger.Api/ServiceCollectionExtensions.cs (1)
20-23: Consider removing the commented-out code for registering in-memory repositories to clean up the file and maintain clarity.src/FIAP.TechChallenge.ByteMeBurger.Domain/ValueObjects/Cpf.cs (1)
47-47: Consider renaming the variableworkingWpftoworkingCpfto correct the apparent typo and improve readability.
| } | ||
|
|
||
| public async Task<Customer> CreateAsync(Customer customer) | ||
| { | ||
| var param = (CustomerDto)customer; |
There was a problem hiding this comment.
Consider verifying the type of customer before casting to CustomerDto to prevent potential runtime errors.
- var param = (CustomerDto)customer;
+ if (customer is CustomerDto param) {
+ // Execute the query with param
+ } else {
+ // Handle the case where customer is not a CustomerDto
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| var param = (CustomerDto)customer; | |
| if (customer is CustomerDto param) { | |
| // Execute the query with param | |
| } else { | |
| // Handle the case where customer is not a CustomerDto | |
| } |
There was a problem hiding this comment.
the type of customer is already known in this case, no need to check
There was a problem hiding this comment.
@italopessoa, thank you for your feedback. It seems you have a strong understanding of the types being handled in this context. Given your confidence that the type of customer is consistently CustomerDto, the direct cast should indeed work as intended.
Just as a note, in broader contexts or public APIs, where the type of incoming data might be less controlled, adding a type check could be a safeguard against potential issues. However, it sounds like this is well-managed in your current setup. If you ever anticipate changes in the type handling or want to make this method more resilient to future changes, adding such checks might be something to consider.
Thank you for clarifying this! If there's anything else you need, feel free to ask.
| public static explicit operator CustomerDto(Customer customer) => new() | ||
| { | ||
| Id = customer.Id, | ||
| Name = customer.Name, | ||
| Email = customer.Email, | ||
| Cpf = customer.Cpf | ||
| }; |
There was a problem hiding this comment.
Consider handling the case where customer is null in the explicit conversion operator to prevent possible null reference exceptions.
- public static explicit operator CustomerDto(Customer customer) => new()
+ public static explicit operator CustomerDto(Customer? customer)
+ {
+ if (customer is null) return null;
+ return new CustomerDto
+ {
+ Id = customer.Id,
+ Name = customer.Name,
+ Email = customer.Email,
+ Cpf = customer.Cpf
+ };
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| public static explicit operator CustomerDto(Customer customer) => new() | |
| { | |
| Id = customer.Id, | |
| Name = customer.Name, | |
| Email = customer.Email, | |
| Cpf = customer.Cpf | |
| }; | |
| public static explicit operator CustomerDto(Customer? customer) | |
| { | |
| if (customer is null) return null; | |
| return new CustomerDto | |
| { | |
| Id = customer.Id, | |
| Name = customer.Name, | |
| Email = customer.Email, | |
| Cpf = customer.Cpf | |
| }; | |
| } |
| public class Customer : Entity<Guid> | ||
| { | ||
| public Cpf Cpf { get; set; } | ||
|
|
||
| public string? Name { get; private set; } | ||
|
|
||
| public string? Email { get; private set; } | ||
|
|
||
| public Customer() | ||
| : base(ValidateId(Guid.NewGuid().ToString())) | ||
| : base(Guid.NewGuid()) | ||
| { | ||
| Name = "Anonymous"; | ||
| } | ||
|
|
||
| public Customer(string cpf) | ||
| : base(ValidateId(cpf)) | ||
| public Customer(Guid id) | ||
| : base(id) | ||
| { | ||
| } | ||
|
|
||
| public Customer(string cpf, string name, string email) | ||
| : base(ValidateId(cpf)) | ||
| public Customer(Guid id, Cpf cpf) | ||
| : base(id) | ||
| { | ||
| ArgumentException.ThrowIfNullOrWhiteSpace(name); | ||
| Cpf = cpf; | ||
| } | ||
|
|
||
| public Customer(Cpf cpf) | ||
| : base(Guid.NewGuid()) | ||
| { | ||
| Cpf = cpf; | ||
| } | ||
|
|
||
| public Customer(string cpf, string name, string email) | ||
| : this(cpf) | ||
| { | ||
| Name = name; | ||
| Email = ValidateEmail(email); | ||
| Id = ValidateId(cpf); | ||
| Email = email; | ||
| Validate(); |
There was a problem hiding this comment.
Ensure that the Cpf property is initialized in all constructors or consider making it nullable to address potential issues with object initialization.
- public Customer() : base(Guid.NewGuid()) { }
+ public Customer() : base(Guid.NewGuid()) { Cpf = new Cpf("default-value"); }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| public class Customer : Entity<Guid> | |
| { | |
| public Cpf Cpf { get; set; } | |
| public string? Name { get; private set; } | |
| public string? Email { get; private set; } | |
| public Customer() | |
| : base(ValidateId(Guid.NewGuid().ToString())) | |
| : base(Guid.NewGuid()) | |
| { | |
| Name = "Anonymous"; | |
| } | |
| public Customer(string cpf) | |
| : base(ValidateId(cpf)) | |
| public Customer(Guid id) | |
| : base(id) | |
| { | |
| } | |
| public Customer(string cpf, string name, string email) | |
| : base(ValidateId(cpf)) | |
| public Customer(Guid id, Cpf cpf) | |
| : base(id) | |
| { | |
| ArgumentException.ThrowIfNullOrWhiteSpace(name); | |
| Cpf = cpf; | |
| } | |
| public Customer(Cpf cpf) | |
| : base(Guid.NewGuid()) | |
| { | |
| Cpf = cpf; | |
| } | |
| public Customer(string cpf, string name, string email) | |
| : this(cpf) | |
| { | |
| Name = name; | |
| Email = ValidateEmail(email); | |
| Id = ValidateId(cpf); | |
| Email = email; | |
| Validate(); | |
| public class Customer : Entity<Guid> | |
| { | |
| public Cpf Cpf { get; set; } | |
| public string? Name { get; private set; } | |
| public string? Email { get; private set; } | |
| public Customer() | |
| : base(Guid.NewGuid()) | |
| { | |
| Cpf = new Cpf("default-value"); | |
| } | |
| public Customer(Guid id) | |
| : base(id) | |
| { | |
| } | |
| public Customer(Guid id, Cpf cpf) | |
| : base(id) | |
| { | |
| Cpf = cpf; | |
| } | |
| public Customer(Cpf cpf) | |
| : base(Guid.NewGuid()) | |
| { | |
| Cpf = cpf; | |
| } | |
| public Customer(string cpf, string name, string email) | |
| : this(cpf) | |
| { | |
| Name = name; | |
| Email = email; | |
| Validate(); |
There was a problem hiding this comment.
there's no default value for Cpf it works like a SIN number
There was a problem hiding this comment.
@italopessoa Thank you for your clarification regarding the Cpf property. You're absolutely right; given that Cpf functions similarly to a SIN number, it should not have a default value and should be explicitly provided during the creation of a Customer object.
Considering this, it would be appropriate to ensure that all constructors of the Customer class require a Cpf to be provided. If there are scenarios where a Cpf might not be immediately available at the time of object creation, another approach could be to make the Cpf property nullable, though this would depend on the specific requirements and rules of your domain.
Here's a revised approach where Cpf is required in all constructors:
public class Customer : Entity<Guid>
{
public Cpf? Cpf { get; private set; }
public string? Name { get; private set; }
public string? Email { get; private set; }
public Customer(Cpf cpf)
: base(Guid.NewGuid())
{
Cpf = cpf;
}
public Customer(Guid id, Cpf cpf)
: base(id)
{
Cpf = cpf;
}
public Customer(Guid id, Cpf cpf, string name, string email)
: base(id)
{
Cpf = cpf;
Name = name;
Email = email;
Validate();
}
}Please let me know if this aligns with your expectations or if there are other adjustments you'd like to consider!
There was a problem hiding this comment.
yes, it makes sense. please open a follow up ticket for this
There was a problem hiding this comment.
@italopessoa The follow-up ticket has been successfully created to address making the Cpf property a required field in all constructors of the Customer class. You can view and track the progress of this issue here: GitHub Issue #43.
Please let me know if there's anything else I can assist you with!
Analysis chain
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Open a GitHub issue for making Cpf a required property in all constructors of the Customer class.
gh issue create --title "Require Cpf in all constructors of Customer class" --body "As discussed in PR https://github.com/soat-fiap/FIAP.TechChallenge.ByteMeBurger/pull/42 and the specific comment https://github.com/soat-fiap/FIAP.TechChallenge.ByteMeBurger/pull/42#discussion_r1589776399, it is necessary to make the Cpf property a required field in all constructors of the Customer class to ensure that each customer has a Cpf assigned upon creation. This change aligns with the domain requirements where Cpf functions similarly to a SIN number and should not have a default value. Requested by @italopessoa."
Length of output: 684
| @@ -55,7 +55,7 @@ public async Task GetAll_Success() | |||
| public async Task Create_Success() | |||
| { | |||
| // Arrange | |||
| var order = new Order(Guid.NewGuid().ToString()); | |||
| var order = new Order(Guid.NewGuid()); | |||
There was a problem hiding this comment.
Consider adding actual SQL query strings in the mock setup for ExecuteAsync to ensure the test is meaningful and accurately reflects the intended database interactions.
| @@ -27,7 +27,7 @@ public CustomerRepositoryDapperTest() | |||
| public async Task Create_Success() | |||
| { | |||
| // Arrange | |||
| var customer = new Customer(Guid.NewGuid().ToString()); | |||
| var customer = new Customer(_cpf); | |||
There was a problem hiding this comment.
Consider adding actual SQL query strings in the mock setup for ExecuteAsync to ensure the test is meaningful and accurately reflects the intended database interactions.
|
|
||
| // Assert | ||
| using (new AssertionScope()) | ||
| { | ||
| result.Should().NotBeNull(); | ||
| result.Should().BeEquivalentTo(expectedCustomer, options => options.ComparingByMembers<Customer>()); | ||
| result.Cpf.Should().BeNull(); |
There was a problem hiding this comment.
The assertion result.Cpf.Should().BeNull(); seems incorrect as the CPF should not be null for a successfully retrieved customer. Consider correcting this to reflect the expected behavior.
There was a problem hiding this comment.
@italopessoa Thank you for updating the test case! If there's anything else you need help with, feel free to ask. Happy coding! 🐰
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| // customer.Id.Should().NotBeNull(); | ||
| // customer.Id.Should().NotMatchRegex(@"^\d{11}$"); |
There was a problem hiding this comment.
Consider uncommenting the assertions for customer.Id and customer.Cpf to ensure the test fully validates the properties of an anonymous customer.
| public string? Name { get; private set; } | ||
|
|
||
| public string? Email { get; private set; } | ||
|
|
||
| public Customer() | ||
| : base(ValidateId(Guid.NewGuid().ToString())) | ||
| : base(Guid.NewGuid()) |
There was a problem hiding this comment.
Consider making Cpf nullable or requiring it in all constructors to ensure proper initialization.
| public Customer(string cpf, string name, string email) | ||
| : this(cpf) | ||
| { | ||
| Name = name; | ||
| Email = ValidateEmail(email); | ||
| Id = ValidateId(cpf); | ||
| Email = email; | ||
| Validate(); |
There was a problem hiding this comment.
The constructor call this(cpf) is incorrect as no such constructor exists that takes a single string argument. Consider using new Cpf(cpf) or adjusting the constructor signature.
- : this(cpf)
+ : this(new Cpf(cpf))Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| public Customer(string cpf, string name, string email) | |
| : this(cpf) | |
| { | |
| Name = name; | |
| Email = ValidateEmail(email); | |
| Id = ValidateId(cpf); | |
| Email = email; | |
| Validate(); | |
| public Customer(string cpf, string name, string email) | |
| : this(new Cpf(cpf)) | |
| { | |
| Name = name; | |
| Email = email; | |
| Validate(); |
| result.Should().BeEquivalentTo(expectedCustomer, | ||
| options => options.ComparingByMembers<Customer>().Excluding(c => c.Cpf)); | ||
| result.Cpf.Value.Should().Be(expectedCustomer.Cpf); |
There was a problem hiding this comment.
The assertion excludes the Cpf property, which is incorrect as Cpf is a critical part of the Customer entity. Consider including it in the equivalence check.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/FIAP.TechChallenge.ByteMeBurger.Domain/Entities/Order.cs (2 hunks)
- src/FIAP.TechChallenge.ByteMeBurger.Domain/ValueObjects/Cpf.cs (1 hunks)
- tests/FIAP.TechChallenge.ByteMeBurger.Infrastructure.Test/Repository/CustomerRepositoryDapperTest.cs (4 hunks)
Files skipped from review as they are similar to previous changes (3)
- src/FIAP.TechChallenge.ByteMeBurger.Domain/Entities/Order.cs
- src/FIAP.TechChallenge.ByteMeBurger.Domain/ValueObjects/Cpf.cs
- tests/FIAP.TechChallenge.ByteMeBurger.Infrastructure.Test/Repository/CustomerRepositoryDapperTest.cs
No description provided.