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

397 make fields and methods protected #398

Merged
merged 4 commits into from
May 21, 2024

Conversation

eldamir
Copy link
Contributor

@eldamir eldamir commented May 9, 2024

Fixes #397

@@ -10,8 +10,8 @@ namespace Ardalis.Specification.EntityFramework6;
/// <inheritdoc/>
public abstract class RepositoryBase<T> : IRepositoryBase<T> where T : class
{
private readonly DbContext _dbContext;
private readonly ISpecificationEvaluator _specificationEvaluator;
protected readonly DbContext _dbContext;
Copy link

Choose a reason for hiding this comment

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

I think a property

protected DbContext { get; private set; }

combined with

public RepositoryBase(DbContext dbContext)
{
    DbContext = dbContext;
}

is more appropriate then a protected member.

The same for SpecificationEvaluator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't want to change too much, but I see your point. If we let subclasses access these "fields" it will be safer to do "props" instead from a maintenance perspective. I've made that change and pushed

@ardalis
Copy link
Owner

ardalis commented May 13, 2024

If you use a property with private set; doesn't that still limit extensibility since there is no way to replace a dependency with a different implementation? Sure, readonly access is better than no access, but I thought the intent of #397 was to allow the replacement of the dependency if desired...

@eldamir
Copy link
Contributor Author

eldamir commented May 13, 2024

For my needs, I only needed to override the methods while still being able to refer to and read the fields (now props) from the subclass. I don't need to mess with the constructor signatures or replace/modify the state. Only the behaviour.

So for me, this is enough. I can see a case for opening it up further. If you believe this is the way to go, I'll happily remove the private markers 😊

@ardalis
Copy link
Owner

ardalis commented May 13, 2024

I'm not clear on what the benefits of the property are over the field, in this case. The field change (as originally done in this PR) seems simpler and provides greater extensibility. The property adds additional encapsulation, but I'm not sure that's needed or desirable in this case.

However, I recognize I'm not the target audience for this change, so if y'all are ok with it as is I think we can merge it. Let me know.

This affords maximum extensibility, while still encapsulating
the values within a property
@eldamir
Copy link
Contributor Author

eldamir commented May 14, 2024

In that case, let's opt for maximum extensibility with maximum encapsulation. Most power and most safety. I've removed the private keyword to make the properties protected on both get and set

@eldamir eldamir requested a review from enrij May 14, 2024 04:08
@enrij
Copy link

enrij commented May 14, 2024

TL;DR: Holy 💩, I didn't mean to start such a conversation about a property 🤣... Any solution allowing te behavior described here is fine for me.

Longer version
Protected properties with private sets look like the cleanest way to create RepositoryBase<T> children with custom behavior. Personally, I'm not a big fan of field inheritance and I always saw this as a code smell (thus my suggestion for the property).

As per @ardalis observations about extensibility, I opened #397 mostly because I needed a way to "override" the auto-save behavior in the methods and therefore, as @eldamir correctly pointed out, you need to access the DbContext somehow. Furthermore, having injected the generic DbContext trough the constructor provides enough flexibility even for scenarios with multiple contexts.

Copy link

@enrij enrij left a comment

Choose a reason for hiding this comment

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

See comment here ⬆️

@eldamir
Copy link
Contributor Author

eldamir commented May 21, 2024

I think we landed in the best spot here. No "field inheritance", and properties are available for read and write as needed by inheritors with proper encapsulation. I'd be happy to see this merged 😉

@ardalis ardalis merged commit 4e54413 into ardalis:main May 21, 2024
1 check passed
@ardalis
Copy link
Owner

ardalis commented May 21, 2024

Now I just need to release it...

@gocampo
Copy link

gocampo commented Jun 10, 2024

Just what I needed!! Waiting the release

@eldamir eldamir deleted the 397-make-fields-and-methods-protected branch June 17, 2024 10:40
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.

Make repository methods protected to allow for subclassing
4 participants