Question on style
Locked
 
|
I'm writing a little app to calculate the roots of quadratic equations. The first step is to build the model and then I would like to link this model with a Spec GUI.
So far the model is almost done (I have to solve yet imaginary roots). Would you consider this good code? Would you change something? or re-arrange the code? This is what I have so far: "The idea is to have a class to calculate quadratic equations. This first draft only calculates real roots (leaving aside imaginary roots). A QuadraticEquation is a class used to solve Quadratic Equations of the form: ax^2 + bx + c = 0, where a is not 0. You can set the coefficients one by one using: self quadraticCoefficient: aNumber self linearCoefficient: aNumber self constant: aNumber." Object subclass: #QuadraticEquation instanceVariableNames: 'quadraticCoefficient linearCoefficient constant checkTerm root1 root2' classVariableNames: '' category: 'IS-Math' "I create the accessors and then the following methods" QuadraticEquation>>calculateRoots checkTerm := ((linearCoefficient squared) - ( 4 * quadraticCoefficient * constant)) . self checkNegativeSqrt. QuadraticEquation>>checkNegativeSqrt self checkTerm >= 0 ifTrue: [ root1 := ((linearCoefficient negated) + (checkTerm sqrt)) / (2 * quadraticCoefficient). root2 := ((linearCoefficient negated) - (checkTerm sqrt)) / (2 * quadraticCoefficient) ] ifFalse: [ ^ 'Error: Negative square root']. "Example: -3x^2+2x+2=0 root1 = -0.5485837703548636 root2 = 1.2152504370215302 print(it) | anEquation | anEquation := QuadraticEquation new. anEquation quadraticCoefficient: -3. anEquation linearCoefficient: 2. anEquation constant: 2. anEquation calculateRoots. anEquation root1. Then change the last line to 'anEquation root2' to get the other root." Thanks so much for the input. Best regards Nacho
Nacho
Smalltalker apprentice.
Buenos Aires, Argentina.
|
|
Hi Nacho, On Wed, Apr 2, 2014 at 6:35 AM, nacho <[hidden email]> wrote:
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 |
|
|
Damien,
Thanks a lot for your comments and the time taken to review the code. It makes a lot of sense what you said, and it's a great aid in learning to not only solve a given problem but also to do it with style and in a better Smalltalk way. I will refactor this to reflect your suggestions. I read in a message that someone was asking about 99 problems for Smalltalk and I thought that it would be a wonderful idea to have simple problems solved in a very good way (not only solving the problem but also doing it with style), so I started to formulate some problems for me. I'm writing notes to myself for every problem solved and if you don't mind I will like to quote what you say. May be in the future as I go on with this little, but increasing problems, someone could benefit also by having not only the code, but also some comments on why some approach was taken, or problem tackled. Thanks again best Nacho
Nacho
Smalltalker apprentice.
Buenos Aires, Argentina.
|
|
|
Well here is again, the refactored version.
"The idea is to have a class to calculate quadratic equations. This second draft still only calculates real roots (leaving aside imaginary roots). But thanks to Damien Cassou I think it has a better style. A QuadraticEquation is a class used to solve Quadratic Equations of the form: ax^2 + bx + c = 0, where a is not 0. You can set the coefficients using the following method setQuadraticCoefficient: linearCoefficient: constant: " Object subclass: #QuadraticEquation instanceVariableNames: 'quadraticCoefficient linearCoefficient constant roots' classVariableNames: '' category: 'IS-Math' "I create accessors (only getters for each term) and then the following method to set the terms" QuadraticEquation>>setQuadraticCoefficient: aNumber1 linearCoefficient: aNumber2 constant: aNumber3 quadraticCoefficient := aNumber1. linearCoefficient := aNumber2. constant := aNumber3 QuadraticEquation>>calculateRoots | checkNegative | checkNegative := linearCoefficient squared - ( 4 * quadraticCoefficient * constant). checkNegative >= 0 ifFalse: [ Error signal: 'Negative square root'] ifTrue: [ ^ self solveRoots: checkNegative] QuadraticEquation>>solveRoots: aTerm | rootA rootB | roots := OrderedCollection new. rootA := (linearCoefficient negated + aTerm sqrt) / (2 * quadraticCoefficient). rootB := (linearCoefficient negated - aTerm sqrt) / (2 * quadraticCoefficient). roots add: rootA; add: rootB. ^ roots " Example: -3x^2+2x+2=0 an OrderedCollection(-0.5485837703548636 1.2152504370215302) print(it) | anEquation | anEquation := QuadraticEquation new. anEquation setQuadraticCoefficient: -3 linearCoefficient: 2 constant: 2. anEquation calculateRoots." Thanks again Best, Nacho
Nacho
Smalltalker apprentice.
Buenos Aires, Argentina.
|
|
|
Hi Nacho,
I wouldn't throw an error because otherwise one must know beforehand that an equation is non-resolvable or must use #on:do: everywhere. Since that when you ask to resolve an equation you get back a collection of solutions, a non-resolvable equation could just return an empty collection, couldn't it?. If you really want to consider that asking to resolve a non-resolvable equation is an error, another solution would be to provide a #rootsIfNone: method. That's a common pattern in Smalltalk. I would also rename 'checkNegative' to 'discriminant', because it's not obvious to see what that variable represents at first glance. On 2 avr. 2014, at 15:14, nacho <[hidden email]> wrote: > Well here is again, the refactored version. > > "The idea is to have a class to calculate quadratic equations. > This second draft still only calculates real roots (leaving aside imaginary > roots). > But thanks to Damien Cassou I think it has a better style. > > A QuadraticEquation is a class used to solve Quadratic Equations of the > form: ax^2 + bx + c = 0, where a is not 0. > > You can set the coefficients using the following method > setQuadraticCoefficient: linearCoefficient: constant: " > > Object subclass: #QuadraticEquation > instanceVariableNames: 'quadraticCoefficient linearCoefficient constant > roots' > classVariableNames: '' > category: 'IS-Math' > > "I create accessors (only getters for each term) and then the following > method to set the terms" > > QuadraticEquation>>setQuadraticCoefficient: aNumber1 linearCoefficient: > aNumber2 constant: aNumber3 > quadraticCoefficient := aNumber1. > linearCoefficient := aNumber2. > constant := aNumber3 > > > > QuadraticEquation>>calculateRoots > | checkNegative | > checkNegative := linearCoefficient squared - ( 4 * quadraticCoefficient * > constant). > > checkNegative >= 0 > ifFalse: [ Error signal: 'Negative square root'] > ifTrue: [ ^ self solveRoots: checkNegative] > > QuadraticEquation>>solveRoots: aTerm > | rootA rootB | > roots := OrderedCollection new. > rootA := (linearCoefficient negated + aTerm sqrt) / (2 * > quadraticCoefficient). > rootB := (linearCoefficient negated - aTerm sqrt) / (2 * > quadraticCoefficient). > roots add: rootA; add: rootB. > ^ roots > > > " Example: -3x^2+2x+2=0 > an OrderedCollection(-0.5485837703548636 1.2152504370215302) > > print(it) > > | anEquation | > anEquation := QuadraticEquation new. > anEquation setQuadraticCoefficient: -3 linearCoefficient: 2 constant: 2. > anEquation calculateRoots." > > Thanks again > Best, > Nacho > > > > ----- > Nacho > Smalltalker apprentice. > Buenos Aires, Argentina. > -- > View this message in context: http://forum.world.st/Question-on-style-tp4752165p4752262.html > Sent from the Pharo Smalltalk Users mailing list archive at Nabble.com. > |
|
|
This post was updated on .
<quote author="camille teruel">
Hi Nacho, >I wouldn't throw an error because otherwise one must know beforehand that an equation is non-resolvable >or must use #on:do: everywhere. >Since that when you ask to resolve an equation you get back a collection of solutions, a non-resolvable >equation could just return an empty collection, couldn't it?. >If you really want to consider that asking to resolve a non-resolvable equation is an error, another solution >would be to provide a #rootsIfNone: method. That's a common pattern in Smalltalk. >I would also rename 'checkNegative' to 'discriminant', because it's not obvious to see what that variable >represents at first glance. Very good points. I came up with a third draft. This is evolving good! Thanks Nacho "The idea is to have a class to calculate quadratic equations. This second third still only calculates real roots (leaving aside imaginary roots). But thanks to Damien Cassou, and Camille Teruel has a better style. A QuadraticEquation is a class used to solve Quadratic Equations of the form: ax^2 + bx + c = 0, where a is not 0. You can set the coefficients using the following method setQuadraticCoefficient: linearCoefficient: constant:" Object subclass: #QuadraticEquation instanceVariableNames: 'quadraticCoefficient linearCoefficient constant roots' classVariableNames: '' category: 'IS-Math' "I create accessors (only getters for each term) and then the following method to set the terms" setQuadraticCoefficient: aNumber1 linearCoefficient: aNumber2 constant: aNumber3 quadraticCoefficient := aNumber1. linearCoefficient := aNumber2. constant := aNumber3 QuadraticEquation>>calculateRoots | discriminant | discriminant := linearCoefficient squared - ( 4 * quadraticCoefficient * constant). discriminant >= 0 ifFalse: [ ^ self rootsIfNone ] ifTrue: [ ^ self solveRoots: discriminant] QuadraticEquation>>solveRoots: aTerm | rootA rootB | roots := OrderedCollection new. rootA := (linearCoefficient negated + aTerm sqrt) / (2 * quadraticCoefficient). rootB := (linearCoefficient negated - aTerm sqrt) / (2 * quadraticCoefficient). roots add: rootA; add: rootB. ^ roots QuadraticEquation>>rootsIfNone ^ 'No real solution for the equation' " Example: -3x^2+2x+2=0 an OrderedCollection(-0.5485837703548636 1.2152504370215302) print(it) | anEquation | anEquation := QuadraticEquation new. anEquation setQuadraticCoefficient: -3 linearCoefficient: 2 constant: 2. anEquation calculateRoots."
Nacho
Smalltalker apprentice.
Buenos Aires, Argentina.
|
|
|
As solving an equation is basically giving the set of its solutions, I prefer to use a set to solve the equation.Instead of the OrederedCollection of course. It simplifies the algorithm if the discriminant is 0, because adding two times the same root yields a set with only one element, not the same element counted twice. The algorithm (much like the one with ordered collection) goes like this: *create an empty set with Set new. *compute the discriminant. *if it is positive, add to the set the "two" roots. *you're done! On Wed, Apr 2, 2014 at 8:45 PM, nacho <[hidden email]> wrote:
|
|
|
In reply to this post by nacho
On 2 avr. 2014, at 18:45, nacho <[hidden email]> wrote:
In fact, what I meant is that the function that compute the roots should be called #calculateRootIfNone: and take a block as parameter: QuadraticEquation>>calculateRootsIfNone: aBlock | discriminant | discriminant := linearCoefficient squared - ( 4 * quadraticCoefficient * constant). discriminant >= 0 ifFalse: aBlock ifTrue: [ ^ self solveRoots: discriminant] Like this, you can provide a #calcultaeRoots method like this: QuadraticEquation>>calculateRoots ^ self calculateRootsIfNone: [ self error: 'No real solution for the equation' ] QuadraticEquation>>calculateRootsSafely ^ self calculateRootsIfNone: [ { } ] This is a common pattern in Smalltalk. Look at collection methods like #detect:ifNone: #at:ifPresent: #at:ifAbsent: ,etc ... BTW, is it expected that when the discriminant equals 0, the same root is returned twice?
|
|
|
In reply to this post by Alain Busser
Alain,
Very good idea. A Set it's much better than an OrderedCollection. Already changed that. Now when the solution is in both cases 0, it returns only (0) and not (0 0). And also intuitively I agree with you that the solutions of an equation are a set. Thanks! Nacho
Nacho
Smalltalker apprentice.
Buenos Aires, Argentina.
|
|
|
In reply to this post by camille teruel
Camille,
I now understood what you've said. It makes sense and I've changed it that way. The only thing I'm not happy with is that implementing: QuadraticEquation>>calculateRoots ^ self calculateRootsIfNone: [ self error: 'No real solution for the equation' ] Makes the debugger appear and I would rather let the user know that there are no real solutions. Bringing up the debugger can make one think that the problem is in the code. What do you think of: QuadraticEquation>>calculateRoots ^ self calculateRootsIfNone: [ ^ 'No real solution for the equation' ] Thanks! nacho
Nacho
Smalltalker apprentice.
Buenos Aires, Argentina.
|
|
|
In reply to this post by nacho
Yes, and if there are no solution, you get an empty set instead of an error message. I find it more coherent, and simpler to use. On Wed, Apr 2, 2014 at 9:48 PM, nacho <[hidden email]> wrote: Alain, |
|
|
In reply to this post by nacho
nacho wrote:
> Camille, > > I now understood what you've said. It makes sense and I've changed it that > way. The only thing I'm not happy with is that implementing: > > QuadraticEquation>>calculateRoots > ^ self calculateRootsIfNone: [ self error: 'No real solution for the > equation' ] > > Makes the debugger appear and I would rather let the user know that there > are no real solutions. Bringing up the debugger can make one think that the > problem is in the code. > > What do you think of: > > QuadraticEquation>>calculateRoots > ^ self calculateRootsIfNone: [ ^ 'No real solution for the equation' ] > Thanks! > nacho > > > > > > ----- > Nacho > Smalltalker apprentice. > Buenos Aires, Argentina. > -- > View this message in context: http://forum.world.st/Question-on-style-tp4752165p4752388.html > Sent from the Pharo Smalltalk Users mailing list archive at Nabble.com. > > > cheers -ben |
|
|
In reply to this post by nacho
On Wed, Apr 2, 2014 at 2:34 PM, nacho <[hidden email]> wrote:
> if you don't mind I don't mind, go ahead. And don't hesitate to send us your future code -- Damien Cassou http://damiencassou.seasidehosting.st "Success is the ability to go from one failure to another without losing enthusiasm." Winston Churchill |
|
|
Thanks to all for your input. I consider this problem solved and I will write a little tutorial for those who might be interested.
I now will be moving to another simple problem and will post the code here to go again with this process. In this way, we can accumulate slowly simple, but usefull problems resolved in a proper way. As I mentioned the idea is to have some sort of 99 problems for Smalltalk, of course everyone is invited to contribute. Best Nacho
Nacho
Smalltalker apprentice.
Buenos Aires, Argentina.
|
«
Return to Pharo Smalltalk Users
|
1 view|%1 views
| Free forum by Nabble | Edit this page |