Hi Aliaksei, Hi Stef, On Fri, Apr 1, 2016 at 2:40 PM, Aliaksei Syrel <alex.sy...@gmail.com> wrote:
> Thanks for feedback :) > > Indeed, this is a nice case to learn from. That piece of code was written > when I just started working with pharo after coming from Java world. > > Purpose of that catch, as mentioned by Nicolai, is indeed to ignore late > initialized objects. For example, imagine a Morph with height that is equal > to owner's height. In this case one would write in #initialize: > > initialize > self height: [ :morph | morph owner height ]. > > After setting an object as a new height, it gets evaluated and in case of > example before throws DNU because owner is obviously still nil. > > Trying to improve overall readability I decided to ignore all errors in > that kind of blocks instead of putting ifNil:ifNotNil: in hundred places. > > Also, I could not imagine that Warning is actually an Exception and > exception (in its common sense) is an Error. However, I had in mind that > later that kind of code smell should be replaced at least by custom error > handler, like we have now in spotter that catches all errors allowing not > buggy processors to do their work. > Btw, in first versions of spotter it was the same: it catched exception > instead of error but more experienced colleges suggested to not do it like > that. > > Nevertheless, later I realized that blocks are evil, and instead of trying > to work with nils it's much easier and cleaner to just put an assertion. > > As a hotfix, I will replace Exception to Error tomorrow. (bad, but > still...) > > Don't hesitate to post code smells, at least we can make jokes of them :) > :-). Stef, no one is "bashing someone". We /must/ be able to criticise code if we want the code base to get better, and this code was bad. Aliaksei responded to the criticism positively and did not take it personally. But the interesting point is that Aliaksei and people like him are clearly skilled programmers but they are coming to Pharo (or Squeak, or Smalltalk in general) from other systems, and if we as a community are successful in promoting Pharo, Squeak, Newspeak et al, we can expect many more people like Aliaksei to come to our community and want to contribute. So I wonder if we have the energy and resource to write brief documentation on good style vs bad style aimed at the experienced OO programmer who is trying Smalltalk for the first time. I think this is an important demographic to serve, and I think the form of the documentation would be considerably different to normal introductory texts; it can use a much steeper learning curve and can go from "here's how you do it", to "here's how to do it right" and "here's how not to do it", very quickly; something that the usual introductory texts stay away from, leaving these important topics to "advanced" course materials. Cheers > Alex > Cheers! > On Mar 30, 2016 1:34 PM, "Nicolai Hess" <nicolaih...@gmail.com> wrote: > >> Please don't do this: >> >> updateHeight >> "no need to care about height, when it's logic is not customized" >> self layout isHeightCustom ifFalse: [ ^ self ]. >> [ self bounds: (self brickBounds withHeight: self customHeight) ] >> on: Exception >> do: [ "just skip and do nothing" ] >> >> This makes debugging GLM/Brick ui/layout code with "self haltOnce" >> impossible. >> see >> GLMBrickGeometryTrait>>#updateHeight >> GLMBrickGeometryTrait>>#updateWidth >> >> And if you log out the raised exception, you see some calls to >> not initialized fonts and a ZeroDevide and some more errors. >> The above catch, catches and hides wrong / to late initialized objects. >> > -- _,,,^..^,,,_ best, Eliot
