1

I'm kind of new to this "separate your methods from models" (yes I was one of those developers). From what I understand I have to separate everything into models, services and factories. A factory creates stuff (where every "new" keyword of your project should be) which is straight forward, but a service takes something and does something to it, so I'm a bit lost in how to name everything.

I'll try to explain with a example:

I have a "shoppingcart" object and I want to add a "product" object to this shoppingcart, which will result in a "shoppingcartproduct" object. There are a few rules about adding a product so there is quite a lot of logic needed to add the product.

In my controller I put something like this (it's c# mvc):

[Authorize]
public ActionResult AddProduct(long id)
{
    db = db ?? ApplicationDbContextFactory.Create(HttpContext);

    var product = ProductServices.GetProduct(db, id);
    if (product == null)
        return new HttpStatusCodeResult(HttpStatusCode.BadRequest);

    var user = UserServices.GetUser(HttpContext);
    if (user == null)
        return new HttpStatusCodeResult(HttpStatusCode.BadRequest);

    var shoppingcart = ShoppingCartServices.GetCurrentShoppingCart(user);
    if (shoppingcart == null)
        return new HttpStatusCodeResult(HttpStatusCode.BadRequest);

    var shoppingcartproduct = ShoppingCartServices.AddProductToShoppingCart(db, shoppingcart, product, user);

    // if cannot add product due to incorrect membership
    if (shoppingcartproduct == null)
        return RedirectToAction("Index", "Memberships", new { ProductId = id });
        
    // product added correctly, show the view
    return View(shoppingcartproduct);
}

As you can see I've now chosen the approach of bundling my methods by the name of the object that the method works with the most (usually the return object, but not always). This at first seemed logic, but I'm ending up taking a lot of time finding the methods when debugging.

There are a few approaches how to solve this:

  1. My current method: bundling by name of most used object.
  2. Bundling by name of the return object.
  3. Not bundling at all; make one method per (static)class and use a naming scheme for the class that represents the action it performs. Like GetProductTask.Run, AddProductToShoppingTask.Run ect.
  4. ???

Can somebody explain to me which method has proven to be the best approach for them (idk if this is standardized in any way)? Or if my understanding of this is maybe still wrong?

Btw, I know I should not use statics for all my services, but as this is a conversion project of my "old way of programming", I choose to make everything static to not have to spend too much time on converting everything.

7
  • 1
    "(where every "new" keyword of your project should be)" Not really. Factories are not the blanket replacement for constructors. They are used when construction is more complex than the consumer would like. That falls into two categories: (1) Making the decision which of many concrete types to instantiate or (2) A complex construction process for a given concrete type. But by no means should a codebase replace all of its instantiation with factories (arguably, unless you include a DI container under the "factories" moniker, and even then it's not 100%) – Flater Apr 15 at 9:47
  • 1
    'separate your methods from models' in an OO paradigm is sometimes referred to as an Anemic Domain Model and is considered an anti-pattern. – Rik D Apr 15 at 10:00
  • @RikD So It's wrong? And my old pattern of doing a "shoppingcart.AddProduct" / using Domain Models was not wrong? I've had a lot of backlash over my usage of methods inside models/entities, also by watching videos of Shiv Kumar and IAmTimCorey on youtube I got this idea. So this gave me the idea to change my habits. Martin Fowler was a person I usually referred to when defending my style of programming. – ikwillem Apr 15 at 10:19
  • @Flater I understand, as you can see in my code example I'm not limiting myself in using the "new" keyword. But by trying to do this wil all of my database models, it did help me improve on code duplication and helped me find stuff faster. Currently I'm not making my factories mockeble, just use static classes as I don't really feel it's needed with unit-testing. – ikwillem Apr 15 at 10:24
  • 1
    @ikwillem I'm not Rik and I won't speak for him, but I think it's safe to say that you haven't quite grasped OOP perfectly, and your imperfect understanding is causing you to label things incorrectly from time to time, making it very hard to give you an accurate answer without misapplying it. You are conflating the meanings of words like object/model/entity, which lies at the root of misunderstanding the guidelines you've been given. I'm already writing an answer, but in very short: anemic models are generally considered an anti-pattern, but datalayer entities are not the focus of that advice. – Flater Apr 15 at 10:27
2

I have a "shoppingcart" object and I want to add a "product" object to this shoppingcart, which will result in a "shoppingcartproduct" object.

Adding something to something else (in the non-mathematical sense) doesn't produce a new thing, it alters the existing thing (shopping cart) to now contain the thing (product) that it didn't contain before.

I suspect you may be thinking that a "shoppingcartproduct" should be created because it's the entry of the cross table between the shopping cart and product tables in the database. You are correct that (based on most webshop implementations) your database should be structured like that and thus create a new entry in the cross table when adding a product, but your business logic should be separate from your data persistence.

Your code is crossing a lot of boundaries that it shouldn't. You have your persistence (= data), business and endpoint logic all mashed into the same controller method, which is too much responsibility for one class (let alone method!).

This is making it hard to advocate for actual good practice given the lack of clean architecture. If I were handed your codebase, I would first dedicate my time towards properly separating the persistence, business and web logic into separate project.

But I'll let the rest of the answer focus on the question you've asked. Just keep in mind that when I present a solution, that it is not the final solution that cleans up your code, it's just an example of the specific issue your question focuses on.

If you have the time and effort, I strongly suggest doing an official (or at least lengthier) course on the basics of OOP. It will pay back dividends in the long run. This answer is only battlefield triage compared to what a course or tutorial can provide for you.


Don't get me wrong, I see the effort you're putting in and I applaud it. You're just struggling to get it right. Don't let my criticism get you down. You're on the path, just not at the finish yet. But in order to get things right, you must first understand the things you're doing wrong, hence my answer focusing on the mistakes you've made.


"Bundling"

As you can see I've now chosen the approach of bundling my methods by the name of the object that the method works with the most (usually the return object, but not always). This at first seemed logic, but I'm ending up taking a lot of time finding the methods when debugging.

There are a few approaches how to solve this:

  1. My current method: bundling by name of most used object.
  2. Bundling by name of the return object.
  3. Not bundling at all; make one method per (static)class and use a naming scheme for the class that represents the action it performs. Like GetProductTask.Run, AddProductToShoppingTask.Run ect.
  4. ???

The fact that you refer to it as "bundling" suggests that you are conceptually still thinking of these as separate things, but you are now begrudgingly putting them together because of a good practice guideline you learned.

Doubly so because you are struggling to find the methods after you've bundled them, which suggests either that (a) your bundling is bad or (b) you're not comfortable with the clean structure that you've created. And if (b) is true, then I'm going to say that it's very likely that you didn't bundle things perfectly either.

Naming

Your question is on naming the things you've created, but the underlying issue is actually that what you're creating isn't quite right. So the main focus here shouldn't be on naming this thing, but rather on improving it.

Names should be descriptive, not prescriptive. By that I mean that it's more important for you to first conceptualize what the class' responsibility and interface is, before you decide what its name should be.

In order for your naming to be intuitive, you need to first have a good picture of the thing you're going to name, because otherwise the name will not reflect what it actually is.

Therefore, this answer mostly focuses on fixing the structuring of your classes, before delving into how to naming well-made classes.

Global scope

make one method per (static)class

I urge you to step away from static classes. What you've done so far (based on the capitalized names, since you didn't post any concrete classes) seems to be making all your logic static, which is not the way to go.

Because you've done this, it feels to me like you've originally written one big monolith method, and then started arbitrarily breaking it up and shoving it into arbitrarily decided static classes, just to conform to the letter of the clean coding guideline without really observing the spirit.

Static classes should be few and far between as far as your main codebase is concerned. C# is an object-oriented language, and you should leverage the benefits of OOP.

There are many things to improve from this example, but at the most rudimentary level, your code needs to instantiate its services, e.g.:

var productService = new ProductService();
var product = productService.GetById(productId);

Keep in mind that it's very uncommon for services to be instantiated on the line above them being used, but this is a massive topic in and of itself. This example is a barebones example meant to showcase how OOP is different from static methods.

Objects vs entities

  1. My current method: bundling by name of most used object.
  2. Bundling by name of the return object.

You keep referring to "objects", but you clearly seem to focus on your entities. It's very important that you understand the difference here, because it's (unsurprisingly) the core of object-oriented programming.

Entities are your data objects, i.e. what you store in the database. In your case: User, ShoppingCart, Product, ShoppingCartProduct. Objects are any instance of a class. Looking back at the previous code example, productService is an object. Your entities are object (when instantiated), but so are many other things.

It's important to stop thinking of your logic as existing solely in service of your entity. Like you said yourself:

"separate your methods from models"

Part of that separation means that your logic needs to live its own life, and be defined by its own worth.

It's actually hard to pick are your specific rules 1 and 2, because they are technically correct in a lot of cases, but you're thinking of the wrong reasons. We don't call ProductService that name because it's "the Product logic bundle". We call it that because it is a service, and since your code will have many different services, we point out that this service is the service that focuses on product-related logic.

The difference here is that -Service is the most important part of the name, and Product- is just a subcategory to distinguish it from other services.

I advocated proper layer separation earlier. I want to bring that back up, to show you how the naming tends to work:

  • ProductController: A controller handles the web requests, in this case product-related web requests.
  • ProductService: A service acts as the main source of business logic, in this case product-related business logic.
  • ProductRepository: A repository acts as the codewise representation of the (usually external to the runtime) data storage. In this case, it focuses on storing and retrieving products.

Notice how my description barely touched on the fact that a Product is involved with these classes. The main focus of the description is what it does.

To use a real world example, your manager isn't called your manager because that's what superiors are called. He's called a manager because he manages his staff. Their job name is based on their job purpose, not what they manage.

Separated chunks of logic

  1. Not bundling at all; make one method per (static)class and use a naming scheme for the class that represents the action it performs. Like GetProductTask.Run, AddProductToShoppingTask.Run ect.

While I strongly suggest you familiarize yourself with OOP before delving in to this, the approach you're hinting at here suggests that you'd be interested to learn about a combination of CQRS and the mediator pattern.

General advice

This answer cannot cover every question you have, as you have significant gaps in several key concepts of modern day OOP practices.

But, on the bright side, you do seem to be motivated and willing to learn, and that is more than half of the battle is already won.

Based on where you are now, this is what I suggest would be the most efficient course of action to bring you up to speed on modern day OOP clean coding practices:

  • Take a refresher course/tutorial on OOP. Focus on understanding names and concepts, to avoid misunderstandings in the future bullet points.
  • Read up on Clean Architecture. Learn how/why code needs to be separated into different layers.
  • When you understand layer separation, delve into inversion of control, dependency injection, and dependency injection frameworks. In most enterprise-grade codebases, DI containers are the glue that holds the layers together (and they act not too dissimilar from what you expect your "factories" to behave like).
  • You should now have a basic understanding of software architecture and how to separate things. From this point on, you just need to continuously look at improving your approaches whenever you see a problem (or when you have time/effort to spare). Focus on SOLID first, it covers many (but not all) of the common issues with OOP implementations.
17
  • You say its not commen to create (though a servicefactory) the service inside my controller action, I guess I have to instanciate them outside my controller action? I've played around with mvc for .netcore a bit and seen they are doing this upfront at start of the application. Is this what you mean? Also I understand I should use a ProductRepository instead of ProductService which will give me a business logic version of a product instead of the entity? – ikwillem Apr 15 at 11:53
  • 1
    @ikwillem "You say its not commen to create (though a servicefactory) the service inside my controller action, I guess I have to instanciate them outside my controller action? [..] doing this upfront at start of the application. Is this what you mean?" Very roughly, yes, but you're not quite on the money here. I suggest you move away from the factory pattern for now as you're misusing that label. And yes that "at startup of application" logic is the registration of the dependency injection - refer to the second to last bullet point at the end of the answer. – Flater Apr 15 at 11:59
  • 1
    @ikwillem "Also I understand I should use a ProductRepository instead of ProductService" Not "instead of", but rather "in conjunction with". The controller speaks to the service, and the service in turn speaks to the repository. Think of it like a company's work process. The salesman makes a sale, requests the production manager to build this thing for them. The production manager instructs the foreman, and the foreman instructs his staff. Different roles, different responsibilities, but the overall idea is to do the work that needs to be done by each doing their own defined part of it. – Flater Apr 15 at 12:02
  • 1
    @ikwillem: You've got the right idea, but you're lacking the correct words. You're trying to get at the idea that your Product data entity is not the same as the ProductDTO that your business logic will be handling. That is correct. This is something that will be explained as part of the Clean Architecture and layer separation bullet point in the last part of the answer. – Flater Apr 15 at 12:12
  • 1
    @ikwillem: Note that my answer doesn't contain all the answers to all of your questions. I mostly tried to point out the things you're missing, and then give you a structured approach on how to learn about the things you're missing (cfr the bullet points at the end). I'm mentioning this because I don't want you to mistakenly assume that my basic overview is somehow the complete image. It's severely limited in scope based on what I can write here in StackExchange's Q&A format - which is why blog posts and tutorials are better guidance resources than answers posted here. – Flater Apr 15 at 12:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.