Re: Shipping a build

Hello Franck,

Thanks for polishing the SRecord get* methods.  Probably negligible speed difference for most applications, but best not to ignore efficiency.

Might as well have a get() that accepts an SFieldInstance.  Same as getObject.   Might be convenient.  Should it be in SRecordInstance or Generic?  Not sure.  The latter would be a bit of a hack, but avoids column names in code.   (If on Generic would not text for consistent SRecordMetas, just get the field name.)

I had another look at the SRecordInstance code, and saw the isValidTest.  So they are indeed consistent.   isEmpty is wrong, should be size() == 0.  Needs to be documented on relevant methods.

But I don't see what is wrong with the following:-
> ri.size() ==> 6
> ri.put(key, value);
> ri.size() ==> 6 !!
put() just updates values.   The key was in the map before, its value was Invalid.   You cannot change the set of keys in a SRecordInstance, just update their values.

One issue is that the sets etc. are not properly backed by the getFieldNames().  So after putting an invalid value they do not get updated.  (They never get updated otherwise, so no problem.)  Invalid values are also rare, and I'm not sure whether they are worth the effort.  

But if it invalid values were left in keySet, then get on an invalid value should probably return null, which is messy.  containsKey would need to return true, to be consistent with keySet.  One would then need isValid to know.  But that probably does not matter.  Except that traditionally SimpleOrm fails fast and throws an exception when trying to get an invalid value.

A separate issue is efficiency.  One expects methods like size() to have very little overhead, should not generate a new object just to get its count.  And entrySet should ideally not generate a keySet as a temporary object.  The worst is containsKey(), which generates a keySet each time -- imagine someone innocently putting that in a tight loop and wondering why their code runs slow.  I would ask you to fix at least this last one.

(If invalid values are included in the keySet, then keySet  (but not entrySet) can simply be cached on SRecordMeta -- it never changes at all.  But efficiency is secondary in these matters.)

Anyway, conclusion is I would not have bothered excising invalid values, but it may well be a good thing.    Please fix empty.  And make containsKey() and size() efficient.  And a comment on entrySet to say that it is inefficient -- must create many Map.Entry objects due to bad Map definition.  (I would not bother caching keySet or entrySet, or trying to update them if values change -- too hard.  Easy to not create a keySet to create an entrySet although improvement will be minimal compared to all the Map.Entry objects.)

Regards,

Anthony

At 11:35 PM 1/08/2009, you wrote:
>  
>
>Hi,
>
>Le samedi 01 août 2009 à  18:49 +1000, <mailto:anthony%40berglas.org>[email protected] a écrit :
>> 
>> You even implemented EntrySet for the Map, which I was not suggesting
>> but it is good. Should have a comment that it is inefficient --
>> creates the map each time (OK, just a documentation issue). Likewise
>> KeySet. 
>
>I added a comment.
>> 
>> get(Object x) -- if x is not String I would throw an exception rather
>> than returning Null. I am more interested in usage patterns than the
>> strict definition of Map. What do you think?
>
>You are right.
>Moreover, the Map interface allows this behaviour (optionnal).
>I wanted to stick to the interface for cases like using JSF, etc. where
>an external component may rely on the fact that a Map is a Map :)
>
>I still have a question: do we only accept Strings as key, and make
>mandatory to call getObject for SFieldMeta, or do we
>also accept SFieldMeta as a parameter for get() ?
>
>> I would not have bothered with removing InvalidValues from the
>> values().
>I did it because if we don't, we would have this kind of weird
>behaviour:
>
>ri.size() ==> 6
>ri.put(key, value);
>ri.size() ==> 6 !!
>
>> But if you do so then should probably do it everywhere -- entrySet(),
>> keySet(). size() is now inconsistent. Common usage would be to display
>> all valid field values in a table. (But probably OK just to return
>> fieldValues asList and forget about InvalidValues. Size is just
>> fieldValues.length. But should be consistent.
>
>Well, actually I think it is consistent.
>SRi internal fieldValues is an array whose size is equal to
>meta.getAllFields().size().
>For unqueried/unset fields, it will contain the InvalidValue object.
>
>So in the Map implementation, we get rid of those values and consider
>that unset fields don't exist in the Map view.
>
>So map.size(), map.values(), map.isEmpty and map.keySet() all behave the
>same: they exclude invalid Fields, until they are set. So what we have
>is:
>
>ri.size() ==> 5
>ri.put(key, value);
>ri.size() ==> 6
>
>[Well, make me think... except if we use put to set a SFieldReference...
>Then we can have
>ri.put(refName, reference);
>ri.size() ==> 8 !!] Not sure if it matters...
>
>> size() should also be reasonably efficient.)
>> 
>
>> Is there a test case for the Map interface?
>> 
>No, I have to make one...
>
>> getDouble(field.getFieldName()) is a bit less efficient than I had in
>> mind, string to array and back, but probably OK in practice. Good to
>> see the code uncopied. (I would have created a private method
>> convertDouble, and then called it from both getDouble methods. Field
>> level methods such as get* should be efficient.)
>> 
>Changed the impelementation. It also allows the SException.Data to
>contain the SFieldMeta information when comming from SRecordInstance.
>
>> SimpleOrm is becoming quite elegant.
>> 
>I also think so :)
>
>Franck
>
>

Dr Anthony Berglas, [email protected]       Mobile: +61 4 4838 8874
Just because it is possible to push twigs along the ground with ones nose
does not necessarily mean that is the best way to collect firewood.


------------------------------------

Yahoo! Groups Links

<*> To visit your group on the web, go to:
    http://groups.yahoo.com/group/SimpleORM/

<*> Your email settings:
    Individual Email | Traditional

<*> To change settings online go to:
    http://groups.yahoo.com/group/SimpleORM/join
    (Yahoo! ID required)

<*> To change settings via email:
    mailto:[email protected] 
    mailto:[email protected]

<*> To unsubscribe from this group, send an email to:
    [email protected]

<*> Your use of Yahoo! Groups is subject to:
    http://docs.yahoo.com/info/terms/