Re: [Ccnet-devel] Url Trigger

[Owen R. <exo...@gm...>]

hi steve,
i've started to go through the code and it looks good.  i have a few
questions/comments if you don't mind.

httpwrapper:
- i tried running the unit tests for the httpwrapper and one of them
failed due to a timeout issue.  i'm also really concerned about how
long the tests take to run.  it will also be a source of annoyance
trying to run the ccnet unit tests if i'm disconnected and these tests
keep failing.  so, IHMO, i think that it is ok not to test the
httpwrapper *so long as* it is really only a thin wrapper around the
httprequest/response objects.  however, to get there, i think that it
might be important to move some of the logic back into the url
trigger.  what do you think?
- do the httprequest and httpresponse variables need to be members on
the class?  they are only used within the scope of the Execute method
and they are not reused across calls.  also, if we make them into
local variables, then you don't have to worry about making the class
implement IDisposable.
- i'm not super comfortable with the httpwrapper class logging and
swallowing exceptions.  i would also rather have that decision reside
in the trigger where it can be more easily testable.  what if the
httpwrapper simply returned a HttpWebResponse or a wrapper around it?=20
this keeps the httpwrapper very generic and moves the last modified
header logic into the trigger where it belongs.

urltrigger:
- i've taken the liberty of converting the properties into public
fields.  i know it's more of a philosophical argument, but i've gotten
in the habit of converting simple properties into fields as it really
reduces the amount of code and improves readibility.  i can always go
and transparently convert the field into a property at some point
later if i need it.
- the CheckUrlForChange method seems to be both validating the url and
executing the http request.  it seems to me that we could integrate
the url validating into the setter for the Url property.  as far as i
can tell, this is the only point that the property is modified.  this
provides the advantage of alerting the user that the url is invalid
right as the server starts up, instead of at the point where the
trigger fires.

please let me know your thoughts on these suggested changes.
cheers,
owen.
--
Owen Rogers | http://dotnetjunkies.com/weblog/exortech |
CruiseControl.NET - http://ccnet.thoughtworks.com