Code mystery

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Code mystery

Peter Kenny

Hello All

 

Following the discussion on dark mode, I was browsing the code on themes (in Moose 6.1 = Pharo 6.0, Latest update: #60486). In Class ThemeIcons, I found this method:

 

downloadTo: dir

               | zipArchive |

              

               zipArchive := self class destinationPath / (self name, '.zip').

               zipArchive exists

                              ifFalse: [

                                             ZnClient new

                                                            url: self url;

                                                            downloadTo: zipArchive ].

 

               ^ zipArchive

 

The mystery is that the argument dir is not referred to anywhere in the code. It probably works, because the only invocation of the method is from ThemeIcons>>downloadFromUrl, which sets the argument from self class destinationPath, and the code above recreates this as the path to zipArchive.

 

I thought I understood Smalltalk coding fairly well, but this really puzzles me. Why would anyone code like this? Shouldn’t it be picked up by a code critic? Or am I going crazy?

 

Any help gratefully received

 

Peter Kenny

Reply | Threaded
Open this post in threaded view
|

Re: Code mystery

kilon.alios
You are not going crazy that's an ugly method. Probably the author intended to use dir and the forgot about it and instead hard coded the dir path inside a class method. A mistake that can happen to anyone.

The method will have to be updated anyway because name is to be removed, so it won't work.

As always too much code too few people. If you think that's bad embrace yourself if you try to read Morphic code. Huge suffering for me when I tried to learn how the task bar works and apparently using it wrong it freezes the image.

Tons of Pharo code needs a clean but needs also a lot more people.
On Tue, 29 Aug 2017 at 01:52, PBKResearch <[hidden email]> wrote:

Hello All

 

Following the discussion on dark mode, I was browsing the code on themes (in Moose 6.1 = Pharo 6.0, Latest update: #60486). In Class ThemeIcons, I found this method:

 

downloadTo: dir

               | zipArchive |

              

               zipArchive := self class destinationPath / (self name, '.zip').

               zipArchive exists

                              ifFalse: [

                                             ZnClient new

                                                            url: self url;

                                                            downloadTo: zipArchive ].

 

               ^ zipArchive

 

The mystery is that the argument dir is not referred to anywhere in the code. It probably works, because the only invocation of the method is from ThemeIcons>>downloadFromUrl, which sets the argument from self class destinationPath, and the code above recreates this as the path to zipArchive.

 

I thought I understood Smalltalk coding fairly well, but this really puzzles me. Why would anyone code like this? Shouldn’t it be picked up by a code critic? Or am I going crazy?

 

Any help gratefully received

 

Peter Kenny

Reply | Threaded
Open this post in threaded view
|

Re: Code mystery

Stephane Ducasse-3
Can you open a bug entry?
I can tell you that we clean a lot but human activity will always
leads to glitches.

On Tue, Aug 29, 2017 at 1:23 AM, Dimitris Chloupis
<[hidden email]> wrote:

> You are not going crazy that's an ugly method. Probably the author intended
> to use dir and the forgot about it and instead hard coded the dir path
> inside a class method. A mistake that can happen to anyone.
>
> The method will have to be updated anyway because name is to be removed, so
> it won't work.
>
> As always too much code too few people. If you think that's bad embrace
> yourself if you try to read Morphic code. Huge suffering for me when I tried
> to learn how the task bar works and apparently using it wrong it freezes the
> image.
>
> Tons of Pharo code needs a clean but needs also a lot more people.
> On Tue, 29 Aug 2017 at 01:52, PBKResearch <[hidden email]> wrote:
>>
>> Hello All
>>
>>
>>
>> Following the discussion on dark mode, I was browsing the code on themes
>> (in Moose 6.1 = Pharo 6.0, Latest update: #60486). In Class ThemeIcons, I
>> found this method:
>>
>>
>>
>> downloadTo: dir
>>
>>                | zipArchive |
>>
>>
>>
>>                zipArchive := self class destinationPath / (self name,
>> '.zip').
>>
>>                zipArchive exists
>>
>>                               ifFalse: [
>>
>>                                              ZnClient new
>>
>>                                                             url: self url;
>>
>>                                                             downloadTo:
>> zipArchive ].
>>
>>
>>
>>                ^ zipArchive
>>
>>
>>
>> The mystery is that the argument dir is not referred to anywhere in the
>> code. It probably works, because the only invocation of the method is from
>> ThemeIcons>>downloadFromUrl, which sets the argument from self class
>> destinationPath, and the code above recreates this as the path to
>> zipArchive.
>>
>>
>>
>> I thought I understood Smalltalk coding fairly well, but this really
>> puzzles me. Why would anyone code like this? Shouldn’t it be picked up by a
>> code critic? Or am I going crazy?
>>
>>
>>
>> Any help gratefully received
>>
>>
>>
>> Peter Kenny

Reply | Threaded
Open this post in threaded view
|

Re: Code mystery

Peter Kenny
Stef

Done - https://pharo.fogbugz.com/f/cases/20363/ThemeIcons-downloadTo-has-an-argument-which-is-never-referenced-in-code

What really puzzled me is that this situation was not picked up by a code critic. 'Local variable declared and not referenced' is a common critic message, and this looks like a parallel situation. But I don't think this can be called a bug in the code critic system - can it?

Peter Kenny

-----Original Message-----
From: Pharo-users [mailto:[hidden email]] On Behalf Of Stephane Ducasse
Sent: 31 August 2017 19:16
To: Any question about pharo is welcome <[hidden email]>
Subject: Re: [Pharo-users] Code mystery

Can you open a bug entry?
I can tell you that we clean a lot but human activity will always leads to glitches.

On Tue, Aug 29, 2017 at 1:23 AM, Dimitris Chloupis <[hidden email]> wrote:

> You are not going crazy that's an ugly method. Probably the author
> intended to use dir and the forgot about it and instead hard coded the
> dir path inside a class method. A mistake that can happen to anyone.
>
> The method will have to be updated anyway because name is to be
> removed, so it won't work.
>
> As always too much code too few people. If you think that's bad
> embrace yourself if you try to read Morphic code. Huge suffering for
> me when I tried to learn how the task bar works and apparently using
> it wrong it freezes the image.
>
> Tons of Pharo code needs a clean but needs also a lot more people.
> On Tue, 29 Aug 2017 at 01:52, PBKResearch <[hidden email]> wrote:
>>
>> Hello All
>>
>>
>>
>> Following the discussion on dark mode, I was browsing the code on
>> themes (in Moose 6.1 = Pharo 6.0, Latest update: #60486). In Class
>> ThemeIcons, I found this method:
>>
>>
>>
>> downloadTo: dir
>>
>>                | zipArchive |
>>
>>
>>
>>                zipArchive := self class destinationPath / (self name,
>> '.zip').
>>
>>                zipArchive exists
>>
>>                               ifFalse: [
>>
>>                                              ZnClient new
>>
>>                                                             url: self
>> url;
>>
>>                                                             downloadTo:
>> zipArchive ].
>>
>>
>>
>>                ^ zipArchive
>>
>>
>>
>> The mystery is that the argument dir is not referred to anywhere in
>> the code. It probably works, because the only invocation of the
>> method is from
>> ThemeIcons>>downloadFromUrl, which sets the argument from self class
>> destinationPath, and the code above recreates this as the path to
>> zipArchive.
>>
>>
>>
>> I thought I understood Smalltalk coding fairly well, but this really
>> puzzles me. Why would anyone code like this? Shouldn’t it be picked
>> up by a code critic? Or am I going crazy?
>>
>>
>>
>> Any help gratefully received
>>
>>
>>
>> Peter Kenny


Reply | Threaded
Open this post in threaded view
|

Re: Code mystery

Stephane Ducasse-3
Yes I was thinking the same.

Stef

On Thu, Aug 31, 2017 at 8:54 PM, PBKResearch <[hidden email]> wrote:

> Stef
>
> Done - https://pharo.fogbugz.com/f/cases/20363/ThemeIcons-downloadTo-has-an-argument-which-is-never-referenced-in-code
>
> What really puzzled me is that this situation was not picked up by a code critic. 'Local variable declared and not referenced' is a common critic message, and this looks like a parallel situation. But I don't think this can be called a bug in the code critic system - can it?
>
> Peter Kenny
>
> -----Original Message-----
> From: Pharo-users [mailto:[hidden email]] On Behalf Of Stephane Ducasse
> Sent: 31 August 2017 19:16
> To: Any question about pharo is welcome <[hidden email]>
> Subject: Re: [Pharo-users] Code mystery
>
> Can you open a bug entry?
> I can tell you that we clean a lot but human activity will always leads to glitches.
>
> On Tue, Aug 29, 2017 at 1:23 AM, Dimitris Chloupis <[hidden email]> wrote:
>> You are not going crazy that's an ugly method. Probably the author
>> intended to use dir and the forgot about it and instead hard coded the
>> dir path inside a class method. A mistake that can happen to anyone.
>>
>> The method will have to be updated anyway because name is to be
>> removed, so it won't work.
>>
>> As always too much code too few people. If you think that's bad
>> embrace yourself if you try to read Morphic code. Huge suffering for
>> me when I tried to learn how the task bar works and apparently using
>> it wrong it freezes the image.
>>
>> Tons of Pharo code needs a clean but needs also a lot more people.
>> On Tue, 29 Aug 2017 at 01:52, PBKResearch <[hidden email]> wrote:
>>>
>>> Hello All
>>>
>>>
>>>
>>> Following the discussion on dark mode, I was browsing the code on
>>> themes (in Moose 6.1 = Pharo 6.0, Latest update: #60486). In Class
>>> ThemeIcons, I found this method:
>>>
>>>
>>>
>>> downloadTo: dir
>>>
>>>                | zipArchive |
>>>
>>>
>>>
>>>                zipArchive := self class destinationPath / (self name,
>>> '.zip').
>>>
>>>                zipArchive exists
>>>
>>>                               ifFalse: [
>>>
>>>                                              ZnClient new
>>>
>>>                                                             url: self
>>> url;
>>>
>>>                                                             downloadTo:
>>> zipArchive ].
>>>
>>>
>>>
>>>                ^ zipArchive
>>>
>>>
>>>
>>> The mystery is that the argument dir is not referred to anywhere in
>>> the code. It probably works, because the only invocation of the
>>> method is from
>>> ThemeIcons>>downloadFromUrl, which sets the argument from self class
>>> destinationPath, and the code above recreates this as the path to
>>> zipArchive.
>>>
>>>
>>>
>>> I thought I understood Smalltalk coding fairly well, but this really
>>> puzzles me. Why would anyone code like this? Shouldn’t it be picked
>>> up by a code critic? Or am I going crazy?
>>>
>>>
>>>
>>> Any help gratefully received
>>>
>>>
>>>
>>> Peter Kenny
>
>

Reply | Threaded
Open this post in threaded view
|

Re: Code mystery

Richard Sargent
Administrator
In reply to this post by Peter Kenny
I think the answer my lie in the general case and no one tried to address the complexity of the problem.

Consider an API that's implemented in a number of classes, classes which may not be in a hierarchy. Let's use #ifNil: as an example. 

Some implementers use the argument and others don't, so you cannot criticise the methods that don't.

It's much more difficult to recognize whether methods with the same name actually implement the same API or are entirely independent of each other.

That being said, one could quite easily create a criticized for the special case of a single method, lacking any other implementations, that doesn't use it's argument.

On Aug 31, 2017 11:55, "PBKResearch" <[hidden email]> wrote:
Stef

Done - https://pharo.fogbugz.com/f/cases/20363/ThemeIcons-downloadTo-has-an-argument-which-is-never-referenced-in-code

What really puzzled me is that this situation was not picked up by a code critic. 'Local variable declared and not referenced' is a common critic message, and this looks like a parallel situation. But I don't think this can be called a bug in the code critic system - can it?

Peter Kenny

-----Original Message-----
From: Pharo-users [mailto:[hidden email]] On Behalf Of Stephane Ducasse
Sent: 31 August 2017 19:16
To: Any question about pharo is welcome <[hidden email]>
Subject: Re: [Pharo-users] Code mystery

Can you open a bug entry?
I can tell you that we clean a lot but human activity will always leads to glitches.

On Tue, Aug 29, 2017 at 1:23 AM, Dimitris Chloupis <[hidden email]> wrote:
> You are not going crazy that's an ugly method. Probably the author
> intended to use dir and the forgot about it and instead hard coded the
> dir path inside a class method. A mistake that can happen to anyone.
>
> The method will have to be updated anyway because name is to be
> removed, so it won't work.
>
> As always too much code too few people. If you think that's bad
> embrace yourself if you try to read Morphic code. Huge suffering for
> me when I tried to learn how the task bar works and apparently using
> it wrong it freezes the image.
>
> Tons of Pharo code needs a clean but needs also a lot more people.
> On Tue, 29 Aug 2017 at 01:52, PBKResearch <[hidden email]> wrote:
>>
>> Hello All
>>
>>
>>
>> Following the discussion on dark mode, I was browsing the code on
>> themes (in Moose 6.1 = Pharo 6.0, Latest update: #60486). In Class
>> ThemeIcons, I found this method:
>>
>>
>>
>> downloadTo: dir
>>
>>                | zipArchive |
>>
>>
>>
>>                zipArchive := self class destinationPath / (self name,
>> '.zip').
>>
>>                zipArchive exists
>>
>>                               ifFalse: [
>>
>>                                              ZnClient new
>>
>>                                                             url: self
>> url;
>>
>>                                                             downloadTo:
>> zipArchive ].
>>
>>
>>
>>                ^ zipArchive
>>
>>
>>
>> The mystery is that the argument dir is not referred to anywhere in
>> the code. It probably works, because the only invocation of the
>> method is from
>> ThemeIcons>>downloadFromUrl, which sets the argument from self class
>> destinationPath, and the code above recreates this as the path to
>> zipArchive.
>>
>>
>>
>> I thought I understood Smalltalk coding fairly well, but this really
>> puzzles me. Why would anyone code like this? Shouldn’t it be picked
>> up by a code critic? Or am I going crazy?
>>
>>
>>
>> Any help gratefully received
>>
>>
>>
>> Peter Kenny


Reply | Threaded
Open this post in threaded view
|

Re: Code mystery

Peter Kenny

But surely all the code critic does is to ask a question – it is actually displayed with the question mark. In many cases the answer is ‘Yes, it is OK’ – e.g. if the comment is ‘Excessive number of methods?’ In the case under discussion, it seems legitimate to ask: ‘Did you really mean to declare the argument and then not use it?’ The programmer will presumably know if the complex situation you describe applies here, and then ignore the message.

 

Peter Kenny

 

From: Pharo-users [mailto:[hidden email]] On Behalf Of Richard Sargent
Sent: 01 September 2017 15:04
To: Any question about pharo is welcome <[hidden email]>
Subject: Re: [Pharo-users] Code mystery

 

I think the answer my lie in the general case and no one tried to address the complexity of the problem.

 

Consider an API that's implemented in a number of classes, classes which may not be in a hierarchy. Let's use #ifNil: as an example. 

 

Some implementers use the argument and others don't, so you cannot criticise the methods that don't.

 

It's much more difficult to recognize whether methods with the same name actually implement the same API or are entirely independent of each other.

 

That being said, one could quite easily create a criticized for the special case of a single method, lacking any other implementations, that doesn't use it's argument.

 

On Aug 31, 2017 11:55, "PBKResearch" <[hidden email]> wrote:

Stef

Done - https://pharo.fogbugz.com/f/cases/20363/ThemeIcons-downloadTo-has-an-argument-which-is-never-referenced-in-code

What really puzzled me is that this situation was not picked up by a code critic. 'Local variable declared and not referenced' is a common critic message, and this looks like a parallel situation. But I don't think this can be called a bug in the code critic system - can it?

Peter Kenny

-----Original Message-----
From: Pharo-users [mailto:[hidden email]] On Behalf Of Stephane Ducasse
Sent: 31 August 2017 19:16
To: Any question about pharo is welcome <[hidden email]>
Subject: Re: [Pharo-users] Code mystery

Can you open a bug entry?
I can tell you that we clean a lot but human activity will always leads to glitches.

On Tue, Aug 29, 2017 at 1:23 AM, Dimitris Chloupis <[hidden email]> wrote:


> You are not going crazy that's an ugly method. Probably the author
> intended to use dir and the forgot about it and instead hard coded the
> dir path inside a class method. A mistake that can happen to anyone.
>
> The method will have to be updated anyway because name is to be
> removed, so it won't work.
>
> As always too much code too few people. If you think that's bad
> embrace yourself if you try to read Morphic code. Huge suffering for
> me when I tried to learn how the task bar works and apparently using
> it wrong it freezes the image.
>
> Tons of Pharo code needs a clean but needs also a lot more people.
> On Tue, 29 Aug 2017 at 01:52, PBKResearch <[hidden email]> wrote:
>>
>> Hello All
>>
>>
>>
>> Following the discussion on dark mode, I was browsing the code on
>> themes (in Moose 6.1 = Pharo 6.0, Latest update: #60486). In Class
>> ThemeIcons, I found this method:
>>
>>
>>
>> downloadTo: dir
>>
>>                | zipArchive |
>>
>>
>>
>>                zipArchive := self class destinationPath / (self name,
>> '.zip').
>>
>>                zipArchive exists
>>
>>                               ifFalse: [
>>
>>                                              ZnClient new
>>
>>                                                             url: self
>> url;
>>
>>                                                             downloadTo:
>> zipArchive ].
>>
>>
>>
>>                ^ zipArchive
>>
>>
>>
>> The mystery is that the argument dir is not referred to anywhere in
>> the code. It probably works, because the only invocation of the
>> method is from
>> ThemeIcons>>downloadFromUrl, which sets the argument from self class
>> destinationPath, and the code above recreates this as the path to
>> zipArchive.
>>
>>
>>
>> I thought I understood Smalltalk coding fairly well, but this really
>> puzzles me. Why would anyone code like this? Shouldn’t it be picked
>> up by a code critic? Or am I going crazy?
>>
>>
>>
>> Any help gratefully received
>>
>>
>>
>> Peter Kenny

Reply | Threaded
Open this post in threaded view
|

Re: Code mystery

Richard Sargent
Administrator


On Fri, Sep 1, 2017 at 11:39 AM, PBKResearch <[hidden email]> wrote:

But surely all the code critic does is to ask a question – it is actually displayed with the question mark. In many cases the answer is ‘Yes, it is OK’ – e.g. if the comment is ‘Excessive number of methods?’ In the case under discussion, it seems legitimate to ask: ‘Did you really mean to declare the argument and then not use it?’ The programmer will presumably know if the complex situation you describe applies here, and then ignore the message.


I don't know if you are writing as if the code critic does exist and doesn't work as you would wish or doesn't exist.

However, from observation, I've concluded that Brant and Roberts tried to avoid too low of signal to noise ratio. They may have never considered this scenario.

Obviously, Pharo is free to do as they wish. Feel free to add the missing code critic. As you suggest, you might be able to use the existing  Unused Temporary Variable code critic as your basis. (But do create a new one, rather than make the existing one responsible for two different things.)

 

Peter Kenny

 

From: Pharo-users [mailto:[hidden email]] On Behalf Of Richard Sargent
Sent: 01 September 2017 15:04
To: Any question about pharo is welcome <[hidden email]>
Subject: Re: [Pharo-users] Code mystery

 

I think the answer my lie in the general case and no one tried to address the complexity of the problem.

 

Consider an API that's implemented in a number of classes, classes which may not be in a hierarchy. Let's use #ifNil: as an example. 

 

Some implementers use the argument and others don't, so you cannot criticise the methods that don't.

 

It's much more difficult to recognize whether methods with the same name actually implement the same API or are entirely independent of each other.

 

That being said, one could quite easily create a criticized for the special case of a single method, lacking any other implementations, that doesn't use it's argument.

 

On Aug 31, 2017 11:55, "PBKResearch" <[hidden email]> wrote:

Stef

Done - https://pharo.fogbugz.com/f/cases/20363/ThemeIcons-downloadTo-has-an-argument-which-is-never-referenced-in-code

What really puzzled me is that this situation was not picked up by a code critic. 'Local variable declared and not referenced' is a common critic message, and this looks like a parallel situation. But I don't think this can be called a bug in the code critic system - can it?

Peter Kenny

-----Original Message-----
From: Pharo-users [mailto:[hidden email]] On Behalf Of Stephane Ducasse
Sent: 31 August 2017 19:16
To: Any question about pharo is welcome <[hidden email]>
Subject: Re: [Pharo-users] Code mystery

Can you open a bug entry?
I can tell you that we clean a lot but human activity will always leads to glitches.

On Tue, Aug 29, 2017 at 1:23 AM, Dimitris Chloupis <[hidden email]> wrote:


> You are not going crazy that's an ugly method. Probably the author
> intended to use dir and the forgot about it and instead hard coded the
> dir path inside a class method. A mistake that can happen to anyone.
>
> The method will have to be updated anyway because name is to be
> removed, so it won't work.
>
> As always too much code too few people. If you think that's bad
> embrace yourself if you try to read Morphic code. Huge suffering for
> me when I tried to learn how the task bar works and apparently using
> it wrong it freezes the image.
>
> Tons of Pharo code needs a clean but needs also a lot more people.
> On Tue, 29 Aug 2017 at 01:52, PBKResearch <[hidden email]> wrote:
>>
>> Hello All
>>
>>
>>
>> Following the discussion on dark mode, I was browsing the code on
>> themes (in Moose 6.1 = Pharo 6.0, Latest update: #60486). In Class
>> ThemeIcons, I found this method:
>>
>>
>>
>> downloadTo: dir
>>
>>                | zipArchive |
>>
>>
>>
>>                zipArchive := self class destinationPath / (self name,
>> '.zip').
>>
>>                zipArchive exists
>>
>>                               ifFalse: [
>>
>>                                              ZnClient new
>>
>>                                                             url: self
>> url;
>>
>>                                                             downloadTo:
>> zipArchive ].
>>
>>
>>
>>                ^ zipArchive
>>
>>
>>
>> The mystery is that the argument dir is not referred to anywhere in
>> the code. It probably works, because the only invocation of the
>> method is from
>> ThemeIcons>>downloadFromUrl, which sets the argument from self class
>> destinationPath, and the code above recreates this as the path to
>> zipArchive.
>>
>>
>>
>> I thought I understood Smalltalk coding fairly well, but this really
>> puzzles me. Why would anyone code like this? Shouldn’t it be picked
>> up by a code critic? Or am I going crazy?
>>
>>
>>
>> Any help gratefully received
>>
>>
>>
>> Peter Kenny