Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

What is better to do in this type of scenario: populate fields or return an object?

The situation is like this: I'm doing calculations to find the prices (inc tax, ex tax, discounted etc) of a product. Is there any advice or are there any guidelines on how to handle a return value?

Populate fields:

public class ProductPrice
{
    public int BasePrice;
    public int IncTax;
    public int ExTax;
    // ...

    public void GetPricesForProduct(int ProductID)
    { /* Populate fields based on results */ }
}

Return object:

public class PriceCalculator
{
    public ProductPrice GetPricesForProduct(int ProductID)
    { /* Populate object fields and return object */ }
}
public class ProductPrice
{
    public int BasePrice;
    public int IncTax;
    public int ExTax;
}

I'm not sure I really like the first method, because the void return type doesn't make it clear what happens when the method executes. However, returning an object results in a class which has no functionality.

share|improve this question
1  
In what language? Java/C#? Which is preferred will matter a bit on the idioms of the language you're using. –  Telastyn Apr 19 '12 at 13:06
1  
@Telastyn, based on the brackets and naming convention, I'd say C#, but it's hard to say. –  Neil Apr 19 '12 at 13:44
    
In the first case, why do you need to pass productId to the method , If productPrice object itslef knows its own Id? –  java_mouse Apr 19 '12 at 13:47

9 Answers 9

up vote 8 down vote accepted

You're right to prefer the second method. The first method would easily clutter a class with members that would better be suited torwards an object which represents those values.

Better still, add a method to ProductPrice which performs the calculation and change "PriceCalculator" to "PriceFetcher" which simply gets the raw values and instantiates ProductPrice.

public class PriceFetcher
{
    public ProductPrice GetPricesForProduct(int ProductID)
    { /* Populate object fields and return object */ }
}
public class ProductPrice
{
    public int BasePrice;
    public int IncTax;
    public int ExTax;

    public float GetPrice() 
    { /* Perform calculation on given data */ }
}

This method has the added advantage that the object determines how the price is calculated and not the method which fetched the data, allowing you to do more complicated operations later with derived classes of ProductPrice and instantiate them using a factory in PriceFetcher.

share|improve this answer
    
I like the idea of the ProductPrice class implementing the calculation logic –  AndyBursh Apr 19 '12 at 13:16

The second approach appears better to me. It decouples price from product.

The class may not have any behavior at this time, but it may in the future. Even if it doesn't it doesn't mean it doesn't have a function - holding data that doesn't belong to the product class.

share|improve this answer
    
+1. Always favor a more simplistic and modularized approach or you risk having classes which perform multiple tasks. –  Neil Apr 19 '12 at 13:06

Well the use of void doesn't make sense as it conflicts with the name of the function. If you have a something that is called get*** I expect that to return an object (or a primitive). The second approach seems to be more correct as your PriceCalculator is really the main program (at least that is how it looks in this trivial example). I would rename the function to GetPricesForProductById as that is what you are doing, not getting prices by a Product

share|improve this answer

the second option (the factory model) is better (add the id to the returned object and make it immutable for completion but that is all I would change)

when you have a method named get* one expects it to return something but even calculatePricesForProduct lacks what it should be because you start from an different object and overwrite it with the new product. This type of mutable objects are a pain to safely work with (making a copy of it everywhere you need to use it) unless you planned out their use fully

share|improve this answer

The second one is clearly better. Imagine you call GetPricesForProduct a second time with a different id before you have copied all fields. To me the first version is an anti-pattern. Never do that.

share|improve this answer

Second Method (for all the reasons already given), and:

  • make PriceCalculator an interface; one day, you might need one for consumers, one for business customers, one for government customers, one for international customers (EU), one for international customers (outside the EU) etc., so you will implement different Calculators and use the one you need

  • make the fields in ProductPrice immutable; maybe it's best to make ProductPrice an interface with some getters (read-only attributes in C#); because you might want to reuse those objects, i.e. by caching them or even using a product price for more than one product (yes, the red mouse costs exactly as much as the green mouse, the yellow mouse and the blue mouse)

share|improve this answer

I don't know why is PriceCalculator is a class on its own. The calculated price value is a property that logically belongs to ProductPrice class. The price calculation is not a utility or helper it is tightly dependent on the ProductPrice and (should) be encapsulated in that class. Accordingly, PriceCalculator should be CaculateProductPrice() method inside ProductPrice class (or Product class). Another reason for that is you don't ever want to instantiate ProductPrice and PriceCalculator separately. In fact, you probably never need to create an object from PriceCalculator on its own.

Edit: Added the text (or Product class), since I am not fully aware of the properties and assications in and between Product and ProductPrice.

share|improve this answer
    
The product price is not a singular scalar value. It's actually represented by 3 or 4 different values. –  AndyBursh Apr 19 '12 at 14:21
    
@AndyBursh, thanks for your explanation. So, is the product price calculated from the individual values? If so, then the calculation belongs to the Product Class. I still can't see it on a class of its own. –  Emmad Kareem Apr 19 '12 at 14:34
    
Sorry, I worded that wrongly. There are 4 different prices (special and standard prices with and without tax), not one price with 4 values. These prices are calculated using the starting price, a modifier and a tax rate. Does this change your proposal? –  AndyBursh Apr 19 '12 at 14:48
    
@AndyBursh, thanks for your clarification. I stick by my proposal. PriceCalculator is not a class. –  Emmad Kareem Apr 19 '12 at 17:38
    
Agree the extra PriceCalculator only confuses the issue. –  James Anderson Apr 20 '12 at 9:27

Thinking about how prices work and what may happen in the future I would recommend:-

A single "productPrice" class.

The class should have a one or more getPrice methods

public int getPrice(getPrice.Type priceType) {}

public int getPrice(getPrice.Type priceType, int offerCode) {} 

public int getPrice(getPrice.Type priceType, int discount) {} 

Where 'priceType' is an ennum defining the type of price you want.

In a large real-life situation you may offer different prices depending a vast number of variables:-

  • Channel -- eg. Web price, may differ from mail order.
  • Special Offers -- customers who attended a trade fair, who visited a particular web site may be givein a offer code which gets them a special price.
  • Turnover -- customers who buy a lot get a better price.
  • New Customers -- introductory offers for new customers.
  • Location -- Marketing may be targeting a particular region or city.
  • Market Segment -- Marketing may be pushing to increase sales to a specific type of customer.
share|improve this answer

I'll be a little contrary, and say that the first method is better, IF you make this change:

Your GetPricesForProduct() function should be the CONSTRUCTOR for ProductPrice.

Because a ProductPrice object with no particular product doesn't MEAN anything; it's not properly initialized.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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