“Stop Using Good Names in Your Code” | Code Cop
2024 ж. 20 Нау.
29 111 Рет қаралды
Subscribe to Dometrain Pro: dometrain.com/dometrain-pro/
Become a Patreon and get special perks: / nickchapsas
Hello, everybody, I'm Nick, and in this episode of Code Cop, I will take a look at terrible code naming practices. Your variables and code, in general, should have clear, long and self-descriptive names, and the advice in this video is everything but that.
Workshops: bit.ly/nickworkshops
Don't forget to comment, like and subscribe :)
Social Media:
Follow me on GitHub: github.com/Elfocrash
Follow me on Twitter: / nickchapsas
Connect on LinkedIn: / nick-chapsas
Keep coding merch: keepcoding.shop
#csharp #dotnet
My students get annoyed at how much time I spend talking about Naming Variables in the intro classes that I teach; but I think it is important to train them from the very beginning so that they don't have to /unlearn/ all these bad habits later.
I'm not sure that this is something that actually registers with students to be honest. IMO this is something you learn when working on large projects with other people in a complex domain.
I would opt to teach people how to think and be good mental analysts of a problem. Building on that will naturally having them questioning their decisions as they develop maturity in their craft. If they're getting annoyed enough that you've noticed, it probably isn't a suitable approach.
Readability is one of the most important things. Let them create an app and then pause the development for 6 months on it or let them work on code from others. They will understand lol.
@@fifty-plus A teacher in their limited time in a class can't magically turn someone into a good critical thinker, that's too idealistic.
I remember my intro to programming class where we had a problem set involving a bagel store. In the program we had to have a variable for the number of bagels. I named it "numberOfBagels". In the same problem set we were supposed to comment our code. I lost points for not commenting my "numberOfBagels" variable. I argued that the name made it clear and adding a comment "// holds the current count of bagels" would have been redundant. In a case of malicious compliance through the remainder of the semester I always added a variable into my code called "numberOfBagels" with a comment explaining what it was doing. Sometimes it was an index variable in a for loop. Sometimes it would be char array. It never represented the number of bagels 😅. To this day I usually include a numberOfBagels variable in my code somewhere, with a comment of course 🤣
🤫 Don't interrupt us feeding bulshit to the next AI training dataset.
Oh yeah?! Soon only AI can understand such code. We dev won't be able to understand code of others, even our own code.
@@vothaisonYou understand your code? Show off
I would use Add instead of Create in this case. Because you pass the object that should be added. The Create would fit more if you passed in some arguments and the method would create an object based on the arguments.
I too would have used add.
Agreed. Or maybe Insert
yes! Create is a method I'd expect to find in a factory, not a repository
I would say it depends, for a plain just add the data sure, but if the method does anything more than just storing it as is, like creating multiple rows or signaling the creation or anything, Create is better in my opinion. Both works, but add in my world of though is simpler than create ;)
@@davidmartensson273 My issue with Create in this case is that you already have an instance of Customer class before. You don't create anything, no matter how many rows you add - you only persist existing model. We could call it Create, if you pass some CreateCustomerRequest - then this method does create a new customer based on some data. But then, it's not a repository
The "repository as a collection of domain objects" advice is common in older DDD related books when ORMs were not very popular. Back then a repository was a collection like abstraction that allowed you to do multiple CUD operations inside a single transaction with the help of a Unit of Work. Repository interfaces are a part of the domain layer. The implementations would have been in the data access layer. The SaveCustomer method should have been called Add though to follow these conventions. These days repositories often abstract ORM contexts that natively follow those principles as well as include custom query methods, etc; and a lot of times they are just another layer of abstraction. ORMs including EF often map their repository-like properties under the plural domain names such as Customers.
If someone can abstract away the details but save the semantics and be consistent, then Repository suffix is just a noise. I am ok with the style.
Bang on Nick. Yes, I still very much use the ~Async suffix as our projects are still far from "everything is async" at this point.
Not everything should be async. Returning task is a huge performance hit. Keep using Async suffix and only return task when you are forced to perform asynchronous operations.
@@7th_CAV_Trooper Async suffix was invented by Microsoft because they already had sync version of the api. It is illegal to have methods Task Save() and void Save(), hence async suffix. If you add Async suffix to every method just because it returns Task then you are introducing code smell into the codebase.
@@markovcduntil the IDE starts coloring methods that return a Task differently to other methods, I respectfully disagree.
The Async suffix immediately conveys an important piece of information at a glance about that code which is removed when you take away the suffix.
@@VanDameDev do add private, public suffixes to your methods, variables?
Totally agree with you and really like this kind of videos from you! Thanks a lot for your efforts! About Async, I'm using it everywhere except cases where in the project no one used it before in async code. With Async code more clear and exactly show what you are working with. Also it helps avoid mistakes sometimes. There was funny story of one person, which doesn't prefer to use Async suffix, spent some time to understand, why code doesn't work as expected. It's because that person didn't see clearly, that Async method was called without await :)
My oppinion -> Long names are good, especially when the context is not clear without looking at the code. And i fully agree, if something is called "Customers" i expect a sort of collection - not something that "saves" stuff. But i dont like the name "Repository", i would rather call it "_customersStorage" as private readonly, injected in the ctor. Good descriptive names also can make documentation obsolete, because with a good name you dont have to guess what this method/class does.
When I'm upgrading programs that I written several years ago, I realize that naming is one of the keys to make maintainable code. My names are long, but as I see it, they are a part of the documentation. Even if everything seemed to be self explaining at the time you were writing a program, it will not be so clear a couple of years later. Or for others... I agree with everything you say in your video. And... I always use the Async suffix. Just for me to know in the future, or for anyone else that will read my code.
In Domain Driven Design (which made the Repository pattern popular), the Repository pattern abstracts the database as a collection. Also, it's recommended to wrap your repository actions in a UnitOfWork, and only save when you have verified all the domain logic. Often, you need to change more than one aggregate in a transaction, so the repository should not commit changes. So, if you are using a UnitOfWork, I would recommend using repository.Add() and repository.Remove(), and not repository.Create(), repository.Save(), repository.Delete(). And since they don't talk with the database, I would likely not make them Async.
Glad I wasn't the only one thinking that. This is literally the original definition and use of the pattern. It's was meant to simplify crud operations by simulating an in memory collection and letting things like UoW manage the persistence and transactions against a repository collection.
None of this is true
@@AndyMehalick elaborate
@AndyMehalick It's hard to have a good discussion with comments like that. I read Eric Evans' big blue DDD book from 2004 twice, and I have been doing DDD for close to 20 years now. I just read my comment again, and I don't see where I state anything as hard facts, except that a repository in DDD a repository abstracts the database as a collection. I would claim that is a fact. The Repository Pattern was first introduced in Martin Fowler's Patterns of Enterprise Application Architecture (PoEAA) from 2003, which says: Page 322: A Repository mediates between the domain and data mapping layers, acting like an "in-memory" domain object "collection". ... Objects can be "added" and "removed" from the Repository, as they can from a simple collection of objects, and the mapping code encapsulated by the Repository will carry out the appropriate operations behind the scenes. But the repository pattern was made popular by DDD 2004: Page 151: A REPOSITORY represents all objects of a certain type as a conceptual set (usually emulated). It acts like a "collection", except with more elaborate querying capability. Objects of the appropriate type are "added" and "removed", and the machinery behind the REPOSITORY "inserts" them or "deletes" them from the database. I feel my comment is very well rooted in the origin of DDD and the repository pattern. So please elaborate on what is not true. Today the Repository pattern is used in many other places, and by definition a pattern is not a definitive, which is why I use the "recommend" instead of "must/should". My "personal recommendation" is still use Add() and Remove() instead of Create(), Save() and Delete(), and wrap it in a Unit of Work (pattern also introduced in PoEAA 2003)
@@thomasjespersen not worth explaining yourself dude , the other guy has no clue why "nothing you said us true" he is just a troll
I love these convention correction videos. Thanks Nick.
You nailed it. Especially this. Some suggestions online is just hunger for spotlight. Don't step on the crap.
Always said this and everybody tried telling me I was wrong because they read somewhere about the typical naming convention. Always bugged me how people can't be practical with their naming so that you can see directly what it is. Instead of having go through all references just to figure it out.... Thanks for this video!
Fully agree with your video and I am also that strict with naming conventions. It makes more sense to say repository and using underscores for private readonly properties. It tells me everything I need to know straight away
At my first job as a developer I always argued with my coworkers over this, one even said naming is not important at all. I always rejected pull requests when i did code reviews for variable, field, property, class names, whether it was a bad name or a typo in the name you are getting your PR rejected. Why? Well not only its clearer whats the purpose of the code, but also having a typo in the name is a huge pain when it comes to class names, example you are searching for a class in the file explorer in a large application with hundreds of files, and sometimes we struggle finding it right even when its right in front of us right? And what do you do, you search it, and what happens when you have a typo? Nothing is found, exactly. Also i always put the Async suffix for async methods. No one should guess if the method is awaitable or not based on context or by compiler warnings and errors
1:36 without further context, I'd assume some sort of static wrapper around a Data Store (maybe even with a cache) that manages all Customer data. Primarily because it has a 'SaveCustomer' function. It's not my preference, but I find it rather minor as far as weird conventions go - these paper cuts do add up though.
I completely agree with your naming version, Nick! Much more clear.
I love this series so much
In fact, in case something is available as Sync and Async, I started to do "Sync" as suffix, because it´s the exception.... usually everyhing is async. Completely on your side with the repo and method naming. Just i have a clash with customer as name :) --> By my own convention, whatever gets returned is called "result".
All my return variables are called result as well
@@ghaf222how I see it: var custumer = reg.ToCustumer() - is correct naming. BUT! CreateAsync(...) should return value, wich will be stored in "result" variable, and then returned by function. It will help when Repository generates some fields automatically, e.g. id
Writing these while watching and before seeing Nick's preferences: I would call the first example "Repository" because I'm in the "CustomerService", so it's clear from context that it's the "Repository of the CustomerService", so it's the repository of whatever the CustomerService needs. If I wasn't in something Customer-related already, I'd call it "CustomerRepository". I would only consider naming it "Customers" if it lived in a class "Repositories" that contains all my repositories (although I never had the need for that approach) but then it's clear that it's a repository from the context (access would be through something like repositories.Customers). On an object called "Customers" I would want to access Customers.Add(item) as I also would assume a collection. However - if that causes a DB entry to be created, I might not want/need to know as this might be implementation specific. The second (CustomerRegistration) I would call CustomerService.Request (being a nested class inside the CustomerService). I don't like the async suffix, but I'm used to having to use it as it's mandatory code-style in the projects I'm working it, so I adapted using it in my private projects too.
Something I learned very soon and very fast when i began to work 25 years ago : Name variable, fields, properties to reflect what they are Name methods to reflect what they do. By the time, I worked with someone who named its variables a,b,c , i, j, x, y, z... and used it in the whole module (we were programming in VBA). That's how I understood what code maintenance was, and I figured out very soon how good guidelines were protecting your code.
People who name their variables with one letter are clinically insane. Unless it's a standardized letter like x,y,z positions, r,g,b colors, i,j,k loops etc. then there is no excuse.
it's pretty common to use the acronyms as variables names when using them in a small scope, e.g. `var sb = new StringBuilder())`, `using(var bw = new BinaryWriter(...))`, `var s = someObject.ToString()`, etc.
@@billy65bob if the scope fits well within the screen yes I agree. But having worked on projects that have lived for over a decade, I know things change and small methods/loops, tend to grow, and suddenly you are refactoring, cutting and pasting, and woupsie, you have unintuitive variable names spreading. I still use short names, but I try to avoid it. Its so easy to use "i" for index, but then someone adds another loop around and you could get subtle bugs.
@@davidmartensson273 in that case, being a no nester helps a lot.
@@NihongoWakannai that's cases where the variables are well named. i and j for iterators are a less obvious, unless for general purpose functions (like sort algorithms or things like that).
I love using the "Async" suffix. Sometimes, I think, it is not really clear if the thing that you are calling should be awaited or not. "Async" makes it clear. Also its common practice in Microsofts APIs. I definitely have had lots of cases where colleagues (or myself) have fogotten to await something, because the naming didn't make it very clear. That is why we have the "Async" suffix in our guidelines now :)
I pretty much always use the async suffix, there is no project where literally everything is async, except maybe a vending machine. I think it always promotes clarity. If there is ever a case where removing it makes sense, that can be decided up mutually by contributors
3 hard things in CS: off-by-one-errors naming things :-)
Yeah, joke (and T-shirt) in my circle goes: Most developers struggle with two things: 0: Cache invalidating 1: Naming things 2: Off by-one-errors
I have learned it as: 2 hard things - naming things - cache invalidation - off by one errors (and for anyone not picking up, the 2 things is a direct ref to the off by one)
Like several others here, I generally like Save vs Add/Create and Update. It simplifies external interactions when I don't have to explicitly determine if this is an insert or update and call the appropriate repository method. If possible, I like to leave that distinction to the repository itself. For example, the repository can tell that the record being saved is an INSERT if the ID = 0 and the record doesn't violate any unique constraints. Even if you want to differentiate actions like RegisterCustomer from UpdateCustomer in your service class, the repository itself doesn't need to differentiate between the two operations. As long as the repository fails correctly on an illegal operation (e.g. violating a constraint) the service can appropriately respond.
I agree as well. Same with the HttpVerbs. I believe those are outdated as well with having WebAPI 2.0 now. I like using routing so much more because they are (if used correctly) can be much more informative due to using the naming then using the GET/POST/PUT/PATCH/DELETE etc. In my private projects, all my endpoints are post now. In work cases, I simply follow the path that is taken in the project or is normal for the company. But I do hope one day the HttpVerbs are gone. I just fear I might not be living long enough to see that day though which makes me sad.
That "Clean Code Tip" is basically saying to stop naming things based on what they actually are. It's a Repository (which is a specific entity that serves a specific architectural role in the codebase). Naming it as such removes all ambiguity.
Nick, I absolutely agree with your point, For this specific example it is better to call it on what it is within the Class/Service Context because it is relevant to the implementation details of this Class/Service. Having something that is named like a collection to hide the Metadata of what the service is, gives less benefit to the reader of the code than including the metadata e.g. *Repository. Additionally, I am one to advocate for removal of the async post fix as that does not provide any beneficial metadata. Also pre-fixing class fields with an underscore does also not provide any beneficial metadata as the Pascal casing provides metadata stating that the used identifier is local to the method or class. By my opinion on the usage of an underscore It provides more static in your code that makes it less readable compared to the benefits of the metadata it provides.
There is so much bad advice on LinkedIn, Twitter, and some blog posts. As developers, we use LinkedIn and Twitter, and we follow blogs that have programming content. Content creators should have proper knowledge to write content. I think many people just copy AI-generated code. It's bad. Lastly, I love watching Code Cop, but how many videos will you create? Nonetheless, Your Code Cop series is one of the best on KZhead. Love it! Please continue this series for as long as you find those type of topics. Thanks a lot for your awesome contribution!
1. Naming is most important for readable code - I'd just have named evrything just as you did. Maybe only using "_repository" because we're in a CustomerService and "single responsibility" should kick in (maybe not considering the LinkedIn guy...). 2. I don't use "Async" any more, because nearly everything is async nowadays... Exceptions are classes/libraries that should implement an async method version besides a sync version of a method (e.g. some industrial machine communications (maybe old school ones) that work like dialogs over blocking hardware interfaces. Where you have to do things like direct communication and message queues at the same time (when not split the library in an async and sync version)) Thanks for the video ✌️
Also for the Customers.Save(customer) I totally agree, that is more like if you were commiting a transaction in a dbset. And adding a parameter makes it confusing not because you wonder what they are saving but its confusing because at least myself, I wonder how they are saving, what they are saving from that thing. And for the Get, definetly a GetById makes things more clear when reading code since there are more than one way to get an object in most of the cases I work on.
I use the async suffix. Makes it more clear. The naming you suggest is great, use this kind of naming too.
8:29 Just wonder; for me 'Save' means exactly to save incoming data 1:1, and 'Create' implies there's a possibility of data modification. Like, when you save your file, or game progress you'd expect to have exactly the same data upon loading them back, while 'Create' creates data from something and could alter the end result to fit some kind of requirements.
I like the async suffix, I don't always await tasks but return them and it's nice to know what is happening without having to look at the above method signature
Returning not nullable Customer seems okay to me. I'd assume in this context if a validation exception is thrown there is some middleware to return a nice response, or if an unforeseen problem occurs during database update just let it return Internal Server Error.
yeah, returning null with no message is misleading
I love this phrasing "piece of advise"
The repository variable: 1) you can go to definition with 1 shortcut, and return to the previous position with 1 other shortcut; time taken: 0.5 seconds -- which could be avoidable if variable name contained "repository", but still... Second point: you can move the mouse cursor on the variable and it will popup the type, or you can open the popup menu of the autocompletion and it will tell you the type. So sure it could be better, but in this specific case i find it's not that bad.
I still use the Async suffix. The model of execution for an async method is so different that it's worth calling out everywhere. Also, it's nice to leave room for a non-async name/implementation.
absolutely agree. be as clear as possible in code.. don't leave it open to interpretation
I believe that I'd use repository keyword with the obj name if it is part of that particular domain only... I might choose to abstract the repository layer as if i am building a framework for multiple projects thhat have a separate repository layers... which in turn are calling this framework to execute some second operations before their operations... as at the end they r also working the same thing just from a different context... also helps us realise that these r not originally part of the domain that we r working on. And could be used if someone is building a library or a package. I believe that using just customers is kind of abstraction of what the object could be doing... and makes the rest of the code more readable. Thats like uk lists... we dont name list objects like customerlist or departmentlist. Im sorry if i wasnt clear with my words. Not a native english speaker @NickChapsas
I'm on the fence about this one, although I would generally lean towards CustomerRepository (or customerRepository if it's a private readonly field) 1. "You can split your repository into different responsibilities, so if you have a product, you might have a product catalog, you might have a basket, you might have a pricing repository..." The author's naming convention applies here too. Instead of "Products", they might opt for ProductCatalog (or ProductCatalogue if they're British), ProductBasket, ProductPricing, etc. however, ProductCatalog(ue)Repository, ProductBasketRepository and ProductPricingRepository are clearly more explicit, as you suggested: "I like to be explicit with my naming" 2. "Customers.Save is crystal clear, assuming that Customers is something like a DbSet" Perhaps you meant DbContext (which has a SaveChanges(Async) method), rather than DbSet (which doesn't)? Personally I think this lacks clarity, because I can't tell whether "Save" here means "add/update", or "commit to the database"...maybe both!? (you somewhat covered this in the video when you went on to rename the method "Create"). If they mean "commit to the database", then they have muddied the responsibilities of the repository. Database commits should be a responsibility of a unit-of-work, not a repository. Database commits from a DbContext/unit-of-work ensure that all changes are coordinated and applied as a single atomic action, maintaining data integrity and consistency, much of which is lost the moment you allow every repository to commit its own data independently. For smaller projects it's probably sufficient to have repositories that call SaveChanges(Async), but I think it's still worth mentioning that DbSet and DbContext are akin to Repository and Unit-Of-Work, broadly speaking.
As for the Async suffix, I only bother with it if I'm introducing Async methods into a project that was inherently synchronous to begin with. Everything new that assumes Async by default, I don't bother. It's just noise when you need to await nearly everything anyway, and the bulk of your remaining 'synchronous' ones call the async method to return its respective Task anyway. And while you'll still have occasional synchronous methods, they can be easily and quickly made async where necessary. Only exception I'd make is for a library, in where I'm following microsoft's example to produce both async and non-async copies of all methods.
“Because isn’t it our goal to represent a stored collection of customers?” => that’s where his mistake is. Not all collections are to be treated equally, there’s a fundamental difference in the expected behaviour of adding something to an in-memory collection vs a persisted collection. Trying to abstract away the fact that you’re persisting is the same pitfall that led people RPC stuff like CORBA. Adding to an in-memory collections is fast and generally faultless; adding to a persisted collection is slow, introduces variable latency, confronts you with network and other I/O hiccups and suddenly introduces the need for fault tolerance.
So true, you should be able to understand if there are side effects or slower code. Its so sad when you find something that looks like just a local property access, like var x = myobject.MyProperty, and then find out that it fetches from a database everytime you access it. Adding the repository name is a clearer indication that this is not a local list or similar but an interface to some external data source, does not need to be a database.
@@davidmartensson273 I have bad news for you guys: performance concerns should not be mixed with the business problem being solved. If you gotta add the user, you gotta add the user, if you gotta fetch the user, you gotta fetch the user, if you gotta delete the user, you gotta delete the user, how fast it's gonna be depends what implementation you're injecting into your algorithm. Sometimes a database indexed search can be faster than an in memory List scan, depending on your list size and matching criteria.
Totally agree. Newcomers should not be allowed to share what they believe to be true. You are totally right to shame and point fingers. I hope they learned something today I'm not sure it's enough for them to understand how friendly our community can be, if only they provided the right advices.
This gatekeeping "were smarter than everyone else" mindset is why so many non developers hate developers and see is as arrogant nerds. The whole "newcomers" and "shaming" thing is cringe as hell man.
I prefer the Async suffix on awaitable methods. (I left it out for sometime but felt really terrible for doing so)
I think the …Async suffix convention makes the most sense when there is an API that contains both an async and non-async method that do the same thing. For example, with entity framework and LINQ. When in the broader codebase of an application written as async-first, the …Async suffix is redundant. The IDE generally provides sufficient warning when accidentally calling an Async method without await, so I feel no special convention is needed. The convention is a remnant of Microsoft broadly adding Async methods to a pre-existing legacy codebase with an emphasis on preserving backward compatibility.
In this situation, I would have just named my Customer Repository variable as _repo or _repository. Basically, the repository is being called from the Customer Service, so the default default repo would be related to the Customer. If I need to call other repos within the service, then those would have the extra qualifier, such as _companyRepo. I know it's bad form to abbreviate my variable names, but for me, repo is always short for repository.
I do agree, as long as the abbreviation is clear enough and commonly used in the code base.
I actually don't mind calling a repository by the plural name of the entities. The idea is that the fact that this collection is backed by a persistence layer is immaterial to its usage. It shouldn't matter to a consumer whether an interface for a collection of entities where you can query, add, remove, and change entities is implemented with an in-memory storage or a database-storage. A function that determines which entities to add to collection and then adds them shouldn't care if or how the collection is durably persisted. Its job is only the interaction with the collection. A function that queries entities from a collection according to some logic and processes them similarly shouldn't care where the collection gets the entities from. Even transactional interactions aren't an issue, because transactional safety is a matter of whether the collection has shared write-access, independently of whether it has an attached persistence layer. I don't mind naming repositories "[...]Repository" either - but I get the argument that it pollutes the "story" told by the code with implementation details which *should* be irrelevant to consumers. A unit of code (function/method/module/class) should IMO only mention things it is concerned with - and eliminate naming which is there to indicate stuff that is beyond its scope. So I ask myself - "Is it within the scope of the knowledge represented in this function whether the collection it operates on is backed by a persistence layer?"... and I found that the answer is very rarely "yes".
THIS
Beginner here: I use the async suffix because sometimes there are now functions which aren't async but in the future they will be
I hope u don't use async suffix if the method isn't async, u should never future-proof by guessing how the code might look in the future, u will create spaghetti code
If you never use async as suffix you can't see that an old function isn't awaitable.
@@mslucass its not about never using async as a suffix or not. Its about future proofing it what you said "i always use it, if its not async now it might be in the future". If its not async now based on context if im using it i will await it which will result in a compiler error. So if its not async dont use it. But if the day comes when the method needs to be async then you might need to think about how will you approach it.
If I am defining an interface I’ll generally make return types Task and the method names will have the Async suffix. However I’d never otherwise suffix a method Async if it wasn’t. If you need to change it in the future just rename it then!
@@ghaf222 well changing the method logic to make it async and renaming is not the best idea honestly. It is a disaster ready to happen, i would much rather create a new async method that does the same thing, then change existing method which is used througout the application and break everything
Not using the 'Async' suffix as it feels redundant
Nick I heared that we should avoid Enum in the domain layer, and use record instead of that, do you agree? is it really a good practice
I agree, customersRepository is the way to go. On the other hand, my first guess on Customers was some sort of collection. Second guess was repository. I don't know why, maybe I'm the stupid one, but I would consider it :D
It was definitely either of those, unless it was very poorly named. Neither of those were outliers.
I use the Async suffix because no, all methods are not async, most yes, but not all, and many methods will never be, only those where it makes sense I know, I am using the word "never" a bit loosely 😊 If a method is not yet async but u intend to make it async later, do not add the async suffix until it is
Looking at methods use, Customers is thought like some DB wraper around SQL, but name sugest it is DB Set or collection, so you are right, i don't know if i like Repository, rather Service with such methods
Im fine with a field named "repository" if there is only one injected but to name it "customers" is just weird. I also never saw someone naming a repo like "customers" in my 15 years of dev lol.
Even if its one i dont recomend naming the field repository or service or whatever. The reason being if its now one it doesnt mean it will stay that way and tomorow we might need to add another one and rename the old one. In cases like this i justify future proofing.
Not using `*Async` suffix (in own apps) - it's noise. In contrary, if I shared library - which could easily happen - I would use it since it's how convention is set in .NET universe.
Actually I use Async at the end for every async method. But I would prefere to use Async suffix only if there is 2 different implementation, one async and one sync.
I'm hesitant to add "type suffixes" like "Repository" on the end of a var name, but I understand the thinking, and I wouldn't make a big deal if that was the pattern I inherited in a code base. And I actually like the "Save" semantics vs explicit Create and Update. Those methods have a place, but the branching logic between them is boilerplate IMO, and there should be a higher level Save method that picks the right approach (insert or update) based on the entity state.
I would love to see a UI course for .NET. Maybe about how to do and maintain FluentUI in a Blazor web app properly. Do you know someone that specializes in front-end blazor Nick?
"Spoiler, this is actually a Customer repository, which no one should have guessed." LOL I thought it was obvious from the usage and the context. It should have been everyone's _first_ guess. I agree that the naming convention is a bit unconventional, but I think this is being a bit too picky. Also, one of the original definitions of "repository", from Martin Fowler in his book on enterprise architecture patterns, is "A Repository mediates between the domain and data mapping layers, acting like an in-memory domain object collection." While it's more common to name the repository with "repository" in the name, I don't think it's unreasonable to name it like a collection. The most important thing to me is consistency throughout the codebase.
We use async in our 4.7 Framework coodebase only. Everything in core and later is withou async suffix.
It's good advice iff Customers is defined as a List, just like you said. Repo objects would contain code for "Get(string parameter)"? That's kinda cool!
The Async suffix is bloody essential. It tells you straight up that you need (at some point) to await it. Not every method needs to be async, so let's keep that clarity. Please.
Hey Nick, what do you think about repository pattern, do you like it? Or do you prefer injecting the DbContext into the service directly? My first API to go to production used the repo pattern, but now a days i'm kind abandoning it.
I know you asked Nick but I'd like to add I like to abstract the DbContext but not the Repositories individually, since it's all about the context. In this video example you'd have an IAppContext with an IQueryable Customers, then you implement this interface with your DbSetContext and inject it. Accessing it in code would look like "this.context.Customers", see? no "Repository" word and the Context can be implemented using memory (ram), file system or external databases. If not this then I prefer injecting DbContext directly.
I'm totally agree with you naming preferences Nick! Regarding Get methods, by the way, considering the name of the param sent as well (id, email...) I would then just name it Get taking advantage of the method overloading, unless there's a type coincidence, so then I would rename them as you suggested. About Async suffix, in a perfect world I would prefer not to have them, however ir creates confusion when you have a mix ot naming conventions: has the dev forgot to add it? Is it then an async but not awaited invocation? I think imho that in the real world, having it avoids misunderstandings and confusions... Again, Great video btw as always!
But if it accepts a string, how will you know if it fetches on email, or name, or customer number (in case that is alphanumeric) or ... If you use a plain Get and depend on overloading, the arguments should be domain objects so Get(EmailAddress email) or Get(FirstName firstname) and so on. Otherwise its ambivalent and a very likely source of problems.
@@davidmartensson273 that's true and why I've said that only if there is no conflict/collision between arguments (since it's not technically possible) If I have a Get(Guid id) and a Get(string email) I see no problem nor misunderstanding when using them. If on the other hand you need to filter by email or name then I would have something like GetByName(string name) and GetByEmail(string email)
@@xelit3I would argue that as soon as the argument is of a standard primitive time you should have a more descriptive name of the method. But sure, if its obvious from the context that a string has be be an email I might let it pass a PR ;)
I use the Async suffix only when I have the non-Async method in the same class!
We like using Async when naming methods as we work with a whole swathe of applications, spanning multiple years with some not using asynchronous calls. So when something is asynchronous we call it so.
I don't use the Aync Suffix unless I have a non-async version of the same method that I can't get rid of. For me, the fact that the method returns a Task, is good enough to communicate that it is async.
Is there a reason you don't use primary constructors? I find them awesome for dependency injection as it's only 1 line per dependency.
I personally never use them for dependency injection because I prefer having them readonly. Also because private fields have white text whereas a variable in a primary area is light blue so can be mixed up with a local variable
I guess my name is "no one" because when I saw that I thought, it's probably a repository. :)
Naming is hard and individualistic. I like to try to write things as self-documenting code utilizing Intellisense and to be short since I've got to type and read them. I don't like variable names to be the same as the class names. What information does "CustomerRepository customerRepository = new CustomerRepository();" give? That's also why I like the new simplifications of new(). 😁
I loved the code cop moment 3:55 haha
Abbrevehain is a cardinal sin in computer programming unless they are widely understood acronyms or initialisms (HTML, HTTP, etc) 9/10 times naming variables is easy. There are only a couple names, mostly driven by the type. Naming types is obviously the most difficult. I think that if you ask yourself “is this a good name?” you'll come out fine.
I dont like the async suffix even we have to use it if there is only a async version of a method. There is also the cancellationtoken missing if you make it async
Using Async because it means the Task has started. Without the Async there is no convention that the returned Task is an already running Task, and that may lead the await to wait indefinitely.
I'm asking myself if they belive this bs themselves... As I read the method I thought: dafaq does it? I couldn't figure out what Customers shoudl be, wtf is Save, and why are you using async methods without the postfix of Async... The Validate method, I dunno, I would make it a bool, and then a if block around to handle the case of an invalid object, and also if an object is nullable and everything... Everything about this advise and the code itself made me cry... And yeah, I use the Async postfix for async methods since then it's clear you have to use await or have to deal with whatever. Sure, I can miss it sometimes but the AsyncApostle helps me to remind me to add it. ^^
I actually like SaveCustomer, if it represents both Insert and Update operation, commiting object to DB. depending on whether key is assigned.
CustomerRepo > Customers. We don’t live in the world of 40 column displays and intellisense of some kind is a standard in modern tooling. Names should always be descriptive over short.
Does `CreateAsync` change Id property of its argument? I expect it to be more unusual then naming.
I perfer to ommit the Async most of the time, feels redundant and rarely adds value. If I miss an await I'll get a analyzer yelling at me anyways. But yeah, I'm with Nick on this naming thing, and we adhere quite closely to the modern C# guidelines if not for consistency. And for that validation, that should be done in the API handling earlier, because what's this going to do now if it fails the validation? Thrown an exception, and that's.... yeah, I don't like that approach anymore after having used it.
If I saw some variable starting from capital letter and the name is in plural form, I would assume that it is a property and a collection, not a repository. If I wanted an entity to be repository, I would explicitly named a property of a field "BlablablaRepository".
why the Validate method is async? does it call something external that needs to be awaited?
It might. But if it is it should return any problems since exceptions in async code is more complex, especially since the caller could have to dig down into the captured error. I try to avoid using exceptions for anything I expect to happen, so if the Validate for example checks common mistakes that a user could have done entering data, I would not want that to be exceptions since its something that will happen quite regularly I assume. Exceptions are for exceptional things, things you know can happen but that you think will not happen very often.
I don't understand why a customer repository is not a collection of customers as to me it is. It's not an array or set of customers but a repository is also a form a collection to me. Okay, maybe some C# thing I don't fully understand, maybe repository has a different meaning in C#. But a general rule for good naming is to not duplicate type information in names. So just as you would not name an array of customers CustomerArray and the name of customer would also not be CustomerNameString (here you could also argue, how would you know it's a string without String in the name), you would also not name a repository of customers CutomerRepository. You also should not use names like Manager, Service, Controller, etc. unless those have a special meaning in your programming language of the system API you are programming for, as every class that manages or controls something is a controller/manager, that adds nothing useful to the name. Names should be useful and represent the purpose and naming an object that represents your entire customer base customers, doesn't seem too far fetched.
I know this is all very subjective to some, but I hear you Nick. What I would do here: - Customers -> I would use this only if it was a collection type. I see this I am expecting a list or an array at the outset. That also means I am not expecting any enhanced state methods or extensions one would get from a more complex object like a repository. - _customerRepository or _repository for the repo. The first would be for clarity in complex cases where it's not clear what my repo represents - esp with the cases you mentioned in the video. I only use "_repository" if it's a super class with a single repository in the pattern - making it clear that it can only mean one thing. Either way, it's a private member and I name it _lowerCaseName. - Method naming on your repository to me completely depends on if you can control it. If it's coming from EF you likely have no say there, but if you do I agree that SaveCustomer is redundant when you already know what it's for. It would be "_customerRepository.Save(customer)". And for the other methods, I agree with Get, GetById, etc. It comes down to what you can control. Most time I see redundancies in naming due to generated methods. Yes on "Async" at the end of a method name. It tells me immediately that I am getting back a Task rather than the expected type and that I will need to await the result. Not all methods are asynchronous and shouldn't be assumed to be so because asynchronous operations cannot be used in all cases.
“It’s a CustomerRepository, which no one should have guessed”. I guess I’m no one. Or maybe I’ve just been around the block a time or 10.
Always use "...Async" .. it makes sense to be explicit
I prefer overloading to using ByParam in the name. The parameter names are plenty enough context. Down with verbosity! Also, registration > customerRegistration. We’re in the CustomerService component, so that it is a customer’s registration is completely implied. I’d also name a CustomerRepository dependency just "repo"
I would argue adding the `-Async'` suffix is useless unless the method has a sync counterpart. It would be similar to adding `-Void` and `-Task`, and that's just visual clutter; it takes away from providing actually descriptive names and adds characters for your brain to parse. It may be helpful if you're programming outside of an IDE, but why would you write C# outside of an IDE? As with most things, it comes down to preference rather than what's objectively best.
I always use the Asynchrony suffix because it just fits in line with Microsoft’s guidelines. If I am building something for use in .NET, then I want it to feel like it fits in well with what the guidelines for building for that framework actually are. Microsoft uses it in the BCL and elsewhere, so why wouldn’t I?
I prefer it when names tell you what it is so you don't have to guess or peek around the codebase for its definition.
Nick, you should ask yourself why you need Repository suffix in the first place.
I think I agree with the part of naming explicitly a CustomerRepository inside a CustomerService but I believe there are cases where explicitly sounds weird. Maybe you were refering specifically to the repository inside service but my example is for example the Customer class model having a CustomerId, that just feels too repetitive to have explicit Customer.CustomerId.
Ignore this comment, you totally made it clear in the _customerRepository.CustomerCreate() should be _customerRepository.Create(). It is implied but I leave the comment if someone didn't watch all the video and read the comments, which I believe cant happen because code cop is the best series ever
The problem with naming variables is that it's subjective, what is obvious for you, is not the case for another. For myself, I much prefer writing code that can be verified easily, as the name will inevitably be misleading. What I dislike about the above code is the CustomerRegistration able to create a Customer via ToCustomer() that function is exceptionally misleading on what the intent is. I'd much rather have the service provide the ToCustomer function and CustomerRegistration is passed in.
i only use ...Async if there are other methods in the projects that is not async. ... on that note, i can't remember when last i created any method that is not async :)
The post seems to be made by a dev that has only worked in his code base. And for the naming I usually use Async as a suffix, however if its a Web API I don't think it's necessary as most of its actions would be asynchronous, however if it's like a console, desktop, etc I think it's best to have the suffix
I prefer writing Async suffix, it is explicit that you need await here, as there are still many synchronous methods everywhere
You should not inject the repositories directly anyway, you inject the Context (be it dbcontext or not) and then access like context.Customers, with "Customers" being only a container with a collection-like interface (IQueryable preferably) and the context being the abstraction for where the data is at (in memory, filesystem, database...)
Thumbs up for "Naming is extremly hard!" in the first 30sec (still struggling...). My first assumption was, it must be an Repo, no other things does make sense. I use ...Async
We all know the best way to name variables is to start with the letter A, then the next variable is B, etc etc. Once you get to Z, the next one after that is AA, then AB, etc etc. 😂 /s
Imho the issue starts "above". It makes sense for the application to have a class Customers which aggregates everything. That class has a CustomerService, which actually does the work. CustomerService has a CustomerRepository where it stores the work. Then Customers.Add(customer) would make total sense, even if that is just an indirection to CustomerService.Add(customer), which in itself is an indirection to CustomerRepository.Add(customer), which is... etc.
IList or ICollection have all the crud methods you need, so how is a repo anything more than ICollection impl that writes to a persistent store instead of ram?
Those interfaces are inappropriate to use in this case for a few reasons. - Some actions may be non-synchronous (async) - IList has int indexer, the data may not be indexable at all or at least not by an int. - ICollection has enumerator, the data may not be enumerable, the API contract is also synchronous and if the data were numerable you'd want something like an IAsyncEnumerable instead. You would not want a db call on every enumeration step anyways.
Because IList and ICollection *are* stores for items. Repositories *do* store items. You use repositories to access an arbitrary storage, but it is not the storage itself
@@Sindrijo you're absolutely correct. Those are the points I wanted Nick to make.
@@EikeSchwass oh, I like that explanation a lot.
@@7th_CAV_Trooper You should be using IQueryable, since IEnumerable, IList, ICollection and even IAsyncEnumerable will fetch all data into memory before doing any processing, while when using IQueryable you end fetching only the final result of the processing (filters, order bys, groupings etc), since they are all done remotely (in case of a database).
The only case, I'd accept a repository to be called simply "Customers" is withing Unit of Work