nus-cs2030 / 2021-s2

2 stars 2 forks source link

Immutability of Classes for MOCKPA1 #55

Open ryantjx opened 3 years ago

ryantjx commented 3 years ago

In Level 1, Employee has a setSalaryIncrease(double) method that will set the annual raise rate.

From my pov, I would design annual raise rate as an attribute of Employee(Salary would have an annual raise rate). However, I am having a dilemma because if I set the attribute as private final, ie

'private final double annualRaise',

as it would go against the spirit of immutability. My understanding is that setSalaryIncrease(double) should create a new Employee object with the raise attribute, however, during the instantiation of Employee, there is no input for annualRaise(from Main.java).

My question is, should I continue designing my Employee and other classes as Mutable classes? or is there some other way to make them Immutable? (ps abit stupid heh)

Hope to seek some help regarding this!

miao-tianjia commented 3 years ago

Yes, I also got this problem at first... but I tried with mutable classes (i.e. I simply defined them as private instead of private final) and I passed all the test cases in CodeCrunch (instead of getting a terminated audition as before). So I think as long as you can pass all the cases in CodeCrunch, it will be fine to use mutable classes. (But I am still not very sure about the PE this time)

gableejh commented 3 years ago

From my understanding, this mock pa was crafted a few years back where immutability was not covered yet. For me, I made everything private final except for the annualRaise variable. Ideally, everything should be immutable i.e. private final

Keithlimhs commented 3 years ago

Normally our attributes should be declared private final to adhere to immutability and encapsulation principles of OOP as covered in our lectures and labs. However, as the mock PA was from a few years back, the concept of immutability was not covered extensively (i think) and hence they were allowed to have mutable attributes for their implementations. If you were to make the attribute mutable in this case you would still pass the test cases on CodeCrunch. However, for our PE this friday, our attributes should all be declared private final as what we have been doing for our labs to adhere to immutability design of classes

Atem729 commented 3 years ago

If I recall correctly, it was mentioned in lecture that immutability was not tested on in the past. In this case, it is acceptable to go without "final". However, I think prof also said it was not much to worry about as it should not require much effort to make it immutable if necessary.

marvinljw commented 3 years ago

Yes I agree with Atem729, I believe the syllabus has changed over the years. It is good to note on the immutability as it protects the client from changing the attribute values in the classes. However, in this case, it is okay to make it mutable.

etbf commented 3 years ago

i think in this case, in order to fit the code in main.java, we have to think of a way to set the annualRaiseRate to an employee. If we declare it and cannot take it in via the constructor, i think the most intuitive way is to set it via a void method, in which annualraise has to be mutable.

cf6600 commented 3 years ago

Maybe another way to make it immutable is actually to create another private constructor which takes in another value which is the annual raise rate. Making it private makes sure this special constructor is only called if the set raise method is called.