fix(oop): assign the return value to the method name in Capacity - #40
fix(oop): assign the return value to the method name in Capacity#40Poseidonas wants to merge 1 commit into
Conversation
In ST a method returns by assigning to its own name. Capacity computes the
percentage into the protected _capacity member and stops there, so the
method returns 0 to its caller.
program.st reads that return value:
capacity_percentage := tank.Capacity(currentVolume := current_volume);
and the exercise text asks for "Capacity : REAL that returns a real with
the percentage of capacity of the volume of the tank", so the exercise as
shipped does not do what it describes.
The member assignment is kept and the return added on top, which is the
shape GetState already uses in ValveBase.st:
GetState := ValveState#Open;
_state := GetState;
Reported in simatic-ax#38.
|
@Poseidonas thanks for your contribution. I see the issues, you mentioned in your PR. First: It was correct, that the method "capacity" hasn't returned a correct value. Instead it has just stored it in the member variable _capacity. But to be honest, it's a little bit confusing: the capacity_percentage will be written cyclically. In the tank capacity will just be calculated internally and shouldn't return a value. But in this case, I would rename the method to CalculateCapacity without a return value. Over all, it seems not to be a good example to demonstrate inheritance. It just shows inheritance, but the problem which has to be solved with inheritance is Regarding the GetState of the ValveBase class: is redundant, to line 27/29 and can be removed. Regarding the TankWithShape... I think, that's the same issue linke with the Capacity-Method. I would like prefer a pattern like and then working with Getter/Setter-Methoden (or initializers in ST). |
Fixes #38.
Capacitycomputes the percentage into the protected_capacitymember and stops there. In ST a method returns by assigning to its own name, so it hands back0.That return value is read:
and the exercise text asks for:
So
capacity_percentagestays at zero for anyone following the module, which is a confusing thing to meet while learning the language.The member assignment is kept and the return added on top, which is the shape
GetStatealready uses inValveBase.st:Applied in both copies —
exercises/solution/andexercises/4_inheritance_complex_valve/.Two things I looked at and left alone
0_basic_valve_class/src/ValveBase.stalso has aGetStatethat does not assign to its name, but every method there is;— it is the skeleton the exercise asks the reader to fill in, so it is correct as it stands.TankWithShape.sthas twoVolumeCalculator : REALmethods which assign tovolumerather than to the method name. Here I was not sure what you intend: the slides say the volume is "calculated with a method called VolumeCalculator and the result stored in the property volume", and the only caller ignores the return value:So the body matches the brief and nothing is broken — but then the
: REALreturn type is never used, which is its own small trap for a reader learning how ST methods return. Either addingVolumeCalculator := volume;or dropping the return type would settle it. Happy to do whichever you prefer, in this PR or a separate one.On verification
I have no AX toolchain here, so I did not compile this. What I checked is that the change follows the assignment form already used elsewhere in the module, that the callers listed above do read the value, and that nothing else in module 6 has the same shape — I walked every
METHOD ... : <type>in the module and the only remaining ones are the two described above.