mentor question 2.

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

mentor question 2.

Pharo Smalltalk Users mailing list
Hello,

I have to make some code that convert a binary to a decimal and im not
allowed to use the convert methods that Pharo has.

So I have written this :


decimalFromBinary: aString
     | result isValid |
     isValid = self isValidBinary: aString.
     isValid
         ifTrue: [ result := 0.
             aString reverse
                 withIndexDo:
                     [ :digit :index | result := result + (digit
digitValue * (2 raisedTo: index - 1)) ].
             ^ result ]
         ifFalse: [ ^ nil ]

isValidBinary: aString
     ^ aString allSatisfy: [ :c | c = 0 or: [ c = 1 ] ]


but on the first method I see a message that the temp variables are read
before written.
and the second one I see a message that I use a or instead of searching
literals.

Where did  I think wrong here ?

Roelof


Reply | Threaded
Open this post in threaded view
|

Re: mentor question 2.

gcotelli
In the first method you aren't doing an assignment, are sending the message = to the temp isValid, if you change the method to:
decimalFromBinary: aString
     | result isValid |
     isValid := self isValidBinary: aString.
     isValid
         ifTrue: [ result := 0.
             aString reverse
                 withIndexDo:
                     [ :digit :index | result := result + (digit
digitValue * (2 raisedTo: index - 1)) ].
             ^ result ]
         ifFalse: [ ^ nil ]
it should work.

In the second one you're comparing characters with numbers, this will always return false because the number 0 is not the same as the character 0.
Use
isValidBinary: aString
     ^ aString allSatisfy: [ :c | c = $0 or: [ c = $1 ] ]
or something like

isValidBinary: aString
     ^ aString allSatisfy: [ :c | '01' includes: c ]

On Sun, Apr 26, 2020 at 2:52 PM Roelof Wobben via Pharo-users <[hidden email]> wrote:
Hello,

I have to make some code that convert a binary to a decimal and im not
allowed to use the convert methods that Pharo has.

So I have written this :


decimalFromBinary: aString
     | result isValid |
     isValid = self isValidBinary: aString.
     isValid
         ifTrue: [ result := 0.
             aString reverse
                 withIndexDo:
                     [ :digit :index | result := result + (digit
digitValue * (2 raisedTo: index - 1)) ].
             ^ result ]
         ifFalse: [ ^ nil ]

isValidBinary: aString
     ^ aString allSatisfy: [ :c | c = 0 or: [ c = 1 ] ]


but on the first method I see a message that the temp variables are read
before written.
and the second one I see a message that I use a or instead of searching
literals.

Where did  I think wrong here ?

Roelof


Reply | Threaded
Open this post in threaded view
|

Re: mentor question 2.

Sven Van Caekenberghe-2


> On 26 Apr 2020, at 20:10, Gabriel Cotelli <[hidden email]> wrote:
>
> In the first method you aren't doing an assignment, are sending the message = to the temp isValid, if you change the method to:
> decimalFromBinary: aString
>      | result isValid |
>      isValid := self isValidBinary: aString.
>      isValid
>          ifTrue: [ result := 0.
>              aString reverse
>                  withIndexDo:
>                      [ :digit :index | result := result + (digit
> digitValue * (2 raisedTo: index - 1)) ].
>              ^ result ]
>          ifFalse: [ ^ nil ]
> it should work.

There is a #reverseWithIndexDo: method

Also, #digitValue might be considered a builtin method that you are not allowed to use. Since you already did the #isValidBinary: test, you could say

  (digit charCode - $0 charCode)

> In the second one you're comparing characters with numbers, this will always return false because the number 0 is not the same as the character 0.
> Use
> isValidBinary: aString
>      ^ aString allSatisfy: [ :c | c = $0 or: [ c = $1 ] ]
> or something like
>
> isValidBinary: aString
>      ^ aString allSatisfy: [ :c | '01' includes: c ]
>
> On Sun, Apr 26, 2020 at 2:52 PM Roelof Wobben via Pharo-users <[hidden email]> wrote:
> Hello,
>
> I have to make some code that convert a binary to a decimal and im not
> allowed to use the convert methods that Pharo has.
>
> So I have written this :
>
>
> decimalFromBinary: aString
>      | result isValid |
>      isValid = self isValidBinary: aString.
>      isValid
>          ifTrue: [ result := 0.
>              aString reverse
>                  withIndexDo:
>                      [ :digit :index | result := result + (digit
> digitValue * (2 raisedTo: index - 1)) ].
>              ^ result ]
>          ifFalse: [ ^ nil ]
>
> isValidBinary: aString
>      ^ aString allSatisfy: [ :c | c = 0 or: [ c = 1 ] ]
>
>
> but on the first method I see a message that the temp variables are read
> before written.
> and the second one I see a message that I use a or instead of searching
> literals.
>
> Where did  I think wrong here ?
>
> Roelof
>
>


Reply | Threaded
Open this post in threaded view
|

Re: mentor question 2.

Pharo Smalltalk Users mailing list
Op 26-4-2020 om 20:17 schreef Sven Van Caekenberghe:

>
>> On 26 Apr 2020, at 20:10, Gabriel Cotelli <[hidden email]> wrote:
>>
>> In the first method you aren't doing an assignment, are sending the message = to the temp isValid, if you change the method to:
>> decimalFromBinary: aString
>>       | result isValid |
>>       isValid := self isValidBinary: aString.
>>       isValid
>>           ifTrue: [ result := 0.
>>               aString reverse
>>                   withIndexDo:
>>                       [ :digit :index | result := result + (digit
>> digitValue * (2 raisedTo: index - 1)) ].
>>               ^ result ]
>>           ifFalse: [ ^ nil ]
>> it should work.
> There is a #reverseWithIndexDo: method
>
> Also, #digitValue might be considered a builtin method that you are not allowed to use. Since you already did the #isValidBinary: test, you could say
>
>    (digit charCode - $0 charCode)
>
>> In the second one you're comparing characters with numbers, this will always return false because the number 0 is not the same as the character 0.
>> Use
>> isValidBinary: aString
>>       ^ aString allSatisfy: [ :c | c = $0 or: [ c = $1 ] ]
>> or something like
>>
>> isValidBinary: aString
>>       ^ aString allSatisfy: [ :c | '01' includes: c ]
>>
>> On Sun, Apr 26, 2020 at 2:52 PM Roelof Wobben via Pharo-users <[hidden email]> wrote:
>> Hello,
>>
>> I have to make some code that convert a binary to a decimal and im not
>> allowed to use the convert methods that Pharo has.
>>
>> So I have written this :
>>
>>
>> decimalFromBinary: aString
>>       | result isValid |
>>       isValid = self isValidBinary: aString.
>>       isValid
>>           ifTrue: [ result := 0.
>>               aString reverse
>>                   withIndexDo:
>>                       [ :digit :index | result := result + (digit
>> digitValue * (2 raisedTo: index - 1)) ].
>>               ^ result ]
>>           ifFalse: [ ^ nil ]
>>
>> isValidBinary: aString
>>       ^ aString allSatisfy: [ :c | c = 0 or: [ c = 1 ] ]
>>
>>
>> but on the first method I see a message that the temp variables are read
>> before written.
>> and the second one I see a message that I use a or instead of searching
>> literals.
>>
>> Where did  I think wrong here ?
>>
>> Roelof
>>
>>
>


Thanks,  this problem is solved.

Roelof

Reply | Threaded
Open this post in threaded view
|

Re: mentor question 2.

Peter Kenny

Roelof

 

You maybe have enough mentors already, but there are some important features of this problem which can be of use in future. Your code solves the problem, but it could be improved in two ways, which make it simpler and clearer.

 

  1. What is the purpose of the local variable ‘isValid’? It provides somewhere to remember the result of your validity test. But you have no need to remember it; all you want to do is evaluate it, use it to determine the course of the program and then forget it. If you write:

(self isValidBinary: aString)

         ifTrue: [….]

               ifFalse: [^nil]

you will achieve the same result without introducing the local variable. It probably reads more clearly, revealing the intention of the code. (You can simplify further by replacing the ifFalse: clause with an unconditional ^nil; if the code reaches this point, we know it’s false.) The general principle is: only introduce a named local variable if you need to remember something for later re-use.

 

  1. It is not necessary to reverse the string in order to evaluate it; it is in fact easier to evaluate it from left to right. You have used the withIndexDo: method to supply the index and know the appropriate power of two to use. Instead you could multiply by two as you process each digit, which will automatically use the right power of two without ever needing to know what it is. You could write:

result := 0.

aString do:

               [:digit| result := result * 2 + digit digitValue].

^result.

This is a special case of a very useful general procedure for evaluating a polynomial. The digits of a binary number are the coefficients of a polynomial in x which is evaluated at x=2; similarly, a decimal number is a polynomial evaluated at x=10.

 

I hope this is helpful.

 

Peter Kenny

 

From: Pharo-users <[hidden email]> On Behalf Of Roelof Wobben via Pharo-users
Sent: 26 April 2020 19:52
To: [hidden email]
Cc: Roelof Wobben <[hidden email]>
Subject: Re: [Pharo-users] mentor question 2.

 

Op 26-4-2020 om 20:17 schreef Sven Van Caekenberghe:

> 

>> On 26 Apr 2020, at 20:10, Gabriel Cotelli <[hidden email]> wrote:

>> 

>> In the first method you aren't doing an assignment, are sending the message = to the temp isValid, if you change the method to:

>> decimalFromBinary: aString

>>       | result isValid |

>>       isValid := self isValidBinary: aString.

>>       isValid

>>           ifTrue: [ result := 0.

>>               aString reverse

>>                   withIndexDo:

>>                       [ :digit :index | result := result + (digit

>> digitValue * (2 raisedTo: index - 1)) ].

>>               ^ result ]

>>           ifFalse: [ ^ nil ]

>> it should work.

> There is a #reverseWithIndexDo: method

> 

> Also, #digitValue might be considered a builtin method that you are

> not allowed to use. Since you already did the #isValidBinary: test,

> you could say

> 

>    (digit charCode - $0 charCode)

> 

>> In the second one you're comparing characters with numbers, this will always return false because the number 0 is not the same as the character 0.

>> Use

>> isValidBinary: aString

>>       ^ aString allSatisfy: [ :c | c = $0 or: [ c = $1 ] ] or

>> something like

>> 

>> isValidBinary: aString

>>       ^ aString allSatisfy: [ :c | '01' includes: c ]

>> 

>> On Sun, Apr 26, 2020 at 2:52 PM Roelof Wobben via Pharo-users <[hidden email]> wrote:

>> Hello,

>> 

>> I have to make some code that convert a binary to a decimal and im

>> not allowed to use the convert methods that Pharo has.

>> 

>> So I have written this :

>> 

>> 

>> decimalFromBinary: aString

>>       | result isValid |

>>       isValid = self isValidBinary: aString.

>>       isValid

>>           ifTrue: [ result := 0.

>>               aString reverse

>>                   withIndexDo:

>>                       [ :digit :index | result := result + (digit

>> digitValue * (2 raisedTo: index - 1)) ].

>>               ^ result ]

>>           ifFalse: [ ^ nil ]

>> 

>> isValidBinary: aString

>>       ^ aString allSatisfy: [ :c | c = 0 or: [ c = 1 ] ]

>> 

>> 

>> but on the first method I see a message that the temp variables are

>> read before written.

>> and the second one I see a message that I use a or instead of

>> searching literals.

>> 

>> Where did  I think wrong here ?

>> 

>> Roelof

>> 

>> 

> 

 

 

Thanks,  this problem is solved.

 

Roelof

 

 

 

Reply | Threaded
Open this post in threaded view
|

Re: mentor question 2.

Pharo Smalltalk Users mailing list
Op 27-4-2020 om 13:16 schreef PBKResearch:

Roelof

 

You maybe have enough mentors already, but there are some important features of this problem which can be of use in future. Your code solves the problem, but it could be improved in two ways, which make it simpler and clearer.

 

  1. What is the purpose of the local variable ‘isValid’? It provides somewhere to remember the result of your validity test. But you have no need to remember it; all you want to do is evaluate it, use it to determine the course of the program and then forget it. If you write:

(self isValidBinary: aString)

         ifTrue: [….]

               ifFalse: [^nil]

you will achieve the same result without introducing the local variable. It probably reads more clearly, revealing the intention of the code. (You can simplify further by replacing the ifFalse: clause with an unconditional ^nil; if the code reaches this point, we know it’s false.) The general principle is: only introduce a named local variable if you need to remember something for later re-use.

 

  1. It is not necessary to reverse the string in order to evaluate it; it is in fact easier to evaluate it from left to right. You have used the withIndexDo: method to supply the index and know the appropriate power of two to use. Instead you could multiply by two as you process each digit, which will automatically use the right power of two without ever needing to know what it is. You could write:

result := 0.

aString do:

               [:digit| result := result * 2 + digit digitValue].

^result.

This is a special case of a very useful general procedure for evaluating a polynomial. The digits of a binary number are the coefficients of a polynomial in x which is evaluated at x=2; similarly, a decimal number is a polynomial evaluated at x=10.

 

I hope this is helpful.

 

Peter Kenny

 

From: Pharo-users [hidden email] On Behalf Of Roelof Wobben via Pharo-users
Sent: 26 April 2020 19:52
To: [hidden email]
Cc: Roelof Wobben [hidden email]
Subject: Re: [Pharo-users] mentor question 2.

 

Op 26-4-2020 om 20:17 schreef Sven Van Caekenberghe:

> 

>> On 26 Apr 2020, at 20:10, Gabriel Cotelli <[hidden email]> wrote:

>> 

>> In the first method you aren't doing an assignment, are sending the message = to the temp isValid, if you change the method to:

>> decimalFromBinary: aString

>>       | result isValid |

>>       isValid := self isValidBinary: aString.

>>       isValid

>>           ifTrue: [ result := 0.

>>               aString reverse

>>                   withIndexDo:

>>                       [ :digit :index | result := result + (digit

>> digitValue * (2 raisedTo: index - 1)) ].

>>               ^ result ]

>>           ifFalse: [ ^ nil ]

>> it should work.

> There is a #reverseWithIndexDo: method

> 

> Also, #digitValue might be considered a builtin method that you are

> not allowed to use. Since you already did the #isValidBinary: test,

> you could say

> 

>    (digit charCode - $0 charCode)

> 

>> In the second one you're comparing characters with numbers, this will always return false because the number 0 is not the same as the character 0.

>> Use

>> isValidBinary: aString

>>       ^ aString allSatisfy: [ :c | c = $0 or: [ c = $1 ] ] or

>> something like

>> 

>> isValidBinary: aString

>>       ^ aString allSatisfy: [ :c | '01' includes: c ]

>> 

>> On Sun, Apr 26, 2020 at 2:52 PM Roelof Wobben via Pharo-users <[hidden email]> wrote:

>> Hello,

>> 

>> I have to make some code that convert a binary to a decimal and im

>> not allowed to use the convert methods that Pharo has.

>> 

>> So I have written this :

>> 

>> 

>> decimalFromBinary: aString

>>       | result isValid |

>>       isValid = self isValidBinary: aString.

>>       isValid

>>           ifTrue: [ result := 0.

>>               aString reverse

>>                   withIndexDo:

>>                       [ :digit :index | result := result + (digit

>> digitValue * (2 raisedTo: index - 1)) ].

>>               ^ result ]

>>           ifFalse: [ ^ nil ]

>> 

>> isValidBinary: aString

>>       ^ aString allSatisfy: [ :c | c = 0 or: [ c = 1 ] ]

>> 

>> 

>> but on the first method I see a message that the temp variables are

>> read before written.

>> and the second one I see a message that I use a or instead of

>> searching literals.

>> 

>> Where did  I think wrong here ?

>> 

>> Roelof

>> 

>> 

> 

 

 

Thanks,  this problem is solved.

 

Roelof

 

 

 


Thanks for the feedback.
The parts of  polynomial I do not get but that is oke.
I think I needed a higher degree of maths then I have now.
Never reached a university level.

Roelof

Reply | Threaded
Open this post in threaded view
|

Re: mentor question 2.

Peter Kenny

Roelof

 

Sorry if I introduced unnecessary technicalities. Your solution to the problem was in fact using a polynomial explicitly; that’s what all the powers of 2 were doing. But as long as you can follow how my version works, that’s all that matters.

 

Peter

 

From: Pharo-users <[hidden email]> On Behalf Of Roelof Wobben via Pharo-users
Sent: 27 April 2020 12:23
To: [hidden email]
Cc: Roelof Wobben <[hidden email]>
Subject: Re: [Pharo-users] mentor question 2.

 

Op 27-4-2020 om 13:16 schreef PBKResearch:

Roelof

 

You maybe have enough mentors already, but there are some important features of this problem which can be of use in future. Your code solves the problem, but it could be improved in two ways, which make it simpler and clearer.

 

  1. What is the purpose of the local variable ‘isValid’? It provides somewhere to remember the result of your validity test. But you have no need to remember it; all you want to do is evaluate it, use it to determine the course of the program and then forget it. If you write:

(self isValidBinary: aString)

         ifTrue: [….]

               ifFalse: [^nil]

you will achieve the same result without introducing the local variable. It probably reads more clearly, revealing the intention of the code. (You can simplify further by replacing the ifFalse: clause with an unconditional ^nil; if the code reaches this point, we know it’s false.) The general principle is: only introduce a named local variable if you need to remember something for later re-use.

 

  1. It is not necessary to reverse the string in order to evaluate it; it is in fact easier to evaluate it from left to right. You have used the withIndexDo: method to supply the index and know the appropriate power of two to use. Instead you could multiply by two as you process each digit, which will automatically use the right power of two without ever needing to know what it is. You could write:

result := 0.

aString do:

               [:digit| result := result * 2 + digit digitValue].

^result.

This is a special case of a very useful general procedure for evaluating a polynomial. The digits of a binary number are the coefficients of a polynomial in x which is evaluated at x=2; similarly, a decimal number is a polynomial evaluated at x=10.

 

I hope this is helpful.

 

Peter Kenny

 

From: Pharo-users [hidden email] On Behalf Of Roelof Wobben via Pharo-users
Sent: 26 April 2020 19:52
To: [hidden email]
Cc: Roelof Wobben [hidden email]
Subject: Re: [Pharo-users] mentor question 2.

 

Op 26-4-2020 om 20:17 schreef Sven Van Caekenberghe:

>> On 26 Apr 2020, at 20:10, Gabriel Cotelli <[hidden email]> wrote:

>> 

>> In the first method you aren't doing an assignment, are sending the message = to the temp isValid, if you change the method to:

>> decimalFromBinary: aString

>>       | result isValid |

>>       isValid := self isValidBinary: aString.

>>       isValid

>>           ifTrue: [ result := 0.

>>               aString reverse

>>                   withIndexDo:

>>                       [ :digit :index | result := result + (digit

>> digitValue * (2 raisedTo: index - 1)) ].

>>               ^ result ]

>>           ifFalse: [ ^ nil ]

>> it should work.

> There is a #reverseWithIndexDo: method

> Also, #digitValue might be considered a builtin method that you are

> not allowed to use. Since you already did the #isValidBinary: test,

> you could say

>    (digit charCode - $0 charCode)

>> In the second one you're comparing characters with numbers, this will always return false because the number 0 is not the same as the character 0.

>> Use

>> isValidBinary: aString

>>       ^ aString allSatisfy: [ :c | c = $0 or: [ c = $1 ] ] or

>> something like

>> 

>> isValidBinary: aString

>>       ^ aString allSatisfy: [ :c | '01' includes: c ]

>> 

>> On Sun, Apr 26, 2020 at 2:52 PM Roelof Wobben via Pharo-users <[hidden email]> wrote:

>> Hello,

>> 

>> I have to make some code that convert a binary to a decimal and im

>> not allowed to use the convert methods that Pharo has.

>> 

>> So I have written this :

>> 

>> 

>> decimalFromBinary: aString

>>       | result isValid |

>>       isValid = self isValidBinary: aString.

>>       isValid

>>           ifTrue: [ result := 0.

>>               aString reverse

>>                   withIndexDo:

>>                       [ :digit :index | result := result + (digit

>> digitValue * (2 raisedTo: index - 1)) ].

>>               ^ result ]

>>           ifFalse: [ ^ nil ]

>> 

>> isValidBinary: aString

>>       ^ aString allSatisfy: [ :c | c = 0 or: [ c = 1 ] ]

>> 

>> 

>> but on the first method I see a message that the temp variables are

>> read before written.

>> and the second one I see a message that I use a or instead of

>> searching literals.

>> 

>> Where did  I think wrong here ?

>> 

>> Roelof

>> 

>> 

 

 

Thanks,  this problem is solved.

 

Roelof

 

 

 


Thanks for the feedback.
The parts of  polynomial I do not get but that is oke.
I think I needed a higher degree of maths then I have now.
Never reached a university level.

Roelof

 

Reply | Threaded
Open this post in threaded view
|

Re: mentor question 2.

Pharo Smalltalk Users mailing list
Not any problems.
I take the points out of it and forget the things I do not understand.

Roelof


Op 27-4-2020 om 13:50 schreef PBKResearch:

Roelof

 

Sorry if I introduced unnecessary technicalities. Your solution to the problem was in fact using a polynomial explicitly; that’s what all the powers of 2 were doing. But as long as you can follow how my version works, that’s all that matters.

 

Peter

 

From: Pharo-users [hidden email] On Behalf Of Roelof Wobben via Pharo-users
Sent: 27 April 2020 12:23
To: [hidden email]
Cc: Roelof Wobben [hidden email]
Subject: Re: [Pharo-users] mentor question 2.

 

Op 27-4-2020 om 13:16 schreef PBKResearch:

Roelof

 

You maybe have enough mentors already, but there are some important features of this problem which can be of use in future. Your code solves the problem, but it could be improved in two ways, which make it simpler and clearer.

 

  1. What is the purpose of the local variable ‘isValid’? It provides somewhere to remember the result of your validity test. But you have no need to remember it; all you want to do is evaluate it, use it to determine the course of the program and then forget it. If you write:

(self isValidBinary: aString)

         ifTrue: [….]

               ifFalse: [^nil]

you will achieve the same result without introducing the local variable. It probably reads more clearly, revealing the intention of the code. (You can simplify further by replacing the ifFalse: clause with an unconditional ^nil; if the code reaches this point, we know it’s false.) The general principle is: only introduce a named local variable if you need to remember something for later re-use.

 

  1. It is not necessary to reverse the string in order to evaluate it; it is in fact easier to evaluate it from left to right. You have used the withIndexDo: method to supply the index and know the appropriate power of two to use. Instead you could multiply by two as you process each digit, which will automatically use the right power of two without ever needing to know what it is. You could write:

result := 0.

aString do:

               [:digit| result := result * 2 + digit digitValue].

^result.

This is a special case of a very useful general procedure for evaluating a polynomial. The digits of a binary number are the coefficients of a polynomial in x which is evaluated at x=2; similarly, a decimal number is a polynomial evaluated at x=10.

 

I hope this is helpful.

 

Peter Kenny

 

From: Pharo-users [hidden email] On Behalf Of Roelof Wobben via Pharo-users
Sent: 26 April 2020 19:52
To: [hidden email]
Cc: Roelof Wobben [hidden email]
Subject: Re: [Pharo-users] mentor question 2.

 

Op 26-4-2020 om 20:17 schreef Sven Van Caekenberghe:

>> On 26 Apr 2020, at 20:10, Gabriel Cotelli <[hidden email]> wrote:

>> 

>> In the first method you aren't doing an assignment, are sending the message = to the temp isValid, if you change the method to:

>> decimalFromBinary: aString

>>       | result isValid |

>>       isValid := self isValidBinary: aString.

>>       isValid

>>           ifTrue: [ result := 0.

>>               aString reverse

>>                   withIndexDo:

>>                       [ :digit :index | result := result + (digit

>> digitValue * (2 raisedTo: index - 1)) ].

>>               ^ result ]

>>           ifFalse: [ ^ nil ]

>> it should work.

> There is a #reverseWithIndexDo: method

> Also, #digitValue might be considered a builtin method that you are

> not allowed to use. Since you already did the #isValidBinary: test,

> you could say

>    (digit charCode - $0 charCode)

>> In the second one you're comparing characters with numbers, this will always return false because the number 0 is not the same as the character 0.

>> Use

>> isValidBinary: aString

>>       ^ aString allSatisfy: [ :c | c = $0 or: [ c = $1 ] ] or

>> something like

>> 

>> isValidBinary: aString

>>       ^ aString allSatisfy: [ :c | '01' includes: c ]

>> 

>> On Sun, Apr 26, 2020 at 2:52 PM Roelof Wobben via Pharo-users <[hidden email]> wrote:

>> Hello,

>> 

>> I have to make some code that convert a binary to a decimal and im

>> not allowed to use the convert methods that Pharo has.

>> 

>> So I have written this :

>> 

>> 

>> decimalFromBinary: aString

>>       | result isValid |

>>       isValid = self isValidBinary: aString.

>>       isValid

>>           ifTrue: [ result := 0.

>>               aString reverse

>>                   withIndexDo:

>>                       [ :digit :index | result := result + (digit

>> digitValue * (2 raisedTo: index - 1)) ].

>>               ^ result ]

>>           ifFalse: [ ^ nil ]

>> 

>> isValidBinary: aString

>>       ^ aString allSatisfy: [ :c | c = 0 or: [ c = 1 ] ]

>> 

>> 

>> but on the first method I see a message that the temp variables are

>> read before written.

>> and the second one I see a message that I use a or instead of

>> searching literals.

>> 

>> Where did  I think wrong here ?

>> 

>> Roelof

>> 

>> 

 

 

Thanks,  this problem is solved.

 

Roelof

 

 

 


Thanks for the feedback.
The parts of  polynomial I do not get but that is oke.
I think I needed a higher degree of maths then I have now.
Never reached a university level.

Roelof

 


Reply | Threaded
Open this post in threaded view
|

Re: mentor question 2.

Richard Sargent
Administrator
In reply to this post by Pharo Smalltalk Users mailing list
On Sun, Apr 26, 2020 at 10:11 AM Roelof Wobben via Pharo-users <[hidden email]> wrote:
Hello,

I have to make some code that convert a binary to a decimal and im not
allowed to use the convert methods that Pharo has.

So I have written this :


decimalFromBinary: aString
     | result isValid |
     isValid = self isValidBinary: aString.
     isValid
         ifTrue: [ result := 0.
             aString reverse
                 withIndexDo:
                     [ :digit :index | result := result + (digit
digitValue * (2 raisedTo: index - 1)) ].
             ^ result ]
         ifFalse: [ ^ nil ]

isValidBinary: aString
     ^ aString allSatisfy: [ :c | c = 0 or: [ c = 1 ] ]

Others have answered your question, so I'll limit my response to other aspects of this exercise.

Answering nil is a bad practice, in general. There will be occasions where it is correct, if non-existence really is part of the model. Answering nil may have been a requirement of the exercise and if that's the case, you have no choice. By answering nil, you force every sender to check for nil rather than being able to rely on receiving an Integer result. Even with method selectors that tell the reader a nil can be the result, too many people will fail to check the result before using it. In general, I prefer to have the validation throw an exception.
e.g.
validateBinaryString: aString
    (aString allSatisfy: [:each | '01' includes: each])
        ifFalse: [self error: aString printString, ' is not a valid representation of a binary number'].

The method simply returns self if it succeeds or throws an exception is the string is invalid. And likewise, the conversion method itself only answers the integer corresponding to a valid binary string and does not answer (anything) for an invlaid one.


There is another pattern that could be used, but this particular problem seems (to me) best suited to an exception. A common pattern in Smalltalk, one of its greatest innovations, is the #ifAbsent: keyword and others like it. When you send #at:ifAbsent:, *you* tell the receiver to give you what you ask for *and* you tell it what *you* want to do if it cannot. There are infinitely many possible keywords like #ifAbsent:, as you can make up any one that suits your needs.

If you look at the implementation of #at: or #detect:, you will see that they are implemented using #at:ifAbsent: and #detect:ifNone: and that the block provided for the second argument is one which throws an error. Given all that, perhaps the "best" implementation of your solution would be to have "decimalFromBinary: aString" implemented in terms of a "decimalFromBinary: aString ifUnable: aBlock".



but on the first method I see a message that the temp variables are read
before written.
and the second one I see a message that I use a or instead of searching
literals.

Where did  I think wrong here ?

Roelof


Reply | Threaded
Open this post in threaded view
|

Re: mentor question 2.

Richard O'Keefe
In reply to this post by Pharo Smalltalk Users mailing list
String>>
  isValidBinary
    ^self allSatisfy: [:each | each = $0 or: [each = $1]]

 binaryToDecimal
    "The caller should ensure that self isValidBinary before calling this."
    ^self inject: 0 into: [:acc :each |
      acc*2 + (each codePoint - $0 codePoint)]


On Mon, 27 Apr 2020 at 06:05, Roelof Wobben via Pharo-users
<[hidden email]> wrote:

>
> Hello,
>
> I have to make some code that convert a binary to a decimal and im not
> allowed to use the convert methods that Pharo has.
>
> So I have written this :
>
>
> decimalFromBinary: aString
>      | result isValid |
>      isValid = self isValidBinary: aString.
>      isValid
>          ifTrue: [ result := 0.
>              aString reverse
>                  withIndexDo:
>                      [ :digit :index | result := result + (digit
> digitValue * (2 raisedTo: index - 1)) ].
>              ^ result ]
>          ifFalse: [ ^ nil ]
>
> isValidBinary: aString
>      ^ aString allSatisfy: [ :c | c = 0 or: [ c = 1 ] ]
>
>
> but on the first method I see a message that the temp variables are read
> before written.
> and the second one I see a message that I use a or instead of searching
> literals.
>
> Where did  I think wrong here ?
>
> Roelof
>
>