There is IMHO a nasty code smell around a_Html_add_new_image(): it takes a boolean parameter that determines whether or not it actually adds the image. It's only called from two other places, once with the parameter set to false and once with it set to true. This patch cleans things up by renaming a_Html_add_new_image() to a_Html_image_new(), removing the boolean parameter and making the caller add the image (or not) as appropriate. Perhaps it is a judgement call as to whether this is an improvement, but I feel that it is. Please comment. Regards, Jeremy Henty
On Sat, Feb 07, 2009 at 05:57:21PM +0000, Jeremy Henty wrote:
There is IMHO a nasty code smell around a_Html_add_new_image(): it takes a boolean parameter that determines whether or not it actually adds the image. It's only called from two other places, once with the parameter set to false and once with it set to true.
This patch cleans things up by renaming a_Html_add_new_image() to a_Html_image_new(), removing the boolean parameter and making the caller add the image (or not) as appropriate.
Perhaps it is a judgement call as to whether this is an improvement, but I feel that it is. Please comment.
did you forget to attach the patch? Cheers, Johannes
Jeremy wrote:
There is IMHO a nasty code smell around a_Html_add_new_image(): it takes a boolean parameter that determines whether or not it actually adds the image. It's only called from two other places, once with the parameter set to false and once with it set to true.
This patch cleans things up by renaming a_Html_add_new_image() to a_Html_image_new(), removing the boolean parameter and making the caller add the image (or not) as appropriate.
Perhaps it is a judgement call as to whether this is an improvement, but I feel that it is. Please comment.
In principle it might be dangerous in some way for the callers to wait to add widgets until after Html_load_image() is called, but as long as there aren't any timeouts in between, I don't think we'd be able to make it break.
On Sat, Feb 07, 2009 at 08:20:18PM +0000, corvid wrote:
Jeremy wrote:
There is IMHO a nasty code smell around a_Html_add_new_image(): it takes a boolean parameter that determines whether or not it actually adds the image. It's only called from two other places, once with the parameter set to false and once with it set to true. [...] In principle it might be dangerous in some way for the callers to wait to add widgets until after Html_load_image() is called,
If we want the a_Html_* API to guard against that then that is another reason for changing it, since the current API allows you to create the image and not add it. In that case I'd suggest applying my patch and then changing a_Html_image_new() to static Html_image_new() and adding a_Html_add_new_image() and a_Html_add_new_image_button(). That would fix the code smell *and* enforce the "add any image that you create" condition. Does that make sense? Regards, Jeremy Henty
Jeremy wrote:
On Sat, Feb 07, 2009 at 08:20:18PM +0000, corvid wrote:
Jeremy wrote:
There is IMHO a nasty code smell around a_Html_add_new_image(): it takes a boolean parameter that determines whether or not it actually adds the image. It's only called from two other places, once with the parameter set to false and once with it set to true. [...] In principle it might be dangerous in some way for the callers to wait to add widgets until after Html_load_image() is called,
If we want the a_Html_* API to guard against that then that is another reason for changing it, since the current API allows you to create the image and not add it. In that case I'd suggest applying my patch and then changing a_Html_image_new() to static Html_image_new() and adding a_Html_add_new_image() and a_Html_add_new_image_button(). That would fix the code smell *and* enforce the "add any image that you create" condition. Does that make sense?
I'm not sure the "danger" is really worth worrying about; it's just something that crossed my mind...
On Sat, Feb 07, 2009 at 11:59:03PM +0000, corvid wrote:
Jeremy wrote:
On Sat, Feb 07, 2009 at 08:20:18PM +0000, corvid wrote:
In principle it might be dangerous in some way for the callers to wait to add widgets until after Html_load_image() is called,
If we want the a_Html_* API to guard against that then that is another reason for changing it, since the current API allows you to create the image and not add it. [...]
I'm not sure the "danger" is really worth worrying about; it's just something that crossed my mind...
Sure, but even if there's no real danger it's nice to have an API that expresses how the code is meant to work. Regards, Jeremy Henty
On Sat, Feb 07, 2009 at 05:57:21PM +0000, Jeremy Henty wrote:
There is IMHO a nasty code smell around a_Html_add_new_image(): it takes a boolean parameter that determines whether or not it actually adds the image. It's only called from two other places, once with the parameter set to false and once with it set to true.
This patch cleans things up by renaming a_Html_add_new_image() to a_Html_image_new(), removing the boolean parameter and making the caller add the image (or not) as appropriate.
Perhaps it is a judgement call as to whether this is an improvement, but I feel that it is. Please comment.
It looks like an improvement. Committed. -- Cheers Jorge.-
participants (4)
-
corvid＠lavabit.com -
jcid＠dillo.org -
Johannes.Hofmann＠gmx.de -
onepoint＠starurchin.org