[SPAM] [squeak-dev] code formatting

Sven Van Caekenberghe sven at beta9.be
Mon Jun 28 11:42:45 UTC 2010


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