Smalltalk › Squeak › Squeak - Dev

code formatting

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

13 messages Options

Rob Withers

code formatting

I found some code changes made to code I wrote several years ago. I am
thrilled someone has been working with it. However, some of the changes
were cosmetic and I wondered what people thought was better formatting: A or
B, C or D.

SocketEndpointBottom>>#send:

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

OR
case B:
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]
]

SocketEndpointBottom>>#getData

case C:
SocketEndpointBottom>>#getData

| buf count |
Processor yield.
buf := ByteArray new: 18432.
[self socket dataAvailable
ifTrue:
[[count := self socket receiveDataInto: buf.
^buf copyFrom: 1 to: count]
on: Error
do: [:ex | ^ nil]]
ifFalse:
[[self socket waitForDataFor: 10] on: Error do: [:e | ]].
self isConnected]
whileTrue.
^nil

OR
case D:
SocketEndpointBottom>>#getData

| buf count |
Processor yield.
buf := ByteArray new: 18432.
[
self socket dataAvailable
ifTrue: [
[
count := self socket receiveDataInto: buf.
^buf copyFrom: 1 to: count
]
on: Error
do: [:ex |
^ nil
]
]
ifFalse: [
[self socket waitForDataFor: 10]
on: Error
do: [:e | ]
].
self isConnected
]
whileTrue.
^nil

Sven Van Caekenberghe

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

Rob Withers

Re: [SPAM] [squeak-dev] code formatting

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" <[hidden email]>
Sent: Monday, June 28, 2010 3:30 AM
To: "The general-purpose Squeak developers list"
<[hidden email]>
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
>
>
>

Sven Van Caekenberghe

Re: [SPAM] [squeak-dev] code formatting

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

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

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" <[hidden email]>
> Sent: Monday, June 28, 2010 3:30 AM
> To: "The general-purpose Squeak developers list" <[hidden email]>
> 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
>>
>>
>

Nicolas Cellier

Re: [SPAM] [squeak-dev] code formatting

2010/6/28 Sven Van Caekenberghe <[hidden email]>:

> 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" <[hidden email]>
>> Sent: Monday, June 28, 2010 3:30 AM
>> To: "The general-purpose Squeak developers list" <[hidden email]>
>> 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
>>>
>>>
>>
>
>
>

Sven Van Caekenberghe

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 <[hidden email]>:
>> 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" <[hidden email]>
>>> Sent: Monday, June 28, 2010 3:30 AM
>>> To: "The general-purpose Squeak developers list" <[hidden email]>
>>> 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
>>>>
>>>>
>>>
>>
>>
>>
>

Julian Fitzell-2

Re: [SPAM] [squeak-dev] code formatting

On Mon, Jun 28, 2010 at 12:42 PM, Sven Van Caekenberghe <[hidden email]> wrote:
> I think on: Error do: [ ] and on: Error do: [ nil ] are the same.

I believe that is not necessarily the case if you're writing portably.
I can't quite recall where the issue comes up, but I remember learning
that you cannot count on the value of an empty block.

Also, in reference to your comment about niladic on:do: blocks, some
platforms do not support that. If you are writing portable code, you
must use a monadic block.

on: Error do: [ :error | nil ]

Julian

Levente Uzonyi-2

Re: [SPAM] [squeak-dev] code formatting

In reply to this post by Sven Van Caekenberghe

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

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 <[hidden email]>:
>>> 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" <[hidden email]>
>>>> Sent: Monday, June 28, 2010 3:30 AM
>>>> To: "The general-purpose Squeak developers list" <[hidden email]>
>>>> 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
>>>>>
>>>>>
>>>>
>>>
>>>
>>>
>>
>
>
>

Bert Freudenberg

Re: code formatting

On 28.06.2010, at 16:00, Levente Uzonyi wrote:

> You can exchange the temp for a message send this way:
>
> self socket ifNotNil: #closeAndDestroy.

No, you can't.

- Bert -

Frank Shearar

Re: [SPAM] [squeak-dev] code formatting

In reply to this post by Sven Van Caekenberghe

On 2010/06/28 09:30, Sven Van Caekenberghe wrote:

I'll second this, on all counts: conserves vertical space, the "] ]"
pattern makes it easier to count brackets, and Seaside uses this style.

It also looks more "lispy" having all the closing ]s on one line. I do
find it difficult to count brackets: shout's highlighting of the pairs
of brackets is a bit subtle for me. (To which the obvious response is:
submit a patch!)

frank

Levente Uzonyi-2

Re: code formatting

In reply to this post by Bert Freudenberg

On Mon, 28 Jun 2010, Bert Freudenberg wrote:

> On 28.06.2010, at 16:00, Levente Uzonyi wrote:
>
>> You can exchange the temp for a message send this way:
>>
>> self socket ifNotNil: #closeAndDestroy.
>
> No, you can't.

I see, the compiler doesn't like it. It's a minor bug. But even if that's
fixed it won't work, because symbols only understand #value: and
#ifNotNil: sends #valueWithPossibleArgs:.

| message |
message := #squared.
5 ifNotNil: message.

This is another strange case:

| block |
block := [ :a :b | a ].
5 ifNotNil: block. "===> 5"

I wonder if there's code that relies on this feature. If not, then we
could replace some #valueWithPossibleArgs: sends to #cull:.

Would it make sense to implement #valueWithPossibleArgs: (and friends) and
#cull: (and friends) for symbols?

Levente

>
> - Bert -
>
>
>
>

Rob Withers

Re: [SPAM] [squeak-dev] code formatting

In reply to this post by Sven Van Caekenberghe

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" <[hidden email]>
Sent: Monday, June 28, 2010 7:42 AM
To: "The general-purpose Squeak developers list"
<[hidden email]>
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 <[hidden email]>:
>>> 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" <[hidden email]>
>>>> Sent: Monday, June 28, 2010 3:30 AM
>>>> To: "The general-purpose Squeak developers list"
>>>> <[hidden email]>
>>>> 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
>>>>>
>>>>>
>>>>
>>>
>>>
>>>
>>
>
>

Travis Griggs-4

Re: code formatting

In reply to this post by Levente Uzonyi-2

On Jun 28, 2010, at 8:06 AM, Levente Uzonyi wrote:

> On Mon, 28 Jun 2010, Bert Freudenberg wrote:
>
>> On 28.06.2010, at 16:00, Levente Uzonyi wrote:
>>
>>> You can exchange the temp for a message send this way:
>>>
>>> self socket ifNotNil: #closeAndDestroy.
>>
>> No, you can't.
>
> I see, the compiler doesn't like it. It's a minor bug. But even if
> that's fixed it won't work, because symbols only understand #value:
> and #ifNotNil: sends #valueWithPossibleArgs:.
>
> | message |
> message := #squared.
> 5 ifNotNil: message.
>
> This is another strange case:
>
> | block |
> block := [ :a :b | a ].
> 5 ifNotNil: block. "===> 5"
>
> I wonder if there's code that relies on this feature. If not, then
> we could replace some #valueWithPossibleArgs: sends to #cull:.
>
> Would it make sense to implement #valueWithPossibleArgs: (and
> friends) and #cull: (and friends) for symbols?

VisualWorks does so. I use the pattern frequently.

--
Travis Griggs
Objologist
Light travels faster than sound. This is why some people appear bright
until you hear them speak...