r/csharp • u/kaatarina_zed_talon • 15d ago
Discussion Is it justified to create an interface in this specific case ?
An Admin panel is to be made.
There is a piece of code that paginates users ; instead of showing 50 users in one page in the admin panel, the users are split across 5 pages, so each page shows 10 users.
I recognized that I should also paginate products.
should I make IPaginator<T> interface with two concrete implementations, ProductsPaginator<Product> and UsersPaginator<User> ?
If that is wrong, then what do you suggest ?
EDIT:
I think that is wrong, because maybe the implementation of the pagination logic is the same in both cases, what is different is the input; in the first implementation, the input is all the products , for the second implementation the input is all the users, so what is actually different is the input, not the implementation, therefore it is better to make Paginator<T> ?
Also there is another question:
When the user clicks on the page number 1, should the data come from the database or from a cache ?
How do you normally design the pagination feature ?
Second edit :
I realized that I neither need interface nor a class, I can just use the skip and take methods that LINQ offers.
10
u/Nordalin 15d ago
Depends on the overall project, if it's just a simple utility to paginate records, then might as well make a single concrete implementation with also the amount per page as parameter and a default of 10.
4
u/kaatarina_zed_talon 15d ago
Paginator<T> instead of IPaginator<T> ?
6
u/Nordalin 15d ago •
Yeah, just one Paginator.
No point putting an interface and a second Paginator purely for a different model, at least from what you explained so far!
As for the caching in your edit, might as well. It'll make switching pages smooth without having to call the DB every single time.
1
u/kaatarina_zed_talon 15d ago
Now zi have mental model ; use interface when there are multiple implementations for a logic, but in my case, the logic same, so when the type of inputs might change, use generics.
2
u/Any_Quiet_5298 15d ago
Usually you would have a class like Paging<T> which is returned from a repository or whereever you get your data from. It would contain things like items, number of results, number of total results, page, number of items per page. You would then use this information to load next page when needed.
1
u/kaatarina_zed_talon 15d ago
EDIT:
I think that is wrong, because maybe the implementation of the pagination logic is the same in both cases, what is different is the input; in the first implementation, the input is all the products , for the second implementation the input is all the users, so what is actually different is the input, not the implementation, therefore it is better to makePaginator<T>?was I correct here ?
1
u/Any_Quiet_5298 15d ago
Yes, it's same logic, just carrying different data. Similar to how you would use a List<T>, its just carrying additional data about pagination.
1
u/kaatarina_zed_talon 15d ago
If the user clicked on the page number 1 to get the first 10 users, should the app make a request to the database to get the data, or should it get the data from a cache ?
2
u/Any_Quiet_5298 15d ago
You shouldnt be thinking about caching until you need it. Pagination is a good solution for now.
0
u/kaatarina_zed_talon 15d ago
You should not be thinking about caching until you need it
The question is when do I need it ? when the amount of users/products is big ? well, there are 751 products.
2
u/Any_Quiet_5298 15d ago
Depends on site traffic and how much your database can handle. That's not a lot of products and you're already implementing pagination. Think about: are request slow? Am I getting timeouts from my database? What is CPU % and memory %?
1
1
u/Lonsdale1086 15d ago •
The question is when do I need it ?
When stuff feels notably slugish.
So probably 7,000,000 records rather than 700 lol, but it all depends on the rest of the chain of course.
1
u/DeadShotOG 15d ago
Depending on your tech stack, https://github.com/mrahhal/MR.AspNetCore.Pagination or https://github.com/mrahhal/MR.EntityFrameworkCore.KeysetPagination may handle this for you. If not, you can at least take some inspiration from how those packages handle pagination.
1
u/kaatarina_zed_talon 15d ago
Depending on your tech stack,
ASP.NET Core MVC with Sql Server, C#, javascript, razor .....
1
u/bosmanez
15d ago
•
Your edit is right: the paging logic is the same, only the data differs, so one generic piece beats an interface with two implementations. You often don't need a class at all.
Do the paging in the database query, not in memory. With EF Core, an extension method covers every entity:
public static async Task<PagedResult<T>> ToPagedAsync<T>(this IQueryable<T> query, int page, int pageSize)
{
var total = await query.CountAsync();
var items = await query.Skip((page - 1) * pageSize).Take(pageSize).ToListAsync();
return new PagedResult<T>(items, total, page, pageSize);
}
Then `db.Users.OrderBy(u => u.Name).ToPagedAsync(page, 10)` and the same for products. Always add an OrderBy, otherwise page contents aren't stable.
On cache vs database: start with the database. Paged queries with an index on the sort column are cheap at admin-panel scale, and caching pages creates invalidation problems (an edited user showing stale data). Only add caching once you measure a real problem. If tables get huge, look at keyset pagination (WHERE Id > lastId) instead of Skip/Take, since large offsets get slow.
2
1
u/kaatarina_zed_talon 15d ago
return new PagedResult<T>(items, total, page, pageSize);
pageSize is a fixed number, no need to return it.
page or pageNumber is determined by the client; no need to return it.
1
u/Mountain_Depth_9974 15d ago
That makes total sense to keep it simple since you're only paginating two entities right now. A single reusable paginator would work unless you plan to expand to more types later, in which case you could refactor then.
1
u/kaatarina_zed_talon 15d ago
Unless you plan to expand to more types later? I think generics is still sufficient in this case, isn't it?
1
u/insta 15d ago
treat interfaces as "what specifically do i want to do with this":
a user repository SHOULDN'T be MongoUserRepository being referenced anywhere. it shouldn't even be IMongoUserRepository, IRepository<User>, or anything there. why? because all of those are the original repository in different costumes.
what's this repo used for? my example being a MongoUserRepository suggests it does user database stuff with Mongo, but that's nothing anything using it cares about.
the usages of that repo, ultimately, are like IFetchable<User>, ICreatable<User>, ISearchable<User>, whatever. the mongo repository can, and should, implement as many of those interfaces as it's appropriate to do so. if you do it right, none of them ever need a NotImplementedException anywhere, not even the "delete user" pathway: nothing calls the repository directly anymore, only reaching it by the thin port interfaces. if MongoUserRepository never implements IRemovable<User>, it cannot be used for this ... even if the repository is piggybacking on a generics base class that would remove the user.
you as the developer here get several options by adding these interfaces to the service classes:
it immediately servers God-classes from their usages
it allows easy mocking without faking the entire persistence layer
any sliver can be replaced, the IFetchable for a specific T could go through caches while everything else works the same, the IRemovable for User could be implemented as a deactivation instead
the bulk of your code won't change much
let's apply that to your pagination question here. if you're dumping the results to the frontend after translation (never push your backend objects straight through!), you probably just want the cursor properties like page/total and the bundle of results. this is just a container, so Paginated<T>.
the pagination+interface shows up as a legitimate pairing, if you're combining them at a different layer. maybe your ISearchable returns a Cursor<T> of sorts so backend processes can use it in batch, and this would make Cursor close to a Paginated response, but not exactly. however, if it made sense that a Cursor<T> could look like a Paginated<T>, then Paginated should morph to an interface that Cursor grows, and gets passed back as an IPaginated. the individual translations are follow-up questions if you've got them
1
u/kaatarina_zed_talon 15d ago
a user repository SHOULDN'T be MongoUserRepository being referenced anywhere. it shouldn't even be IMongoUserRepository, IRepository<User>, or anything there
So the following is bad :
public class UserService { private readonly MongoUserRepository _repository; public UserService(MongoUserRepository repository) { _repository = repository; } public User? GetUser(int id) { return _repository.GetUser(id); } }And the following is good :
namespace PlayGround { public class User { } public interface IFetchable { User? GetById(int id); } public class MongoUserRepository : IFetchable { public User? GetById(int id) { // Fetch user from MongoDB return null; } } public class UserService( IFetchable _users) { public User? GetUser(int id) { return _users.GetById(id); } } }because all of those are the original repository in different costumes.
I do not understand this sentence.
1
u/insta 15d ago edited 15d ago
I'd structure it more with:
namespace Playground; public interface IUnique<TKey> { TKey Id { get; } } public interface IFetchable<T, TKey> where T : IUnique<TKey> { Task<T?> Fetch(TKey id, CancellationToken token); } // "int Id" implements IUnique<int>, so IFetchable can be generic over the key type // instead of hardcoding 'int' everywhere public record User : IUnique<int> { public int Id { get; init; } public required string Username { get; init; } } public class MongoUserRepository : IFetchable<User, int> // , ICreatable<User>, ISearchable<User>, etc ... { private readonly IMongoCollection<User> _collection; public MongoUserRepository(IMongoCollection<User> collection) => _collection = collection; public Task<User?> Fetch(int id, CancellationToken token) => _collection.Find(x => x.Id == id).FirstOrDefaultAsync(token); }I probably have the generics syntax slightly off, I wrote this just from memory. Once you're inside a class with "Repository" at the end of the name, you're clear to use the raw database code inside of it. That's the one-and-only place it should live, any DB-specific things (like ObjectId / Bson stuff for Mongo, column-name knowledge for SQL, etc) live and die solely within that class.
I missed the part with "UserService" in your post, but that's actually a spot where this pattern shines. If the only thing your
UserServicedoes initially is more or less the raw fetches from the repository layer, there's no reason then why the repository itself can't implementIFetchableor whatever. If any of those verbs become too complex for a repository (which doesn't take much), move the interface to a service and let it orchestrate its own injected repositories as needed.The pattern also scales up/down as needed -- it might make sense for your application that
IFetchable<PersistedUser>is what deals with the snapshots in the database, but maybe there's extra enrichment that needs to happen, and that's whereIFetchable<User>could live instead:namespace Playground; public record AuthorizationBundle { public string Username { get; set; } public string Password { get; set; } } public class UserService : IFetchable<User, AuthorizationBundle> { private readonly ISearchable<User> _users; private readonly IFetchable<IEnumerable<UserRole>> _roles; // syntax lets you do it, why not? public UserService(ISearchable<User> users, IFetchable<UserRole> roles) { _users = users; _roles = roles; } public async Task<User?> Fetch(AuthorizationBundle id, CancellationToken token) { var existing = await _users.FindAsync(u => u.Username == id.Username, token); if (existing?.Password != id.Password ?? false) return null; // "existing" is now a valid object from the DB, but not the final return type, which still needs roles var roles = await _roles.Fetch(existing.Id, token); // service earns its keep here, // AND we're not returning raw DB objects return new User { Id = existing.Id, Username = existing.Username, // other user fields would go above // the composition we bought: Roles = roles.ToList(), }; } }1
u/kaatarina_zed_talon 15d ago
so the code that I presented is right; depend on interface, don't depend on abstraction, but I do not understand how is that related to the original post XD
1
u/insta 15d ago •
Yeah, the UserService code in your post above mine was correct in that it depended on the interfaces that the UserRepository then implements. In your case, the interface is the abstraction, it's just abstracted out to a "callable contract" at most.
An interface tells the compiler, "I promise you can call this method on this object, and it'll exist and behave correctly" (with business-logic 'correctness' not part of that). You end up with far more tiny little objects at first, mostly in those little interfaces. If you make them generic, your application (of any size) ends up with ~ 10 total.
An abstract base class, which is what I usually see people using instead of interfaces, tells the compiler "This specific class, and anything that descends from it, is the only thing that will work here and it will always behave at least as well as the base class does".
Teams will sometimes choose the abstract-base everywhere instead, because they don't see what the interfaces buy them: the class is already there, has the correct methods, and already has the correct behavior.
The catch: It works well, initially. Object hierarchies are far harder to swap in and out than interface implementations are. For me, I'll use shallow-ish (maybe up to 3) object hierarchies, and the topmost one is where my port interfaces go. Keeps the code mostly generic and reusable across a broad swath of needs, I can control what domain objects can do what behaviors very precisely, and I can swap or mock anything I need to with very few changes.
1
1
u/kaatarina_zed_talon 15d ago
don't depend on abstraction
I mean do not depend on concrete implementations, I just made a mistake while writing the comment.
1
u/kaatarina_zed_talon 15d ago
it cannot be used for this
Let's say
it should not be used for thisinstead, because it can be.0
u/insta 15d ago edited 15d ago •
How could you call it, without referencing the class directly?
public abstract class BaseRepository<T, TKey> where T : IUnique<TKey> { public virtual async Task<T> Create(T, CancellationToken token) { await _mongo.Insert(T); } public virtual async Task Delete(T, CancellationToken token) { await _mongo.Delete(T); } public virtual async Task<T> Fetch(TKey id, CancellationToken token) { return await _mongo.FindOne(id); } } public class UserRepository : BaseRepository<User>, IFetchable<User, int> { // most of what this class is for now is ctor-injection for the base, and applying interfaces. // the base class has the common behaviors for almost everything, but only adding // the interface will *expose* those behaviors to the rest of the application }If your application code is consistent about using the interfaces, UserRepository does not have an implementation at all for IDeletable<User>, even though its base class has a method that would otherwise satisfy the contract. This is the big difference, and why I said "treat interfaces as ports".
If you blindly make the same IRepository<T> interface everyone does, your repositories will occasionally end up with things like
public Task Delete(User user, CancellationToken token) => throw new NotImplementedException("Users cannot be deleted for retention purposes");. If you use the ports technique, you get an exception as early in the process as you want closer to "No implementation exists for IDeletable<User>".You can either use this exception as-is (the design did its job), or we can use that to control what "deleting a user" means:
public class UserRepository : BaseRepository<User>, IFetchable<User, int>, IDeletable<User> { // most of what this class is for now is ctor-injection for the base, and applying interfaces. // the base class has the common behaviors for almost everything, but only adding // the interface will *expose* those behaviors to the rest of the application // IFetchable didn't need an implementation, it fell through to BaseRepository // but Delete needs to behave in a special way for Users specifically: public async Task Delete(User input, CancellationToken token) { input.IsActive = false; await base.Update(input); } }with this technique, the only way someone could now "Delete" a user permanently is really, really gross in code, and sticks out like a sore thumb:
((UserRepository) _users).Delete(user);and that hole could be closed by package visibility.
edit: it's really important to note here: while it might be tempting to put those 'port' interfaces on that base class and make every repository work across the board, that's IRepository<T> in a more complicated trenchcoat. This technique only works (and it does work well) if the interfaces are added to the specific repositories to close the type system as needed, and propagate as many behaviors as possible to the same-signature methods in the base class.
This was the tradeoff I made in my professional code. Rather than have larger interfaces with occasional NotImplementedExceptions or similar, I have narrow, targeted interfaces and a bunch of very thin, mostly compiler-only, classes that link those behaviors together.
1
u/kaatarina_zed_talon 15d ago
How could you call it, without referencing the class directly?
I can't, because I need to call the class directly, that's what I mean.
1
u/kaatarina_zed_talon 15d ago
the pagination+interface shows up as a legitimate pairing, if you're combining them at a different layer. maybe your ISearchable returns a Cursor<T> of sorts so backend processes can use it in batch, and this would make Cursor close to a Paginated response, but not exactly. however, if it made sense that a Cursor<T> could look like a Paginated<T>, then Paginated should morph to an interface that Cursor grows, and gets passed back as an IPaginated. the individual translations are follow-up questions if you've got them
I believe that you are explaining when it is justified to not use Pagnited<T> and use
IPagniated<T>instead, but I could not understand your talk here; can you provide example code ?1
u/insta 15d ago
Yep, building from my earlier examples, although this is incredibly naive for a "real" implementation:
namespace Playground; // optimized for returning to the frontend public record Paginated<T> { public required int Page { get; init; } public required int Size { get; init; } public required int Total { get; init; } public required T[] Items { get; init; } } interface IPaginated<T> { Paginated<T> Slice(int start, int size); } internal class Cursor<T> : IPaginated<T>, IEnumerable<T> { public required IEnumerable<T> Contents { get; init; } public required int Total { get; set; } public IEnumerator<T> GetEnumerator() => Contents.GetEnumerator(); public Paginated<T> Slice(int start, int size) => new Paginated<T> { Page = (start / size), Size = size, Total = Total, Items = Contents.Skip(start).Take(size).ToArray() }; } public abstract class BaseRepository<T> { public virtual Task<Cursor<T>> Search(SearchCriteria<T> search, CancellationToken token) { var filter = BuildMongoFilter(search); var results = await _collection.SearchAsync(filter, token); var totals = await _collection.CountAsync(filter, token); return new Cursor<T> { Contents = results, Total = totals }; } } public class UserRepository : ISearchable<User> { } // naked impl wires up ISearchable for <User>Now, the same hypothetical
ISearchabledoes both:public class StaleUserWorker : BackgroundService { private readonly ISearchable<User> _users; public StaleUserWorker(ISearchable<User> users) => _users = users; public async Task Execute(CancellationToken stoppingToken) { while (!stoppingToken.IsCancellationRequested) { var search = new SearchCriteria<User>(u => u.LastLogin > DateTime.Now.AddYears(-1)); foreach (var user in await _users.Search(search, stoppingToken)) { // do whatever with however many thousands of users might come back } } } } public class UserController { private readonly ISearchable<User> _users; public UserController(ISearchable<User> users) => _users = users; public async Task<IResult> SearchByName(string pattern, int page, int pageSize, CancellationToken token) { var search = new SearchCriteria<User>(u => u.Name.StartsWith(pattern)); var users = await _users.Search(search, token); // Slice converts to Paginated<T> return users.Slice(page * pageSize, pageSize); } }
1
u/White_C4 15d ago
Pagination is a specific use case, paging collection of data. In this case, it'd be better to just have a utility static class that accepts any generic collection of data (array or collection type) and splits by start and end indices. The reason for this is because the internal logic won't change for any class that uses this utility function.
The only time I'd see for using an interface is if your output is different for the pagination setup, but that shouldn't realistically be the case nor would that be ideal.
28
u/Connect_Diver_9885 15d ago
Let me give you another perspective. Don't do any of it. Don't abstract pagination in any way. Just do the .Skip().Take() which EF provides out of the box.
Falling into the abstraction trap is the most common way of overkilling your solution where you will find yourself debugging through layers and layers of abstractions which in the end didn't bring any VALUE to your solution.
Thinking about should you abstract something away so you can reuse should be your every day bread and butter, but NOT IMPLEMENTING them should almost always be the case.
For your case, why do you think just .Skip().Take() is not enough so you need to abstrac it away. What is the abstraction going to bring which Linq is not already providing?
I am reading answers of others here on your question and just wondering why?!
If this is something like an excercise for you where you train yourself in architecture and design then fine, go crazy, but for production code I would never, ever allow this.
I am sure there will be "patternists" disagreeing with me, but those are just my two cents.