[SPAM] [squeak-dev] code formatting

Rob Withers reefedjib at yahoo.com
Mon Jun 28 22:19:47 UTC 2010


Nice comments, although the formatting has been lost on me.  I am sending 
this as plain text in hopes that it sticks the formatting.

1. I took the refactoring suggestions and extracted some methods.
2. I use sock as the block arg since I do have a socket ivar.
3. I use ex as an exception handler block var, since I always have and 
always will.  I refuse to type out exception.  ex means exception.
4. The thing I may find difficult to stick to is the space inside block open 
and close.   16 years of Smalltalking a particular way and it is hard to 
change a habit.  It may be more readable, I will give it that.  For finding 
matching braces, I just double click inside of one and it selects the block 
contents.
5. I am not sure I like the return in #receiveData.  I feel more comfortable 
with a return at the copy buffer and one in the exception handler.

I think we now have - with buffer instance variable initialized to 18432:

SocketEndpointBottom>>#getData
    Processor yield.
    [ self socket dataAvailable
        ifTrue: [^ self receiveData ]
        ifFalse: [ self waitForDataFor: 10 ].
    self isConnected ] whileTrue


SocketEndpointBottom>>#send: data
    self isConnected
        ifTrue: [ self sendSomeData: data ]
        ifFalse: [ self closeSocket ].


SocketEndpointBottom>>#receiveData
    | count |
    [ count := self socket receiveDataInto: self buffer.
    ^ self buffer copyFrom: 1 to: count ]
        on: Error
        do: [ :ex | ^ nil ]


SocketEndpointBottom>>#waitForDataFor: time
    [ self socket waitForDataFor: time ] on: Error do: [ :ex | nil ].


SocketEndpointBottom>>#sendSomeData: data
    [ self socket sendSomeData: data ]
        on: Error
        do: [ :ex |
            self closeSocket.
            SSLSendError signal: ex messageText ].


SocketEndpointBottom>>#closeSocket
    self socket ifNotNil: [ :sock | sock closeAndDestroy ].
    self socket: nil.


--------------------------------------------------
From: "Sven Van Caekenberghe" <sven at beta9.be>
Sent: Monday, June 28, 2010 7:42 AM
To: "The general-purpose Squeak developers list" 
<squeak-dev at lists.squeakfoundation.org>
Subject: Re: [SPAM] [squeak-dev] code formatting

> With Nicolas' remarks (both were valid I think) we now have:
>
> SocketEndpointBottom>>#send: data
> self isConnected
> ifTrue: [
> [ self socket sendSomeData: data ]
> on: Error
> do: [ :exception |
> self socket ifNotNil: [ :socket | socket closeAndDestroy ].
> SSLSendError signal: exception messageText ] ]
> ifFalse: [
> self socket ifNotNil: [ :socket | socket closeAndDestroy ] ]
>
> I used socket instead of sock, won't work if socket is an instance 
> variable.
>
> Now that we are rewriting instead of reformatting,
>
> self socket ifNotNil: [ :socket | socket closeAndDestroy ]
>
> could become a method like #closeSocket.
>
> SocketEndpointBottom>>#getData
> | buffer |
> Processor yield.
> buffer := ByteArray new: 18432.
> [ self socket dataAvailable
> ifTrue: [
> ^ [ | count |
> count := self socket receiveDataInto: buffer.
> buffer copyFrom: 1 to: count ] on: Error do: [ ] ]
> ifFalse: [
> [ self socket waitForDataFor: 10 ] on: Error do: [ ] ].
> self isConnected ] whileTrue
>
> I think on: Error do: [ ] and on: Error do: [ nil ] are the same.
>
> We could also write the count block as one line, no ?
>
> [ buffer copyFrom: 1 to: (self socket receiveDataInto: buffer) ] on: Error 
> do: [ ]
>
> But that might be pushing it since it is less clear ;-)
>
> There is also Socket>>#receiveData, along those lines we could add 
> Socket>>#receiveBinaryData.
>
> Sven
>
> On 28 Jun 2010, at 12:27, Nicolas Cellier wrote:
>
>> 2010/6/28 Sven Van Caekenberghe <sven at beta9.be>:
>>> Rob,
>>>
>>> Discussing code formatting style can go on forever, there just is no 
>>> single answer.
>>> When thinking about the C/D distinction, I maybe would change A a little 
>>> bit.
>>>
>>> I'm not always sure myself how to structure nested blocks, it depends on 
>>> the depth, line length, overall complexity and meaning, and even then 
>>> there could be many good ways to do it.
>>>
>>> case A:
>>>
>>> SocketEndpointBottom>>#send: data
>>>        self isConnected
>>>                ifTrue: [
>>>                        [ self socket sendSomeData: data ]
>>>                                on: Error
>>>                                do: [ :exception |
>>>                                        self socket notNil ifTrue: [ self 
>>> socket closeAndDestroy ].
>>>                                        SSLSendError signal: exception 
>>> messageText ] ]
>>>                ifFalse: [
>>>                        self socket notNilifTrue: [ self socket 
>>> closeAndDestroy ] ]
>>>
>>
>> Speaking of style I would write:
>>    self socket ifNotNil: [:sock | sock closeAndDestroy ]
>>
>>> case C:
>>>
>>> SocketEndpointBottom>>#getData
>>>        | buffer count |
>>>        Processor yield.
>>>        buffer := ByteArray new: 18432.
>>>        [ self socket dataAvailable
>>>                ifTrue: [
>>>                        [ count := self socket receiveDataInto: buffer.
>>>                        ^ buffer copyFrom: 1 to: count ] on: Error do: 
>>> [ ^ nil ] ]
>>>                ifFalse: [
>>>                        [ self socket waitForDataFor: 10 ] on: Error do: 
>>> [ ] ].
>>>         self isConnected ] whileTrue
>>>
>>
>> and also
>>                ifTrue: [
>>                       ^ [ | count |
>>                          count := self socket receiveDataInto: buffer.
>>                          buffer copyFrom: 1 to: count ] on: Error do: [ 
>> nil ] ]
>> I presume this piece of code is used in internal loops, so I would tend 
>> to avoid
>> - outer temp assignment.
>> - non local return,
>> Last one might not meet general agreement for readability reasons 
>> though...
>>
>> Just my cheap advices...
>>
>> Nicolas
>>
>>> I added spaces before returns to make them stand out more, renamed some 
>>> variables and intended the ifTrue:ifFalse's and #on:do:'s differently.
>>>
>>> In Pharo at least, an empty and no argument blocks work as argument for 
>>> the #on:do:.
>>>
>>> And as far as I understand Smalltalk, the last ^nil was unreachable.
>>>
>>> I don't want to sound as if I am in a position to tell you what's best, 
>>> if you did/are writing the SSL code than you probably know more than 
>>> enough about Smalltalk already ;-)
>>>
>>> Sven
>>>
>>> PS: the SSL code is really important, for both client and server sides 
>>> (the WebClient package could use it as well).
>>>
>>> On 28 Jun 2010, at 10:27, Rob Withers wrote:
>>>
>>>> I take it you like C over D as well...?
>>>>
>>>> I hadn't thought of adding a space.  Good idea!  A and C were my 
>>>> versions...
>>>>
>>>> Thanks,
>>>> Rob
>>>>
>>>>
>>>>
>>>> --------------------------------------------------
>>>> From: "Sven Van Caekenberghe" <sven at beta9.be>
>>>> Sent: Monday, June 28, 2010 3:30 AM
>>>> To: "The general-purpose Squeak developers list" 
>>>> <squeak-dev at lists.squeakfoundation.org>
>>>> Subject: Re: [SPAM] [squeak-dev] code formatting
>>>>
>>>>>
>>>>> On 27 Jun 2010, at 22:59, Rob Withers wrote:
>>>>>
>>>>>> case A:
>>>>>> SocketEndpointBottom>>#send: data
>>>>>> self isConnected
>>>>>> ifTrue: [[self socket sendSomeData: data]
>>>>>> on: Error
>>>>>> do: [:ex |
>>>>>> self socket notNil
>>>>>> ifTrue: [self socket closeAndDestroy].
>>>>>> SSLSendError signal: ex messageText]]
>>>>>> ifFalse: [self socket notNil
>>>>>> ifTrue: [self socket closeAndDestroy]]
>>>>>
>>>>> I like A the best, its the most compact and still sematically 
>>>>> structured.
>>>>> Personally I add extra space inbetween square brackets, like this:
>>>>>
>>>>> case A:
>>>>> SocketEndpointBottom>>#send: data
>>>>> self isConnected
>>>>> ifTrue: [ [ self socket sendSomeData: data ]
>>>>> on: Error
>>>>> do: [ :ex |
>>>>> self socket notNil
>>>>> ifTrue: [ self socket closeAndDestroy ].
>>>>> SSLSendError signal: ex messageText ] ]
>>>>> ifFalse: [ self socket notNil
>>>>> ifTrue: [ self socket closeAndDestroy ] ]
>>>>>
>>>>> It gives the code a bit more 'breathing room'.
>>>>> I picked this up from exiting code, I see it a lot in the Seaside code 
>>>>> base.
>>>>>
>>>>> Sven
>>>>>
>>>>>
>>>>
>>>
>>>
>>>
>>
>
> 



More information about the Squeak-dev mailing list