Refactorings in Math-ODE #229
Conversation
SergeStinckwich
commented
Feb 18, 2022
- Refactor isNil ifTrue: as ifNil:
- Demos with announcement are working again
- Demos with announcement are working again
hemalvarambhia
left a comment
There was a problem hiding this comment.
I am still reviewing at the moment. I do notice duplication in the error handing parts and another thing. These will be worth addressing either now or in the next PR.
| ^ (self stepSize / 2) * (3 * (system state: aState time: t) - (system state: prevState time: t - self stepSize)) + aState | ||
|
|
||
| self stepSize ifNil: [ self error: 'step size required by stepper' ]. | ||
| ^ self stepSize / 2 * (3 * (system state: aState time: t) |
There was a problem hiding this comment.
It looks like this computation may represent the same concept, but each class performs it differently. Is that right? At the very least, to give it a name,this computation might be extracted to an intention-revealing method or a local variable.
hemalvarambhia
left a comment
There was a problem hiding this comment.
A great start. Some suggestions offered on how to improve things next. Thank you 😊
| self stepSize ifNil: [ self error: 'step size required by stepper' ]. | ||
|
|
||
| ^ self stepSize / 2 * (3 * (system state: aState time: t) - (system state: prevState time: t - incrementOfTime)) + aState | ||
| ^ self stepSize / 2 * (3 * (system state: aState time: t) |
There was a problem hiding this comment.
You noticed duplication here. I assume removing it is the next PR.
| self stepSize isNil | ||
| ifTrue: [ self error: 'step size required by stepper' ]. | ||
| ^ (self stepSize / 2) * (3 * (system state: aState time: t) - (system state: prevState time: t - self stepSize)) + aState | ||
|
|
There was a problem hiding this comment.
The doStep encompassing method could be renamed to:
stepFrom: previousState to: newState inStepsOf: stepSize
This seems more intention-revealing and almost fluent. Is this really what's happening? Are we stepping from one state to another in steps? Seems so.
| self stepSize isNil | ||
| ifTrue: [ self error: 'step size required by stepper' ]. | ||
| ^ self stepSize * ( ( (55/24) * (system state: thirdState time: initTime+ (3* self stepSize) )) - ((59/24)*(system state: secondState time: initTime+ (2* self stepSize))) + ((37/24)*(system state: firstState time: initTime+ self stepSize ) )- ((3/8)*(system state: initState time: initTime ) )) + thirdState. | ||
| PMAB4Stepper >> doStep3State: thirdState secondState: secondState firstState: firstState initState: initState initTime: initTime [ |
There was a problem hiding this comment.
I wonder if we should make the signatures (selectors) look similar across the classes. There is a pattern by Sandi Metz, J B Rainsberger and Katrina Owen which involves making similar code look even more similar (i.e. makes the duplication obvious).