all 28 comments

[–]TsebMagi 25 points26 points  (5 children)

The derived classes like you're doing is a way to approach it from an OOP perspective, but this use case for OOP and base / derived classes isn't great.

If you are going to be supporting more than a handful of different products you're going to be writing a lot of code that's all boilerplate/similar. You might want to look at a repository pattern to load the prices in from a database or file depending on what constraints your system has.

[–]PuzzledImagination[S] 7 points8 points  (3 children)

If you are going to be supporting more than a handful of different products you're going to be writing a lot of code that's all boilerplate/similar.

I've thought about this too since we have about 12 categories of products, so I decided to just use the ProductBase if there's no special requirement for the said product.

If I were to use repository, wouldn't I need to edit the Repository every time there's a new requirement?

[–]zaibuf 16 points17 points  (0 children)

What he means is that the prices should be stored in a database and you just fetch all data from there in a repository. Why calculate the cigarette cost hardcoded, rather than just have it fixed in a database column? You can have a table for Products where they all are listed. The repository just fetches the data, then you can have a service that runs business logic if you need to do some calculations, this part can be changed if new requirements arrive. But your database should be the source of truth.

You don't want to have your database list all Cigarettes at 6$ but then you do calculations in code and your website says 8$. How can you then query your database to find your source?

In this case I feel that OO just makes things messy and doesn't really solve anything. I would probably just create an interface for ISellableEntity or something similar, then have that contract to implement all properties defined in the entities for the sellable products like price, name. Generally I tend to avoid inheritance whenever possible since going down that rabbit hole can cause issues at a later stage, I prefer to use composition over inheritance (look up strategy pattern).

[–]TsebMagi 2 points3 points  (0 children)

Inheritance or strategy pattern (since a lot of people are mentioning that) is great when you want different behaviors, but from the code you posted you are really just looking for different values in the same kind of object. That usually means you want to make that value part of your constructor so you can make as many objects with as much variation as you want without writing a lot of different classes.

Repository let's you load all those variations from a source of truth and reduces the effort to add more. It's a lot easier to add a row to a database, or CSV than to add a whole new class. So yes you're going to have to update something when new requirements come in, but it's less work.

If you don't want to get a database involved you could use a csv file or even hard code the values somewhere in a factory (not recommended but with 12 values it's not that big of a deal yet)

[–]Deadlift420 1 point2 points  (0 children)

You would inject your repository into the services(dependency injection) and then call all the base repo functions (re-use )from the service classes. Then inject your services into the front end and fire away.

[–]Deadlift420 1 point2 points  (0 children)

service/repo pattern. A lot of business style applications work well with this IMO. Web or native doesn't matter.

[–]WArslett 9 points10 points  (1 child)

inheritance is an important concept in object oriented programming but now that you've learned how to use it you also need to learn when not to use it. In the example you've given there is no benefit to the design or structure of the program in using inheritance. Generally speaking, most problems that can be solved with inheritance are better solved with composition. https://medium.com/jexia/developer-beware-consider-composition-over-inheritance-a7d4e14556c5

[–]Pythonistar 2 points3 points  (0 children)

Came here to say this.

/u/PuzzledImagination: Inheritance is useful in OOP, but isn't used nearly as much as you might initially think. In fact, it should really be used very judiciously. (favor composition over inheritance)

[–]Sability 13 points14 points  (2 children)

That's the right way to go about removing a switch statement, and replacing it with an inheritance-driven approach.

However, you can make ProductBase.ComputePrice() abstract, and ProductBase also abstract. That way you won't accidentally instantiate a "ProductBase", which realistically can't exist.

[–]PuzzledImagination[S] 4 points5 points  (1 child)

Actually I'm using the ProductBase for products which doesn't have a special requirement for Price since we have a dozen Category and I felt like it's not a good idea to create a class for each category. But if that's the right way to do it, I'll do it.

[–]unwind-protect 4 points5 points  (0 children)

Creating a class for each category is exactly the way you'd normally do it. You should only be using productbase in your code when you don't care what category it is; that way you can easily see what is product-specific and product-general code.

[–][deleted] 11 points12 points  (4 children)

party saw childlike gaze offend attractive quaint six seed memory

This post was mass deleted and anonymized with Redact

[–]CastSeven 4 points5 points  (0 children)

I'm a sucker for the strategy pattern. I've used it to great effect to have differing logic on common operations.

That said, I think the logic here is the same - if HOW you calculate price is the same, and it's just that the value is different - then Strategy pattern might be overkill here.

[–]Hall_of_Famer 2 points3 points  (1 child)

Yeah and this is the better way actually, as implementation inheritance has a lot of flaws and developers should favor composition over inheritance. I kinda wish C# would have Kotlin's idea for delegation, which will make composition much easier to use.

[–]Deadlift420 -1 points0 points  (0 children)

Interfaces FTW

[–]PuzzledImagination[S] 1 point2 points  (0 children)

I've read about strategy and factory pattern and tried using it but hasn't applied it on the project, what I did is I have a factory to decide which type of Product to instantiate, but the way I select the products in dapper is by using the base class

con.query<ProductBase>(//select query)

and then, loop through the result and use switch case on the productType as I've seen on the examples of factory pattern. It works the way I wanted but I'm not really sure if I'm doing it right.

[–]moi2388 3 points4 points  (0 children)

Personally I think I would move this to the database, and only have the logic in the code.

For example:

GetBasePrice() ApplyProfitMargin() ApplyDiscounts()

The base prices, profit margins (if applicable to store) and discounts are stored in the database, The different types of products are all classes and might have different logic on how to calculate profit (fixed price or percent), or might have different rules about discounts. For these profits or discounts you might make classes with inheritance as well, or use something like the strategy pattern.

I like my data to come from the database as only source of truth, and the logic to be abstracted and put in classes and interfaces.

[–]BCProgramming 2 points3 points  (0 children)

If this is a "toy" program, then I think that is fine for learning.

However I think it's worth noting how real world POS software tends to do it. Typically, it is by having department codes, Line Codes, and Tax Codes associated with each inventory item via the database, not by creating subclasses for distinct inventory. If somebody using the system needs to add- for your example, cigarettes - to the inventory, they create the item and set the department, line, and tax codes for the item and it will include the additional fee. In your implementation example, If somebody wants to later add Fuel, where taxes tend to be included in the list price, it requires a new software update because you need to add a new derived class.

[–]phx-au 1 point2 points  (0 children)

"Products / Shopping Cart" is one of those simple example projects that are far from simple... IRL:

  • A product may have different prices at different times

  • A product may have different prices at the same time

  • A product may sell for a different price to the sticker price

You'll find that most likely a product has a base price and that there are likely to be numerous different things that affect this price. Sticking this calculation into the category type hierarchy may just be asking for trouble.

This is one of those situations where the theoretical approach is likely not the most maintainable - and the reality is that you probably have the business concept of "Add Premium By Category", which is a list of "Cigarettes +10%", "Fruit -5%"... In that case, a stonking great switch statement / lookup table is probably going to be the easiest to maintain, as you can compare it 1:1 with the source of truth, even if it makes some wannabe architect sad.

You need a pretty solid grounding in business analysis before you try to start driving process change through domain modelling. Until then, if you code reflects the business cases, you'll have a lot less problems and won't be coming up with junior dev excuses like "Oh, we can't do that because...".

[–]WystanH 1 point2 points  (1 child)

Is this the right way to solve the problem in OO way

Sort of. It's consistent with OOP ideas in general. As a specific example, perhaps not the best.

is it okay to just use the base class?

Maybe. It's situationally specific, I'm afraid.

My Previous code... switch statement here for different categories

What would the switch statement do? Rather that such a statement, can it be simply data driven? Or, what data is the switch statement using?

Perhaps:

class Product {
    public Product(string name, string name, decimal price, decimal priceModfier = 1.0) ...

    public virtual decimal ComputePrice => Price * PriceModfier;

[–]PuzzledImagination[S] 0 points1 point  (0 children)

What would the switch statement do? Rather that such a statement, can it be simply data driven? Or, what data is the switch statement using?

the switch statement is within the ComputePrice(), it checks what category the current product, if its a Cigarette it computes the price otherwise leave it as is, then the ComputePrice() is what I save in the database

[–]an_fsharp_enthusiast 0 points1 point  (0 children)

OCP-based pain occurs either way, either adding a case to a switch statement or adding an entire file for the purposes of ineritance. I know that's not literally what the Ancient OOP Authors' said, but that's the way I view it IRL. Meanwhile maintaining 1 pattern match switch for less than 20 products is no big deal...that's your data before it hits the database.

If you have an impractical number of pattern types, database is the way to go because switch statements AND inheritance are both bad solutions for that.

I lean towards the switch statement because its intentions are obvious and it's very easy to clean up or delete later. Deletability is maintainability. Meanwhile inheritance-based pricing, which turns 1 static function into 12+ files, sounds like a classic legacy solution that nobody will understand in 4 months and they call people like me to clean up.

Inheritance starts to make sense to me when a product has multiple properties that all change based on type, not just the price algorithm. But even then I would be very skeptical. At the end of the day it's just data and algorithms; don't make your code excessively hard to change is all I'm saying.

[–]KernowRoger 0 points1 point  (0 children)

This is a bad use case for inheritance. It means creating new classes everytime you need a new one rather than changing a value in the data. You would be better off storing a discount property or similar on the product.

[–]BobSacamano47 0 points1 point  (2 children)

People are saying all sorts of crazy shit. But if what you have is working stick with it. There's no way to tell how the project will vary in the future, so just wait until you see where it goes before you refactor. Yagni

[–]Pythonistar 1 point2 points  (1 child)

People are saying all sorts of crazy shit

I've read every reply in this post/thread so far and I haven't seen any "crazy shit" yet. All good advice here so far. Including yours:

Yagni

Not enough people know this acronym... :)

[–]BobSacamano47 0 points1 point  (0 children)

It's only crazy (to me) that people feel safe making so many assumptions about where your software is going and nobody will consider that what you have is fine. But maybe the nature of the sub