Hi Nacho,
On Wed, Apr 2, 2014 at 6:35 AM, nacho <0800na...@gmail.com> wrote:
> Would you consider this good code?
> Would you change something? or re-arrange the code?
>
Some random comments:
- your class models 2 roles: a quadratric equation and an algorithm to find
roots. And there are instance variables for these 2 roles. Typically, we
try to separate roles into separate classes. I would probably do one of
these 2 things instead:
1/ keep only 1 class that models a quadratic equation and remove the
instance variables that model the roots (checkTerm, root1, root2). You
don't really need these instance variables as they can be returned by
methods directly (and re-computed on demand).
2/ have 2 classes, one for the equation and one for calculating roots of
a particular equation.
- because you have accessors like #quadraticCoefficient:,
#linearCoefficient: and #constant:, your class represents mutable
equations: i.e., you can change any coefficient at any time. I'm not sure
that makes sense. I would probably do a class that represents immutable
equations with an initializer such as
#setQuadraticCoefficient:linearCoefficient:constant:
- 'checkTerm' could probably be passed as a parameter to #checkNegativeSqrt
instead of being an instance variable (this is related to the first comment)
- when setting checkTerm, you have too much parenthesis. Compare:
checkTerm := ((linearCoefficient squared) - ( 4 * quadraticCoefficient *
constant)) .
with
checkTerm := linearCoefficient squared - ( 4 * quadraticCoefficient *
constant).
Same comment for the calculation of root1 and root2
- you don't throw errors, you just return them (with strings). That's bad
:-). Compare
ifFalse: [ ^ 'Error: Negative square root'].
with
ifFalse: [ Error signal: 'Negative square root'].
- typically, we tend to eliminate the problematic cases first and then
write for the standard case (there is a lint rule that will check this for
you in your code). Compare
QuadraticEquation>>checkNegativeSqrt
self checkTerm >= 0
ifTrue: [ ... everything ok... ]
ifFalse: [ ... error signaled or returned ... ].
to
QuadraticEquation>>checkNegativeSqrt
self checkTerm >= 0
ifFalse: [ ... error signaled or returned ... ].
...everything ok...
- you could create unit tests to make sure your current code work and then
improve it making sure the tests still pass.
Enjoy
--
Damien Cassou
http://damiencassou.seasidehosting.st
"Success is the ability to go from one failure to another without losing
enthusiasm."
Winston Churchill
