Hi Martin,
On 2007-09-18 at 14:55:34 [+0200], Martin Edenhofer <[EMAIL PROTECTED]> wrote:
>
> Oliver Tappe schrieb:
>
[ ... ]
> > Please fire any suggestions or questions to this list or directly my way.
>
>
> -=> Yes, 'die "Got no $_" if (!$Self->{$_});' the die() is not a option. ;)
>
> Bit I see different things to integrate it into the current OTRS
> framework (may be we are talking about two different things?).
No, having read your mail I still think we are talking about the same thing ;-)
> Anyway, here my points:
>
> o) Keep it clean:
> If we have a check method in the framework, we should replace any
> other checks from the whole framework with the new check to keep it clean.
Yes, that's the aim, but it is also a lot of work to do so (many lines of code
would have to be changed). But if we see the chance, we should do it all in one
big refactoring step.
> o) Check it every time?/Performance Issue:
> "# check all needed objects" - For normal framework, it should not be
> necessary to check the default objects. The framework should only
> deliver existing objects. You never should use e. g. $Self->{LogObject}
> for other perl objects.
Hm, I do not seem to understand you. What do you mean by default objects? And
why shouldn't one use $Self->{LogObject} in other perl objects? AFAICS, that
kind of use is spread all over the place. What's the problem with it?
> I also would see a really performance issue if
> we would check the object content every time. -=> So for the OTRS
> framework I would expect to not need to check the base objects.
Well, CheckParams() is not meant to check object "content" at all (as you are
not allowed to look into objects by definition anyway). The more complex
structures are only supported for hashes (not objects). It gets a bit
complicated, as objects are in fact implemented as hashes, but CheckParams() is
only intended to check the parameter hash given to a function. Think of it like
this: after the invocation of CheckParams(), a method knows that all required
paramteres have been passed in and (optionally) that they are of the correct
type.
> o) OO-Style;
> Only OO-Style. It would be not good to switch to other styles.
Ok, you have mentioned this before, but I dare to question the validity of the
argument behind it:
Perl is a hybrid programming language (like C++, but unlike Java and Smalltalk
for instance) and it provides a procedural and an object-oriented interface,
both of which are being used by OTRS. As an example, OTRS uses the functions
print(), split(), join(), open() and close() in many places, all of which are
part of perl's procedural interface.
I perfectly understand that most of the time, an object-oriented interface
(involving a class) is more apropriate, but *not* every time. Objects usually
encapsulate some kind of state or complexity, but in this case there would be
no state or complexity to encapsulate. Consider this:
CheckParams($Params, sub { ... })
is so much more convenient than
$Checker = Kernel::System::Params->new($Params);
$Checker->Check(sub { ... });
which would be the standard way of doing it in OTRS style. I personally find
this rather awkward, as it makes it rather difficult to grasp the meaning of
the code.
However, object-oriented interfaces provide an alternative solution when it
comes to implementing functions: class methods (discussion follows below):
Kernel::System::Params->Check($Params, sub { ... })
> o) confess():
> I have looked into your source code. I never have seen confess() in
> the OTRS framework bevor. Whats about this?
confess() is the standard way of requesting a die together with a stack crawl.
It is provided by the Carp module. One could of course use
$Self->{MainObject}->Die() instead, but that would depend on having an instance
of MainObject available, which may not always be the case.
> o) Filename:
> Utils/Params.pm is not on the current directory style.
> Kernel/System/Params.pm or to extend the existing
> Kernel/System/CheckItem.pm would sounds a little bit better to me.
Yep, Kernel/System/Params.pm would be correct another suggestion would be
Kernel/System/MethodArgs.pm (which at least for me expresses the actual meaning
of the module best).
> o) Utils::Params->Check($Params, sub { ... }); looks a little bit
> strange to me. I would suggest something like "$Self->{ParamsObject}"
> oder "$Self->{CheckObject}". And a methode like
Yes, standard OTRS style, which would require every method wanting to check its
arguments to create (or already have access to) an object of the CheckParams
class. As expressed above, I find that less clear (and clarity is what I like a
lot ;-).
The strangelooking invocation is a class method call (just like new()), i.e.
"Kernel::System::Params->Check()" invokes the method Check() on the class
Kernel::System::Params. Class methods are a common ocurrence in object oriented
software development.
> "$Self->{ParamsObject}->Check(
> 'ConfigObject' => '!Class=Kernel::Config',
> 'DBObject' => '!Class=Kernel::System::DB',
> 'LogObject' => '!Class=Kernel::System::Log',
> 'MainObject' => '!Class=Kernel::System::Main',
> 'TimeObject' => '!Class=Kernel::System::Time',
> );"
Hey, but that is using a hash instead of a hash-ref, wasting performance along
the way, as that hash would have to be copied during the invocation of the
method (I remember Richard Jelinek mentioning that).
Using an anonymous subroutine (sub { ... }) instead of a hash-ref makes it
possible to avoid the performance hit of generating the hash at all (like for
the case when the parameter checking has been disabled via SysConfig).
>
> o) One other thing would be to define more via attributes. E. g.
>
> 'TimeObject' => '!Class=Kernel::System::Time',
>
> Is IMO not so good because you need to use some cpu time to use
> a regexp for to find out what "!Class=" is.
Yes, you are right, it would be good to avoid the use of regexps and that can
be done easily (replacing regexps by substr()).
> I mean something like e. g.
>
> 'TimeObject' => {
> Operation => '!',
> Type => 'Class',
> Match => 'Kernel::System::Time',
> },
>
> IMO this would be faster and need less cpu time.
Yes, that certainly would be another way to avoid the regexps. I had outlined
the very compact declarative interface (using just one string) such that every
check could be contained on one line, making the code easier to read. But maybe
it would indeed be better to switch to a more attribute-like declaration like
you have suggested. I don't know, really - is there anybody else with a view on
this?
> o Maybe we need something like Kernel::System::Main->Die().
Yes, I'd like that. I personally would favor a *function* named Croak(),
automatically exported by Kernel::System::Basics, but that is probably just me
...
> So back to the two different things:
> IMO we need only a exists-check for objects (not content related) but we
> could use a new Check() for param validation of e. g. strings or other
> content.
>
> What do you think? :-)
I can't really see the difference between these two. Currently, CheckParams()
can offer both, it just depends on what you ask for:
Kernel::System::Params->Check($Self, sub {
'ConfigObject' => '!',
'DBObject' => '!',
'LogObject' => '!',
'MainObject' => '!',
'TimeObject' => '!',
#
'Filename' => '!',
'IsUTF8' => '?',
});
would do just that: check only the existence of the required framework objects
and check the functional parameters, too (the hashmark just separating these
two for visual clarity).
Notwithstanding any further discussion on this list, I think all of this would
be a fine topic to talk about over dinner during our meeting next week ;-)
cheers,
Oliver
--
((otrs)) :: OTRS GmbH :: Europaring 4 :: D - 94315 Straubing
Fon: +49 (0) 9421 56818-0 :: Fax: +49 (0) 9421 56818-18
http://www.otrs.com/ :: Communication with success!
_______________________________________________
OTRS mailing list: dev - Webpage: http://otrs.org/
Archive: http://lists.otrs.org/pipermail/dev
To unsubscribe: http://lists.otrs.org/cgi-bin/listinfo/dev
