[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
|