> From: julian@... [mailto:julian@...]
> >Christian.Mayer@... wrote:
> >>After some working and modifying the first web page to show custom
> >>From: julian@... [mailto:julian@...]
> >>>So, ok, I got something committed on custom fields here.
> >>I already found a couple of bugs (actually syntax
> >errors...) and also
> >>increased the functionality of custom_field_get_ids().
> >>If I pass a project ID of 0 to custom_field_get_ids now, I'll get a
> >>list of all existing custom field IDs. This is important to
> >be able to
> >>show the user a list of all existing fields so he can
> >choose one to add
> >>it to a project.
> >Hmm... I would prefer that either we have two functions or we
> >your behaviour by calling the function with *no* parameters (using a
> >default of null in the function definition). I don't like
> >using 0 as a
> >special value (even for the "all projects" stuff, really -
> >and this is a
> >slightly different use of 0).
> >I think we really should just have two functions, since one SQL query
> >needs a join and the other doesn't. Not sure exactly what we should
> >call them though... custom_field_get_all_ids() and
> Setting the default to 0 is a good solution.
> I don't like the idea of 2 different functions. Both of the have
> exactly the same functionality, only the SQL query is *slightly*
> different. So we don't need to copy functionaliy. This would even bite
> us if we discover a bug in one of those functions and fix it - but we
> forget the other one...
> So I've attached a diff to the very current CVS version with these
hmm, I still don't buy that. The problem is that using 0 is ambiguous
here. I, personally, would expect that passing in 0 (used everywhere
else as All Projects) would give me the ids of all the fields that are
displayed in every project (the set intersection). Somebody might
expect it to return the set union of every field that's used. You're
using it to mean every field that's defined. It's totally ambiguous.
So first-off, since there's no reason to use 0 internally, I would
really want the default value to be null, that way we can at least catch
0 and either error or return the union or intersection instead of all
the fields (I'm thinking of a case where the current project is passed
in unchecked and happens to be 0).
But, I *do* think they should really just be separate functions. You
say that "Both of the have exactly the same functionality, only the SQL
query is *slightly* different" but there's nothing in that function
*except* the SQL query. And the fact that it *is* different means to me
that it's not doing the same thing. The only other code is a loop to
generate the list of ids and this exists in dozens of places throughout
the code that we'd need to fix if there was a bug.
Putting switches into functions unecessarily can lead to confusion for
ths user since the behaviour is complex and makes the code more complex
and thus more prone to bugs in the first place.
At the very least, if you need to have one interface for the user, you
should add one /on top of/ the other two functions but I don't see a
need for this. Why would a programtic process need to get *either* the
list of fields for a project *or* all the fields? They're two totally
different use cases.
I'll look at and apply your other patch today.
Beta4 Productions (http://www.beta4.com)