Class for exposing information with computer properties (getters)

Question

I need some advice on my code structure. I have read that setters and getters break one of the OOP principals, encapsulation, and that we should avoid them if possible but I cannot think of any other way to expose info other than getters.

I have a class which calculates price, savings and taxes based three inputs from the user through the initializer (initialPrice, discount and taxRate).

I'm using three computer properties to exposed the results, is this considered a bad thing? If yes, why and how can I improve my code?

Besides what I described above the class also has the ability to add all of the items in an array, assuming the instances of the CalculatorBrain were placed in an array.

Again, I'm more concern about the structure of my code. Any advice will be appreciated.

class CalculatorBrain {  

        private var taxRate:Double = 0  
        private var discount:Double = 0  
        private var initialPrice:Double = 0  

        func setInitialValues(initialPrice:Double, discount:Double, taxRate:Double){  
            self.taxRate = taxRate  
            self.discount = discount  
            self.initialPrice = initialPrice  
        }  

        private func salePrice()->Double{  
            return initialPrice - savings  
        }  

        var price:Double{  
            get{  
                return salePrice() + taxes  
            }  
        }  

        var savings:Double{  
            get{  
                return initialPrice * discount / 100  
            }  
        }  

        var taxes:Double{  
            get{  
                return salePrice() * taxRate / 100  
            }  
        }  

        internal func grandTotals(list:Array<Item>)->(grandPrice:Double, grandSavings:Double, grandTaxes:Double){  
            var grandPrice:Double = 0  
            var grandSavings:Double = 0  
            var grandTaxes:Double = 0  

            for i in 0..<list.count {  
                grandPrice = grandPrice + Double(list[i].totalPrice)  
                grandSavings =  grandSavings + Double(list[i].savings)  
                grandTaxes = grandTaxes + Double(list[i].taxes)  
            }  
            return (grandPrice, grandSavings, grandTaxes)  
        }  
    } 

Answer 1

Set Initial Values?

private var taxRate:Double = 0  
private var discount:Double = 0  
private var initialPrice:Double = 0  

func setInitialValues(initialPrice:Double, discount:Double, taxRate:Double){  
    self.taxRate = taxRate  
    self.discount = discount  
    self.initialPrice = initialPrice  
}  

I don't care for this at all.

First, I don't see a good reason for these three properties to necessarily be private. Why should the user have to guess what they've set their properties to?

Further more, you're not protecting these values from being reset. The user simply has to call setInitialValues again. The method name may be an attempted hint at that being the incorrect use of the class, but my mantra is let the compiler do as much for us as possible.

We should expose these properties to the user. We should not allow the user to modify these properties after initialization. We should write an init method.

let taxRate: Double
let discount: Double
let basePrice: Double

init(taxRate: Double = 0, discount: Double = 0, basePrice: Double = 0) {
    self.taxRate = taxRate
    self.discount = discount
    self.basePrice = basePrice
}

---

Why is salePrice() a function?

You have implemented basically everything else as a computed, read-only property. Why can't salePrice also be a computed, read-only property? If you want it to be private, that's fine if that makes sense for your class, but it can still be a computed property.

var salePrice: Double {
    return basePrice - savings
}

Also, note what I've done here. The get { } is omitted. Computed properties that just return a value can omit the get and are implicitly read-only. Any code is assumed to be for the get if you have omitted the get & set keywords.

---

internal is the default access level.

You have marked some things as private. You have omitted an access specifier on most things, and then your grand total method is specified as internal. Marking internal is the same as omitting an access specifier. You should drop this internal keyword here, or consistently use it with all the other properties. None of your members are public in your current implementation.

---

About that grandTotals method...

This method almost seems out of place if it weren't for the fact that the class is called CalculatorBrain. But it doesn't depend on any state, so it should probably be a static method.

Your loop is far from optimal. But, we don't even need a loop. We can use reduce.

return list.reduce((0,0,0)) { 
    return (
        $0.totalPrice + $1.totalPrice,
        $0.savings + $1.savings,
        $0.taxes + $1.taxes
    )
}

But... I might avoid returning a tuple. Why not just make a struct?

struct GrandTotal {
    let price: Double
    let savings: Double
    let taxes: Double
}

And now instead of a method that feels like it's in a weird place, we just add an appropriate init:

struct GrandTotal {
    let price: Double
    let savings: Double
    let taxes: Double

    init(list: [Item]) {
        let totals = list.reduce((0,0,0)) { 
            return (
                $0.totalPrice + $1.totalPrice,
                $0.savings + $1.savings,
                $0.taxes + $1.taxes
            )
        }
        
        price = totals.0
        savings = totals.1
        taxes = totals.2
    }
}