On Mon, 28 Jun 2010, Sven Van Caekenberghe wrote:

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

You can exchange the temp for a message send this way:

self socket ifNotNil: #closeAndDestroy.

>
> could become a method like #closeSocket.

If there's #closeSocket, you can also simplify the code:

SocketEndpointBottom>>#send: data

 	self isConnected ifFalse: [ ^self closeSocket ].
 	[ self socket sendSomeData: data ]
 		on: Error
 		do: [ :exception |
 			self closeSocket.
 			SSLSendError signal: exception messageText ]


>
> 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 ;-)

Copying from 1 to something, can be simplified by using #first::
[ buffer first: (self socket receiveDataInto: buffer) ] on: Error do: [ ]

Though creating intermediate copies usually results in performance loss.


Levente

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