IRC Meeting Full 20090109

From ADempiere
Jump to: navigation, search
This Wiki is read-only for reference purposes to avoid broken links.

Interesting meeting that took place on the IRC. It was discussed some hints and tips for developers, and proposed some standards.


<CarlosRuiz>: Hi
<teo_sarca>: hi all
<teo_sarca>: finally if setup irc client ;)
<teo_sarca>: i've setup
<teo_sarca>: ok
<vpj-cd>: ok
<CarlosRuiz>: what's the topic - I still don't understand -
<trifon>: hi
<vpj-cd>: I sorry Carlos
<vpj-cd>: the topic
<vpj-cd>: was started
<vpj-cd>: for this
<vpj-cd>: method @Override
<vpj-cd>: public MProduct getM_Product()
<vpj-cd>: {
<vpj-cd>: return MProduct.get(getCtx(), getM_Product_ID());
<vpj-cd>: }
<vpj-cd>: @Override
<vpj-cd>: public MUOM getC_UOM()
<vpj-cd>: {
<vpj-cd>: return MUOM.get(getCtx(), getC_UOM_ID());
<vpj-cd>: }
<vpj-cd>:
<vpj-cd>: @Override
<vpj-cd>: public MWarehouse getM_Warehouse()
<vpj-cd>: {
<vpj-cd>: getM_Warehouse();
<vpj-cd>: return MWarehouse.get(getCtx(), getM_Warehouse_ID());
<vpj-cd>: }
<vpj-cd>: in the MPPOrderBOMLine
<vpj-cd>: class
<vpj-cd>: the idea the Teo is set a cache for this class
<vpj-cd>: but with new improve of trifon
<vpj-cd>: you only need (Product) getM_Product()
<vpj-cd>: then the idea is implement the cache in the improve of Trifon
<vpj-cd>: public I_M_Product getM_Product() throws RuntimeException
<vpj-cd>: {
<vpj-cd>: Class<?> clazz = MTable.getClass(I_M_Product.Table_Name);
<trifon>: Victor wants to put cahcing in generated X_ classes.
<vpj-cd>: I_M_Product result = null;
<vpj-cd>: try {
<vpj-cd>: Constructor<?> constructor = null;
<vpj-cd>: constructor = clazz.getDeclaredConstructor(new Class[]{Properties.class, int.class, String.class});
<vpj-cd>: result = (I_M_Product)constructor.newInstance(new Object[] {getCtx(), new Integer(getM_Product_ID()), get_TrxName()});
<vpj-cd>: } catch (Exception e) {
<vpj-cd>: log.log(Level.SEVERE, "(id) - Table=" + Table_Name + ",Class=" + clazz, e);
<vpj-cd>: log.saveError("Error", "Table=" + Table_Name + ",Class=" + clazz);
<vpj-cd>: throw new RuntimeException( e );
<vpj-cd>: }
<vpj-cd>: return result;
<vpj-cd>: }
<vpj-cd>: nop trifon my idea is
<vpj-cd>: se here the validation if exit in cache
<vpj-cd>: I_M_Warehouse result = null;
<vpj-cd>: result = PO.getCache(I_M_Warehouse.Table_Name,getM_Warehouse_ID());
<vpj-cd>: if (result != null)
<vpj-cd>: return result
<vpj-cd>: but the implementation the cache can be implement in PO
<trifon>: ok. i see.
<CarlosRuiz>: let me check code a little ...
<trifon>: Teo what is your objection?
<vpj-cd>: Now here we had the discussion want is best way to implement cache and that class
<vpj-cd>: Teo said that exit some class that not need cache
<vpj-cd>: with this focus the cache is implement for all class that was create some intance
<vpj-cd>: instance
<CarlosRuiz>: not need cache - and some tables where cache is really undesirable
<CarlosRuiz>: i.e. it will be risky to cache invoices
<vpj-cd>: and then here my question is what is the best way to implement a generic cache
<teo_sarca>: yes
<vpj-cd>: yes Carlos this cache
<teo_sarca>: and i a previous discussion
<teo_sarca>: some weeks ago
<teo_sarca>: we talked about
<teo_sarca>: introducing
<teo_sarca>: AD_Table.IsCached
<teo_sarca>: (or similar)
<vpj-cd>: only for object that do not need transaction
<teo_sarca>: to mark which tables needs to be cached
<vpj-cd>: in first instance
<vpj-cd>: Teo we would adding the Tag in the MClass
<vpj-cd>: for the class that apply the cache
<CarlosRuiz>: does somebody remember the cache properties implemented in some j2ee container?
<vpj-cd>: for example MColumn
<vpj-cd>: nop Carlos
<vpj-cd>: is the topic
<vpj-cd>: the disuction is implement a good solution for cache
<CarlosRuiz>: I mean - if we're going with IsCached - maybe a better way is to implement a AD_CacheProperties table and add AD_CacheProperties_ID to AD_Table
<CarlosRuiz>: normally this is implemented in a .properties file
<vpj-cd>: or tag
<teo_sarca>: carlos, i agree with you
<vpj-cd>: @Cached
<CarlosRuiz>: sorry - what's a tag?
<CarlosRuiz>: ah - I see
<teo_sarca>: tag = annotation
<vpj-cd>: I a sorry
<vpj-cd>: the right is annotation
<trifon>: Victor our annotations are in DB. Application dictiory.
<vpj-cd>: yes we will start in create the ADempiere annotation
<vpj-cd>: yes in the future
<vpj-cd>: we should create an annotation for each feature the AD
<trifon>: Annotationas are Metadata. can be in source code as annotations, in propery files, in Db and so on..
<vpj-cd>: as OpenXava :-)
<CarlosRuiz>: :-) and drop the AD? that's Timo idea
<trifon>: Vicotr annoations make no sence in multy Company environment.
<vpj-cd>: nop Carlos
<trifon>: each installation must have different annotations.
<vpj-cd>: the Annotation implement the logic
<vpj-cd>: for affect the dictionary
<CarlosRuiz>: and the magic is to change AD and you don't need to regenerate X_ classes
<vpj-cd>: in a future we create the X_ based in AD
<CarlosRuiz>: with annotations inherited from AD we would need to renegerate X_ classes on every AD change
<vpj-cd>: when this happen
<vpj-cd>: we only can maintenance the source code
<vpj-cd>: for this other subject
<CarlosRuiz>: I don't think so
<teo_sarca>: yes, i think we can store those in DB
<CarlosRuiz>: that's the openbravo model - every AD change you need to regenerate something
<trifon>: Victor my personal opinion is that annotations are static metadata. to change annotation you must change source code --> recompy --> redeploy...
<teo_sarca>: in different table
<teo_sarca>: as Carlos
<teo_sarca>: proposed
<teo_sarca>: in this way we can tune them
<vpj-cd>: yes Trifon
<vpj-cd>: this same issue the openXava
<vpj-cd>: now
<vpj-cd>: the other option is to make smart cache
<trifon>: cache can't be smarter than developer :)
<vpj-cd>: if a object is served for more the 3 times
<trifon>: so let's make new tables which say which tables, methods to cache.
<vpj-cd>: then the cache is created
<vpj-cd>: or some similar
<CarlosRuiz>: Victor - with such smart cache - you can finish caching invoices
<trifon>: this is ok. but who is going to tell to this cace that invoice must not be cached at all.
<teo_sarca>: @trifon : AD_TableCache
<teo_sarca>: records
<trifon>: sure. thi sis the developer who makes record in AD_TableCache
<teo_sarca>: yes
<CarlosRuiz>: thinking in future usage of j2ee container
<vpj-cd>: yes I am agree with Carlos
<CarlosRuiz>: I suppose the AD_CacheProperties must reflect the properties supported by cache container - or at least some of them we need most
<vpj-cd>: why do not set i the AD
<vpj-cd>: in table and columnt
<vpj-cd>: ie we can set in C_Order
<vpj-cd>: for the column c_bparther_Id
<vpj-cd>: ad a flag is chached
<trifon>: i think that it is better if it is in external table not in C_Order.
<trifon>: cache has nothing to do with Order.
<vpj-cd>: in general the cached are necessary
<vpj-cd>: for reference List
<vpj-cd>: TableDir
<vpj-cd>: and Table
<CarlosRuiz>: no
<CarlosRuiz>: it's a property of each table
<CarlosRuiz>: some tables need to be cached - others don't - and cached tables can have different cache strategies (that's the properties of cache)
<CarlosRuiz>: i.e. lazy cache
<CarlosRuiz>: - or complete cache at startup (like ad_message or c_country)
<CarlosRuiz>: the size of the cache - if we want complete cache or some fifo cache (or lifo)
<teo_sarca>: yes
<teo_sarca>: agree
<teo_sarca>: there is another issue
<teo_sarca>: the local cache
<teo_sarca>: i.e. AD_Column.IsCached (idea)
<teo_sarca>: for example
<teo_sarca>: we have C_Invoice.C_BPartner_ID
<teo_sarca>: we have a lot of partners
<teo_sarca>: (so caching C_BPArtner table is not an option)
<teo_sarca>: but we use invoice.getC_BPartner method
<teo_sarca>: a lot
<teo_sarca>: so we want to cache local
<teo_sarca>: WDYT ?
<CarlosRuiz>: or in this case you can implement a LRU cache
<CarlosRuiz>: with specific size
<teo_sarca>: hmmm
<CarlosRuiz>: or the opposite I mean - a specific size cache saving just the most frequently used records
<teo_sarca>: yes, can be an option, not sure :)
<teo_sarca>: i know
<vpj-cd>: I think
<vpj-cd>: that is very simple
<CarlosRuiz>: that's implemented in cache algorithms dropping from the cache the LRU records
<vpj-cd>: we can implement
<vpj-cd>: the cache in the X_
<vpj-cd>: with the improve of trifon
<CarlosRuiz>: but now that we're talking about that - the best is to make that configurable
<vpj-cd>: this way if you use I_M_Warehouse result = null;
<vpj-cd>: result = PO.getCache(I_M_Warehouse.Table_Name,getM_Warehouse_ID());
<vpj-cd>: if (result != null)
<vpj-cd>: return result
<vpj-cd>: or some similar
<CarlosRuiz>: because you can have different cache sizes depending on the implementation
<teo_sarca>: agree with you carlos
<vpj-cd>: if you use getM_Warehouse()
<vpj-cd>: then you know that you use cache
<vpj-cd>: if you do no want use cache
<vpj-cd>: then you create a new intance
<vpj-cd>: instance
<CarlosRuiz>: and in some implementations there are tables that change more frequently than others - you need to fine tune cache in some cases
<teo_sarca>: agree
<CarlosRuiz>: if a table changes constantly - it must not be cached
<CarlosRuiz>: but that's an attribute of the implementation - not the table
<vpj-cd>: ok carlos agree
<vpj-cd>: the cache need be very flexible
<vpj-cd>: .properties file
<vpj-cd>: ?
<CarlosRuiz>: as Trifon said - our properties files are AD
<vpj-cd>: so the PO review this properties file
<CarlosRuiz>: because we're client/server
<vpj-cd>: ok then a new Table
<CarlosRuiz>: BTW - this is another cache property
<CarlosRuiz>: shared cache
<CarlosRuiz>: for zkwebui it will be interesting to share caches from different sessions
<vpj-cd>: in
<CarlosRuiz>: I'm not sure if currently every session has his own cache repeating the same tables
<vpj-cd>: the swing client also
<vpj-cd>: because we can get from server some objects
<vpj-cd>: we would to have
<vpj-cd>: the cache in the server for multiples clients
<vpj-cd>: Carlos what filed should had AD_CacheProperties
<vpj-cd>: ?
<trifon>: do we have agreement that we must create new tables to store information abotu cached tables and columns?
<trifon>: as Carlos proposed AD_Cached...
<trifon>: it is fine for me.
<CarlosRuiz>: I suppose cachestrategy, size of cache, islazy, iscomplete
<trifon>: ok. thi si sgood.
<CarlosRuiz>: cache strategy -> fifo / lifo / most frequently used / complete
<CarlosRuiz>: size - number of records to store (0 means all)
<CarlosRuiz>: but what I think is it will be better to study
<CarlosRuiz>: the cache properties file of some j2ee implementation
<trifon>: cache strategy can be grouping. under it we can have info for each table and column.
<CarlosRuiz>: ie jbosscache
<CarlosRuiz>: and look what can we implement here
<CarlosRuiz>: http://www.jboss.org/community/docs/DOC-10242
<CarlosRuiz>: or study hibernate cache - or something like that - to see what can be implemented initially here
<CarlosRuiz>: and maybe open some columns just for future use
<CarlosRuiz>: I'm not sure if trifon and victor got the last lineas
<CarlosRuiz>: lines
<trifon>: no me.
<trifon>: my net is unstable :(
<CarlosRuiz>: Victor's also - let's wait for Victor to relogin and I'll copy the last lines
<vpj-cd>: I sorry
<vpj-cd>: I lost my connection
<vpj-cd>: some conclusion
<vpj-cd>: what is the general recomendation?
<teo_sarca>: another big issue is the trxName
<CarlosRuiz>: (12:29:08) CarlosRuiz: I suppose cachestrategy, size of cache, islazy, iscomplete
<CarlosRuiz>: (12:29:53) CarlosRuiz: cache strategy -> fifo / lifo / most frequently used / complete
<CarlosRuiz>: (12:29:53) CarlosRuiz: size - number of records to store (0 means all)
<CarlosRuiz>: (12:30:01) CarlosRuiz: but what I think is it will be better to study
<CarlosRuiz>: (12:30:07) CarlosRuiz: the cache properties file of some j2ee implementation
<CarlosRuiz>: (12:30:12) CarlosRuiz: ie jbosscache
<CarlosRuiz>: (12:30:16) CarlosRuiz: and look what can we implement here
<CarlosRuiz>: (12:31:10) CarlosRuiz: http://www.jboss.org/community/docs/DOC-10242
<CarlosRuiz>: (12:31:58) CarlosRuiz: or study hibernate cache - or something like that - to see what can be implemented initially here
<CarlosRuiz>: (12:32:05) CarlosRuiz: and maybe open some columns just for future use
<teo_sarca>: what do you think about that ?
<vpj-cd>: exist some standard around the cache system
<vpj-cd>: ?
<trifon>: i do not know about standard for cache.
<trifon>: i think that most important is if we can have agrremtn how ot store meta information about which table/column must be cahced.
<trifon>: so far i think that best proposal and most accepted is to make new tbales.
<teo_sarca>: in database table
<vpj-cd>: yes but we can adopting good practices
<vpj-cd>: http://www.jboss.org/file-access/default/members/jbosscache/freezone/docs/3.0.1.GA/userguide_en/html_single/index.html#d0e108
<teo_sarca>: i think that we can store metadata in DB
<teo_sarca>: and integrate a caching system
<teo_sarca>: and not reinvent the wheel(c)
<teo_sarca>: like JJ
<trifon>: right.
<trifon>: can we agree on initial table names and model.
<trifon>: i like to see AD_CacheStrategy, AD_CacheTable, AD_CacheColumn.
<vpj-cd>: exacly Teo
<trifon>: most important for me is to have many cache strategies and to be able to switch between them in order to test which one works best.
<CarlosRuiz>: Trifon - I think is just AD_CacheStrategy - and AD_Table.AD_CacheStrategy_ID
<CarlosRuiz>: don't know if we want to go deeper caching columns - that's even harder
<trifon>: Teo must comment here as he wanted to have better cotrol.
<teo_sarca>: (thinking)
<trifon>: Carlos one question.
<teo_sarca>: Carlos, do you think LRU can solve my issues (regarding local cache) ?
<trifon>: how to specify that i want 5 000 records cache for language and only 100 for invoice?
<vpj-cd>: then I think the first
<vpj-cd>: stage is implement
<trifon>: will anybody need to contorl size of cache for specific table?
<vpj-cd>: a cache pojo
<vpj-cd>: http://www.jboss.org/file-access/default/members/jbosscache/freezone/docs/3.0.0.GA/pojo/userguide_en/html_single/index.html#d0e356
<vpj-cd>: http://www.jboss.org/file-access/default/members/jbosscache/freezone/docs/3.0.0.GA/pojo/userguide_en/html_single/index.html#1
<vpj-cd>: the Oher tipic
<vpj-cd>: topic
<vpj-cd>: Carlos
<vpj-cd>: Teo, Trifon
<vpj-cd>: what is the politic for named the method
<vpj-cd>: in ADempiere?
<trifon>: i do not se any post since [19:47] <
<teo_sarca>: @victor
<CarlosRuiz>: sorry - a little local discussion
<teo_sarca>: i vote for my proposal ;)
<teo_sarca>: so do not ask me :)
<teo_sarca>: i.e. forC_Order_ID(....)
<CarlosRuiz>: Teo - for language I suppose is something like "cache complete at startup"
<CarlosRuiz>: for invoice - not cached
<CarlosRuiz>: for bpartners and products it can be -> lazy, size 500, MFU
<vpj-cd>: I vote for descriptives names
<vpj-cd>: getOrder()
<teo_sarca>: hehe
<CarlosRuiz>: AFAIR the rule is simple
<vpj-cd>: is more human language
<CarlosRuiz>: what does the method?
<vpj-cd>: can you see my
<teo_sarca>: @Carlos
<teo_sarca>: here is an example:
<CarlosRuiz>: can you explain me with few words what does the method do?
<teo_sarca>: from class MPPOrderBOMLIne:
<vpj-cd>: log that sent to teo yesterhay
<teo_sarca>: public static MPPOrderBOMLine forM_Product_ID(Properties ctx, int PP_Order_ID, int M_Product_ID, String trxName)
<teo_sarca>: {
<teo_sarca>: final String whereClause = COLUMNNAME_PP_Order_ID+"=? AND "+COLUMNNAME_M_Product_ID+"=?";
<teo_sarca>: return new Query(ctx, Table_Name, whereClause, trxName)
<teo_sarca>: .setParameters(new Object[]{PP_Order_ID, M_Product_ID})
<teo_sarca>: .firstOnly();
<teo_sarca>: }
<CarlosRuiz>: Teo / Victor - can you explain me with few words what does the method do?
<teo_sarca>: returns all bom lines for given product_ID
<CarlosRuiz>: try with less than 6 words
<teo_sarca>: (7)
<CarlosRuiz>: :-)
<CarlosRuiz>: so - what about this getBomLinesForProductID
<vpj-cd>: I voted for Carlos way
<CarlosRuiz>: I'm just translating your words
<vpj-cd>: the human description
<teo_sarca>: personally i don't like that
<CarlosRuiz>: the recommendation is that just reading the method name you must know what it does internally
<teo_sarca>: i am thinking a little bit in future
<CarlosRuiz>: think just about interfaces - what if you don't have access to source code
<teo_sarca>: when maybe i will want to generate methods like that....
<CarlosRuiz>: if you don't have access to source then you don't know what the method does internally
<vpj-cd>: public static MPPOrder forC_OrderLine_ID(Properties ctx, int C_OrderLine_ID, String trxName)
<teo_sarca>: not if i explain you the rule
<teo_sarca>: example:
<teo_sarca>: Class.forName
<teo_sarca>: same idea
<teo_sarca>: so the rule is simple:
<vpj-cd>: getOrderFromSalesOrderLine
<teo_sarca>: for<ColumnName>(...)
<vpj-cd>: MPPOrderBOMLine forM_Product_ID(Properties ctx, int PP_Order_ID, int M_Product_ID, String trxName)
<CarlosRuiz>: ah - I see - such rule is useful for static
<CarlosRuiz>: method returning the object for the same class where they are in
<vpj-cd>: MPPOrderNode activity = getPPOrderNode(); MPPOrderNode activity = getPP_Order_Node();
<vpj-cd>: if we will use _
<vpj-cd>: or not
<CarlosRuiz>: but such rules is not useful for other cases
<CarlosRuiz>: I mean
<teo_sarca>: @Carlos, i have/need a lot of for<ColumnName> methods ;)
<vpj-cd>: and then
<CarlosRuiz>: it's useful just if I can use
<CarlosRuiz>: MPPOrderBOMLine.forM_Product_ID - and it returns a MPPOrderBOMLine
<CarlosRuiz>: but the rule doesn't apply always - i.e for non-static it doesn't serve
<vpj-cd>: each develoer use your criterial
<teo_sarca>: i think is much more descriptive then concatenating 6 words ;)
<CarlosRuiz>: Teo - look my point - it's useful just for static returning an object of the class they're in
<CarlosRuiz>: but it's not useful for non-static methods
<CarlosRuiz>: for non-static methods is better the 6-words
<teo_sarca>: @Carlos, those are static methods
<vpj-cd>: can we define a standard for static methos
<vpj-cd>: methods?
<CarlosRuiz>: yes - I'm not talking about this specific case - just trying to opine about general naming convention
<teo_sarca>: yes
<vpj-cd>: exactly we need general naming convention
<CarlosRuiz>: for static or non-static methods returning an object different than the class - then it's better the 6words
<vpj-cd>: for adempiere
<teo_sarca>: i vote for my proposal ;)
<teo_sarca>: regarding static getters
<vpj-cd>: I vote for 6 words
<CarlosRuiz>: Teo - your proposal just cover statics returning objects from the same class based on a search on parameters
<vpj-cd>: ie in
<vpj-cd>: JPA
<trifon>: i think that it is not necessary name of method to contain returned type.
<vpj-cd>: is common named with 6 words
<trifon>: getC_BPartner_IDfromOrder.
<vpj-cd>: when you try return a collection the objects
<trifon>: method returns C_BPartner_ID so we must remove it from the name.
<teo_sarca>: guys, i am WELL describing in 2 words... why do i need 6 words ?
<CarlosRuiz>: another point for Victor - just for static methods returning ONE object based on parameters
<teo_sarca>: @Trifon: take a look at method signature:
<teo_sarca>: MPPOrderBOMLine forM_Product_ID(Properties ctx, int PP_Order_ID, int M_Product_ID, String trxName)
<CarlosRuiz>: Teo - I think your proposal is good - but it must be limited just for those specific cases
<teo_sarca>: it is returning bom lines for M_Product_ID
<trifon>: "for" is not good prefix.
<teo_sarca>: why trifon ?
<vpj-cd>: in general I no like
<CarlosRuiz>: if you ask me, again - I like Teo proposal for statics methods returning JUST ONE object from the same class based on a search on parameters
<CarlosRuiz>: for all other cases we must prefer the convention - describe what the method does internally
<vpj-cd>: the _ for method
<vpj-cd>: that do not is pojo
<trifon>: i preffer "by" instead of "for".
<vpj-cd>: teo ie
<vpj-cd>: MPPOrderBOMLine forM_Product_ID(Properties ctx, int PP_Order_ID, int M_Product_ID, String trxName)
<CarlosRuiz>: in fact the static method proposed are called Accessors ?
<vpj-cd>: this method do not is right
<vpj-cd>: the reason you can have the same product in the diferent bom line
<vpj-cd>: you need a collection
<teo_sarca>: @Victor
<teo_sarca>: hold a moment
<teo_sarca>: in this case there is a bug in logic
<teo_sarca>: in other cases i return collection
<teo_sarca>: depends on relation between elements
<vpj-cd>: so we should use words
<vpj-cd>: and Uper
<vpj-cd>: Upper case for separate the words
<vpj-cd>: in static methods
<CarlosRuiz>: well - if the method must return more than one object - the for* convention maybe is not good
<vpj-cd>: @Entity
<vpj-cd>: @NamedQueries({
<vpj-cd>: @NamedQuery(name="magsOverPrice",
<vpj-cd>: query="SELECT x FROM Magazine x WHERE x.price > ?1"),
<vpj-cd>: @NamedQuery(name="magsByTitle",
<vpj-cd>: query="SELECT x FROM Magazine x WHERE x.title = :titleParam")
<vpj-cd>: })
<vpj-cd>: magsOverPrice
<vpj-cd>: magsByTitle
<CarlosRuiz>: I agree with Teo about the Class.forName example - but precisely that is a "statics method returning JUST ONE object from the same class based on a search on parameters"
<teo_sarca>: @Carlos, why ?>
<CarlosRuiz>: sorry - why what?
<teo_sarca>: why if the static method is returning a collection, the "for" is not ok ?
<vpj-cd>: forM_Product_ID(Properties, int, int, String) vs getByOrderAndProduct
<vpj-cd>: if a static object return the same class
<vpj-cd>: the should use get
<trifon>: magsOverPrice and magsByTitle are good examples. from the metohd i can understadn what is the menaing of the functionality.
<trifon>: i preffer getByOrderandProduct.
<trifon>: forM_Product_ID is confusing for me.
<vpj-cd>: I this my approach
<CarlosRuiz>: wait a minute - the name forM_Product_ID is incomplete if the parameters are order+product
<vpj-cd>: and I said the teo the same
<vpj-cd>: is very confusing also to me
<trifon>: @Carlos, right. i agrre for this.
<teo_sarca>: @Carlos, it's obvious
<teo_sarca>: also you have javadoc
<CarlosRuiz>: no - it's not obvious when you read it like
<CarlosRuiz>: forM_Product_ID(Properties, int, int, String)
<teo_sarca>: you have eclipse
<trifon>: :)....
<teo_sarca>: jesus
<teo_sarca>: it's obvious
<trifon>: let's make method names trr2345
<vpj-cd>: this other
<trifon>:  :)
<vpj-cd>: MPPOrder forC_OrderLine_ID(Properties ctx, int C_OrderLine_ID, String trxName)
<teo_sarca>: @trifon, not the same
<trifon>: byC_OrderLine_ID
<trifon>: we were commeting the method with two parameters ond confusing name.
<vpj-cd>: other example
<vpj-cd>: public static MPPOrder forC_OrderLine_ID(Properties ctx, int C_OrderLine_ID, String trxName)
<vpj-cd>: {
<vpj-cd>: return new Query(ctx, MPPOrder.Table_Name, COLUMNNAME_C_OrderLine_ID+"=?", trxName)
<vpj-cd>: .setParameters(new Object[]{C_OrderLine_ID})
<vpj-cd>: .firstOnly();
<vpj-cd>: }
<teo_sarca>: @Trifon, so you want to change the "for" in "by"
<trifon>: @Teo. yes.
<teo_sarca>: great, do it ;)
<trifon>: but thisis just my personal taste.
<vpj-cd>: getMPPOrderByOrderLine
<trifon>: we can vote and see if "for" or "by" is prffered.
<CarlosRuiz>: Teo - if we have eclipse and javadoc - then we can name it just get(
<CarlosRuiz>: or we can name it just for(
<teo_sarca>: carlos, let me explain
<teo_sarca>: do not forget that the method is in a class
<teo_sarca>: the class is called MPPOrderBOMLine
<CarlosRuiz>: yes - why to use m_product_id in the name - but not c_order_id ?
<teo_sarca>: which binds table PP_Order_BOMLine
<teo_sarca>: (1 second)
<teo_sarca>: ok
<teo_sarca>: so
<vpj-cd>: we is voted for general naming convention
<teo_sarca>: when you are querying something regarding BOMLine
<vpj-cd>: for Adempiere
<teo_sarca>: Order bom line
<teo_sarca>: it is obvious that you will include the PP_Order_ID parameter
<teo_sarca>: that logic is in class name too
<teo_sarca>: but
<teo_sarca>: if you expect that you will hire linux kernel developers
<trifon>: @Teo: -1 from me. Order_ID must be in method name.
<teo_sarca>: and ask to continue
<teo_sarca>: libero development
<teo_sarca>: they do not understand the code
<vpj-cd>: is in general
<vpj-cd>: Teo
<vpj-cd>: is for Adempiere
<trifon>: when i use eclispe to autocomplete and need method which returns something by two paramters i will search for proper method by it's name. not by it's parameters.
<vpj-cd>: but can be best set this in developer forum
<vpj-cd>: and voting ask the all developers
<CarlosRuiz>: look - Teo - to make this easy - I would vote for the most simpler rule here
<CarlosRuiz>: the rule that you don't need to explain anybody - be linux kernel developer or anything
<teo_sarca>: yes
<CarlosRuiz>: and the simpler rule would be - describe accurately what the method does internally
<teo_sarca>: not agree
<trifon>: @Teo you are alone against 3...
<teo_sarca>: you need to know what libero is doing
<teo_sarca>: and the logic
<trifon>: not big chanhes.
<CarlosRuiz>: in this case the complete name would be
<CarlosRuiz>: getBomOrderLinesByOrderLineAndProduct (or something like that)
<teo_sarca>: @Trifon, so what ? i still vote for my suggestion ;)
<CarlosRuiz>: I know that's a long name to write - but we have eclipse to help us writing - and copy/paste
<trifon>: knowledge is something relative. you have it 2 years and is gone after 1...
<vpj-cd>: I think that for static method
<vpj-cd>: that return the selft objec
<teo_sarca>: nop, will be getBomOrderLinesByOrderLineAndProductAndTransactionInContext
<trifon>: @Carlos we can excluse returned type.
<vpj-cd>: we always use get
<vpj-cd>: as current core
<vpj-cd>: get()
<teo_sarca>: don't forget that the kernel developer is not familiar with ctx and trxname :)
<trifon>: they are in every method so they are something common.
<trifon>: no need to write them.
<teo_sarca>: i am trying to suggest that some fields are obvious
<CarlosRuiz>: right Trifon - when the returned type is an object is ok - but when is an int is not
<trifon>: @Carlos i agree.
<CarlosRuiz>: when the returning object is a primitive - then you must read javadoc
<vpj-cd>: when your returns a collection you get in plural
<teo_sarca>: @Trifon, did you see a method called 'for...' that is returning an int ?
<vpj-cd>: is getMPPOrders()
<CarlosRuiz>: look Teo - I'll put an example
<CarlosRuiz>: we all know that most of adempiere methods have Properties ctx and String trxname
<CarlosRuiz>: right?
<vpj-cd>: we do not should be set the signer method in the name
<vpj-cd>: for this is the description
<vpj-cd>: I think that is the standard of java
<CarlosRuiz>: but I've seen some methods have trxname at first, others at last
<CarlosRuiz>: and that's always a problem
<CarlosRuiz>: I need always to navigate to the method - or read the javadoc to realize what is String, String
<trifon>: sorry but i have to help in bath of Victoria. will be back in 40-60 minutes.
<CarlosRuiz>: ie. the getSQLValue methods
<vpj-cd>: so current core you use get
<vpj-cd>: public static MPPOrder forC_OrderLine_ID(Properties ctx, int C_OrderLine_ID, String trxName)
<vpj-cd>: {
<vpj-cd>: return new Query(ctx, MPPOrder.Table_Name, COLUMNNAME_C_OrderLine_ID+"=?", trxName)
<vpj-cd>: .setParameters(new Object[]{C_OrderLine_ID})
<vpj-cd>: .firstOnly();
<vpj-cd>: }
<vpj-cd>: should be
<CarlosRuiz>: where is the String for sql and where is the String for trxname?
<vpj-cd>: public static MPPOrder get(Properties ctx, int C_OrderLine_ID, String trxName)
<vpj-cd>: {
<vpj-cd>: return new Query(ctx, MPPOrder.Table_Name, COLUMNNAME_C_OrderLine_ID+"=?", trxName)
<vpj-cd>: .setParameters(new Object[]{C_OrderLine_ID})
<vpj-cd>: .firstOnly();
<vpj-cd>: }
<CarlosRuiz>: you can just realize it navigating to the code or reading the javadoc
<vpj-cd>: this is the standard now for core in ADempiere
<teo_sarca>: @trifon, ok go go
<vpj-cd>: we want change
<CarlosRuiz>: now - I suppose we can survive with that - but put a standard - Properties ctx ALWAYS at first - and String trxname ALWAYS at last
<vpj-cd>: public static MPPOrderBOMLine forM_Product_ID(Properties ctx, int PP_Order_ID, int M_Product_ID, String trxName)
<vpj-cd>: {
<vpj-cd>: final String whereClause = COLUMNNAME_PP_Order_ID+"=? AND "+COLUMNNAME_M_Product_ID+"=?";
<vpj-cd>: return new Query(ctx, Table_Name, whereClause, trxName)
<vpj-cd>: .setParameters(new Object[]{PP_Order_ID, M_Product_ID})
<vpj-cd>: .firstOnly();
<trifon>: 100% for Carlos suggestion.
<vpj-cd>: }
<trifon>: we must have standard for this.
<vpj-cd>: public static MPPOrderBOMLine get(Properties ctx, int PP_Order_ID, int M_Product_ID, String trxName)
<vpj-cd>: {
<vpj-cd>: final String whereClause = COLUMNNAME_PP_Order_ID+"=? AND "+COLUMNNAME_M_Product_ID+"=?";
<vpj-cd>: return new Query(ctx, Table_Name, whereClause, trxName)
<vpj-cd>: .setParameters(new Object[]{PP_Order_ID, M_Product_ID})
<trifon>: brb ..
<vpj-cd>: .firstOnly();
<vpj-cd>: }
<teo_sarca>: we have that standard :)
<teo_sarca>: @Victor, your last version is bad
<teo_sarca>: totally
<vpj-cd>: the JJ standard
<CarlosRuiz>: but most of DB.getSQLValue broke such rule (trxName as the last parameter)
<teo_sarca>: sorry, it's JJ some wide standard ;) ?
<vpj-cd>: all the static methos start with get
<teo_sarca>: ok
<vpj-cd>: /**
<vpj-cd>: * Get BPartner with Value
<vpj-cd>: * @param ctx context
<vpj-cd>: * @param Value value
<vpj-cd>: * @return BPartner or null
<vpj-cd>: */
<vpj-cd>: public static MBPartner get (Properties ctx, String Value)
<vpj-cd>: {
<vpj-cd>: if (Value == null || Value.length() == 0)
<vpj-cd>: return null;
<vpj-cd>: String whereClause = "Value=? AND AD_Client_ID=?";
<vpj-cd>: MBPartner retValue = new Query(ctx,MBPartner.Table_Name,whereClause.toString(),null)
<vpj-cd>: .setParameters(new Object[]{Value,Env.getAD_Client_ID(ctx)}).first();
<vpj-cd>: return retValue;
<vpj-cd>: } // get
<vpj-cd>: /**
<vpj-cd>: * Get BPartner with Value
<vpj-cd>: * @param ctx context
<vpj-cd>: * @param Value value
<vpj-cd>: * @return BPartner or null
<vpj-cd>: */
<vpj-cd>: public static MBPartner get (Properties ctx, int C_BPartner_ID)
<CarlosRuiz>: that must be called getByValue
<vpj-cd>: {
<teo_sarca>: victor, ok ok do not copy-paste entire adempiere ;)
<vpj-cd>: String whereClause = "C_BPartner_ID=? AND AD_Client_ID=?";
<vpj-cd>: MBPartner retValue = new Query(ctx,MBPartner.Table_Name,whereClause.toString(),null)
<vpj-cd>: .setParameters(new Object[]{C_BPartner_ID,Env.getAD_Client_ID(ctx)}).first();
<teo_sarca>: so
<vpj-cd>: return retValue;
<vpj-cd>: } // get
<vpj-cd>: and when you need return
<teo_sarca>: for you
<vpj-cd>: we using names
<vpj-cd>: getBPartnerCashTrx(Properties, int)
<CarlosRuiz>: I suppose that's precisely the point
<vpj-cd>: public static MBPartner getBPartnerCashTrx (Properties ctx, int AD_Client_ID)
<vpj-cd>: {
<vpj-cd>: MBPartner retValue = null;
<vpj-cd>: String sql = "SELECT * FROM C_BPartner "
<CarlosRuiz>: we're not saying that we're conform with JJ naming convention
<vpj-cd>: + "WHERE C_BPartner_ID IN (SELECT C_BPartnerCashTrx_ID FROM AD_ClientInfo WHERE AD_Client_ID=?)";
<vpj-cd>: PreparedStatement pstmt = null;
<vpj-cd>:
<vpj-cd>: even when you need other object
<vpj-cd>: * Get Not Invoiced Shipment Value
<vpj-cd>: * @param C_BPartner_ID partner
<CarlosRuiz>: we're trying to define a better naming convention here
<vpj-cd>: * @return value in accounting currency
<vpj-cd>: */
<vpj-cd>: public static BigDecimal getNotInvoicedAmt (int C_BPartner_ID)
<CarlosRuiz>: well - some of we are
<vpj-cd>: {
<teo_sarca>: Victor, please stop a little
<CarlosRuiz>: Victor is playing
<teo_sarca>: yes
<teo_sarca>: 10x
<CarlosRuiz>: please -I need to do a migration today
<teo_sarca>: so
<vpj-cd>: public static MProduct get (Properties ctx, int M_Product_ID)
<vpj-cd>: {
<teo_sarca>: ok
<vpj-cd>: /**
<vpj-cd>: * Get MProduct from Cache
<vpj-cd>: * @param ctx context
<vpj-cd>: * @param whereClause sql where clause
<vpj-cd>: * @param trxName trx
<vpj-cd>: * @return MProduct
<vpj-cd>: */
<vpj-cd>: public static MProduct[] get (Properties ctx, String whereClause, String trxName)
<vpj-cd>: {
<CarlosRuiz>: Victor - I must go
<vpj-cd>: int AD_Client_ID = Env.getAD_Client_ID(ctx);
<vpj-cd>: List<MProduct> list = new Query(ctx, Table_Name, "AD_Client_ID=? AND "+whereClause, trxName)
<vpj-cd>: .setParameters(new Object[]{AD_Client_ID})
<teo_sarca>: i need to go in a couple of minutes too
<vpj-cd>: .list();
<vpj-cd>: return list.toArray(new MProduct[list.size()]);
<vpj-cd>: } // get
<vpj-cd>: this is a good example
<vpj-cd>: /**
<vpj-cd>: * Get MProduct using UPC/EAN (case sensitive)
<teo_sarca>: @Carlos, victor is too busy to listen to you ;)
<CarlosRuiz>: :-D this is the bazaar
<vpj-cd>: * @param ctx Context
<vpj-cd>: * @param upc The upc to look for
<vpj-cd>: * @return List of MProduct
<vpj-cd>: */
<vpj-cd>: public static List<MProduct> getByUPC(Properties ctx, String upc, String trxName)
<vpj-cd>: {
<vpj-cd>: String whereClause = "AD_Client_ID=? AND UPC=?";
<vpj-cd>: Query q = new Query(ctx, Table_Name, whereClause, trxName);
<vpj-cd>: q.setParameters(new Object[]{Env.getAD_Client_ID(ctx), upc});
<vpj-cd>: return(q.list());
<CarlosRuiz>: that's why I'm not used to the bazaar way
<vpj-cd>: }
<CarlosRuiz>: :-D
<vpj-cd>: I only want define some basic rule
<teo_sarca>: but i will tell him that you need to migrate some database ;)
<vpj-cd>: as Carlos say
<teo_sarca>: victor
<vpj-cd>: this way the developer can to speak the same language
<vpj-cd>: but if this do not is issue
<CarlosRuiz>: well - honestly I don't understand what was the meaning of the copy/paste party - just to annoy us
<vpj-cd>: then we can continue with the criterial the each developer
<banym>: hi all
<vpj-cd>: Carlos I set the example
<vpj-cd>: the standard
<vpj-cd>: Teo
<teo_sarca>: @Carlos, i am so tired that is hard to annoy me
<vpj-cd>: because he ask what standard
<vpj-cd>: you only need see the MProduct
<vpj-cd>: and MBPartner
<CarlosRuiz>: hehehehe - good strategy - the problem is that I'm not tired - I need to start a migration  :-D
<vpj-cd>: all the static method is in the top
<vpj-cd>: and use words
<teo_sarca>: ok
<CarlosRuiz>: I didn't read your copy/paste Victor - that was unnecessary
<CarlosRuiz>: I've read such code thousands of times
<CarlosRuiz>: and we all know that they're not the best naming convention
<vpj-cd>: getByUPC(Properties ctx, String upc, String trxName)
<teo_sarca>: yes
<teo_sarca>: that was added recently
<vpj-cd>: then we need rename for_UPC
<vpj-cd>: :-)
<teo_sarca>: nop
<teo_sarca>: forUPC(Properties ctx, String upc, String trxName)
<CarlosRuiz>: precisely when I read MBPartner get(Properties, String, String)
<CarlosRuiz>: I don't know WTF the second String is
<vpj-cd>: or this
<vpj-cd>: public static BigDecimal getNotInvoicedAmt (int C_BPartner_ID)
<teo_sarca>: that's another discussion victor
<vpj-cd>: for public static BigDecimal getC_BPartner_ID_InvoicedAmt (int C_BPartner_ID)
<teo_sarca>: we are talking about accessors
<teo_sarca>: victor
<CarlosRuiz>: that's also a good point, when I see
<CarlosRuiz>: BigDecimal get(int)
<CarlosRuiz>: WTF is BigDecimal and WTF is int?
<CarlosRuiz>: so - that's why I say I think we can vote for the most simpler rule
<vpj-cd>: I think we is to told for define better naming convention
<CarlosRuiz>: to make things easier for everybody
<teo_sarca>: yep
<CarlosRuiz>: for me the most simpler rules is - describe what does the method internally
<teo_sarca>: ok
<CarlosRuiz>: getInvoicedAmtByBPartnerID
<teo_sarca>: i think we need to put that in forums
<vpj-cd>: ok then the conclusion let create a post to voting this
<teo_sarca>: and then vote
<vpj-cd>: do you agree?
<teo_sarca>: victor, unfortunatelly
<teo_sarca>: the conclusion is that there is no conclusion ;)
<teo_sarca>: yes
<vpj-cd>: jajajaja
<teo_sarca>: post in forums
<CarlosRuiz>: I supposed this is going to happen - when Daniel opened the issue about conventions
<CarlosRuiz>: that's very personal taste issue
<teo_sarca>: yes
<CarlosRuiz>: that's why I don't hope to agree on hard rules
<CarlosRuiz>: let's say just some simpler rules
<CarlosRuiz>: to me a golden rules is - make your code readable
<CarlosRuiz>: understandable
<teo_sarca>: naming conventions are very important too
<CarlosRuiz>: whatever you name a method - if I can understand what is doing internally is ok
<teo_sarca>: and coding convetions
<teo_sarca>: else we get spaghetti
<vpj-cd>: yes Carlos
<CarlosRuiz>: the problem is about methods that you can realize what they're doing
<vpj-cd>: as you said
<vpj-cd>: make your code readable
<vpj-cd>: understandable
<CarlosRuiz>: or what the parameters are
<vpj-cd>: I can understand what is doing internally
<CarlosRuiz>: or we must trust completely on javadoc - so we can't use libraries
<vpj-cd>: forC_OrderLine_ID for me this method
<vpj-cd>: no cumple your gold rule
<vpj-cd>: forC_OrderLine_ID
<vpj-cd>: I do no undersntad
<vpj-cd>: and is confusing
<vpj-cd>: but can be I is mistake
<CarlosRuiz>: yes Victor, that naming convention is very specific for some conditions
<vpj-cd>: :-)
<teo_sarca>: tell me that you do not understand it now
<teo_sarca>: i need 10 seconds to explain you
<teo_sarca>: so
<CarlosRuiz>: hehehe - better if you don't need to explain it
<vpj-cd>: I listen
<teo_sarca>: nop
<teo_sarca>: as a coder
<vpj-cd>: as say Carlos
<teo_sarca>: you need to read our
<CarlosRuiz>: the best is if the name is auto-documented
<teo_sarca>: coding conventions (TODO)
<vpj-cd>: when you need explain then you need a context
<teo_sarca>: carlos, name won't be always autodocumented
<vpj-cd>: I need the Teo context for understand :-)
<teo_sarca>: because if you are trying to do it
<CarlosRuiz>: same for variables - most of us are lazy about variable names - the golden rule must be a variable name must say what it contains
<teo_sarca>: you will end in writing novels and not code ;)
<CarlosRuiz>: Teo - according to extreme programming practices - you must not write any comment  :-)
<teo_sarca>: agree
<CarlosRuiz>: those are extreme
<teo_sarca>: nop
<teo_sarca>: i agree with them
<teo_sarca>: but in extreme programming
<CarlosRuiz>: exactly - then you must not write javadoc
<teo_sarca>: there is a rule
<CarlosRuiz>: or write a comment to explain a variable
<teo_sarca>: that says: DO NOT READ PROJECT CODING CONVENTIONS ?
<teo_sarca>: there is such a rule ?
<vpj-cd>: other ask you
<vpj-cd>: if(activity.getDocStatus().equals(MPPOrderNode.DOCACTION_Complete)) if(MPPOrderNode.DOCACTION_Complete.equals(activity.getDocStatus()))
<vpj-cd>: when we use the static costant
<vpj-cd>: when no use
<CarlosRuiz>: Victor - that's to avoid NPE
<teo_sarca>: yes
<CarlosRuiz>: if activity.getDocStatus is null then the first way NPE will be raised, the second will return false
<red1>: Hi all, just back and reading abit here... to add one more thing is "consistency " <-- if not stated yet
<CarlosRuiz>: you can see somewhere in code something like
<CarlosRuiz>: "Y".equals(answer) - to avoid error if answer is null
<vpj-cd>: thank a lot Carlos noe I understand
<teo_sarca>: yes
<vpj-cd>: noe = now
<teo_sarca>: that's the idea
<vpj-cd>: my dobut is for you have not using alwayse
<vpj-cd>: same criterial teo
<vpj-cd>: :-) this my consusion
<vpj-cd>: trunk/base/src/org/eevolution/model/MPPCostCollector.java line 207
<teo_sarca>: hehe
<vpj-cd>: :-)
<teo_sarca>: that's why i am not going closed source .... you can correct it ;)
<vpj-cd>: 212
<teo_sarca>: hehe
<vpj-cd>: yes now I know the reason
<vpj-cd>: then I can understand and can fix
<vpj-cd>: :-)
<vpj-cd>: thank a lot carlos for the tip
<CarlosRuiz>: and we can write a hint for developers about using CONSTANT.equals(variable) instead of variable.equals(CONSTANT)
<teo_sarca>: yes
<CarlosRuiz>: but that's a hint - it's not mandatory
<vpj-cd>: like I said before I dono good developer
<vpj-cd>: but I want improve
<teo_sarca>: nop Carlos, i think is mandatory
<banym>: red1: did you find your scripts?
<teo_sarca>: @Victor, and you are improving your code in big steps
<CarlosRuiz>: is best practice - better if everybody follow it
<vpj-cd>: ie
<vpj-cd>: this is other good practice
<teo_sarca>: @Carlos,
<vpj-cd>: String whereClause ="AND PP_Order_ID=? AND PP_Order_Node_ID<>?" final String whereClause ="PP_Order_ID=? AND PP_Order_Node_ID<>?"
<vpj-cd>: set final in all SQL string
<red1>: banym: just reading Carlos answer and trying the tip
<teo_sarca>: yes
<CarlosRuiz>: not all - just Strings that are supposed to not be modified
<teo_sarca>: i always use final keyword
<teo_sarca>: even for variables
<vpj-cd>: yes when a sql do not need
<teo_sarca>: just to make sure i am not modifing it
<CarlosRuiz>: can you then do
<CarlosRuiz>: whereClause = whereClause + "..."
<vpj-cd>: then use final
<teo_sarca>: yes
<CarlosRuiz>: well - another tip  :-) when you need modifiable String's - is a lot better in performance to use StringBuffer
<vpj-cd>: other issue to evaluating the convention
<vpj-cd>: String whereClause = MPPOrderNodeProduct.COLUMNNAME_PP_Order_Node_ID+"=?"
<CarlosRuiz>: at least that's what I read somewhere
<teo_sarca>: yes
<vpj-cd>: Carlos say use simple field name
<teo_sarca>: I vote +1
<vpj-cd>: Teo use static name
<teo_sarca>: carlos will vote -1
<teo_sarca>: i know the story ;)
<CarlosRuiz>: Teo and I have a disagreement about that - but is better not to discuss it  :-D
<vpj-cd>: jajajaja
<CarlosRuiz>: that's why I didn't answer
<CarlosRuiz>: I prefer readability - Teo prefers compiler catch of potential errors
<vpj-cd>: but I am newbies how is the people I need go
<vpj-cd>: :-)
<teo_sarca>: i answer it ;)
<teo_sarca>: for both of us
<vpj-cd>: the ther question
<vpj-cd>: 342 m_processMsg = "Cannot correct Inventory (MA)"; throw new AdempiereException(); //Cannot correct Inventory (MA)
<vpj-cd>: return DocAction.STATUS_Invalid;
<CarlosRuiz>: well - I'm also with compiler catch - when the risk is big - but in this case I think the risk is low
<vpj-cd>: the ADempiere focust is use m_processMsg
<vpj-cd>: this way I know that is happen
<vpj-cd>: but teo replace for AdempiereException(); //Cannot correct Inventory (MA)
<CarlosRuiz>: for that - I don't know if the user is notified about "the cause"
<vpj-cd>: this are some the my doubts as newbies developer
<teo_sarca>: victor, can you give me the class & line no
<teo_sarca>: ?
<CarlosRuiz>: the preferred method must be to notify the user about the error - the cause - and if possible a proposed solution
<vpj-cd>: trunk/base/src/org/eevolution/model/MPPCostCollector.java
<vpj-cd>: 342
<vpj-cd>: I sent in my question for email
<teo_sarca>: ok
<teo_sarca>: so the problem is with MStorage
<teo_sarca>: method
<teo_sarca>: MStorage.add
<teo_sarca>: should throw exception if is necesary
<teo_sarca>: the main idea
<CarlosRuiz>: in this case I don't know the exact difference
<CarlosRuiz>: I see first way the user will see "Cannot correct Inventory (MA)"
<CarlosRuiz>: with the AdempiereException what does the user see?
<teo_sarca>: Carlos:
<teo_sarca>: public AdempiereException() {
<teo_sarca>: this(getMessageFromLogger());
<teo_sarca>: }
<teo_sarca>: that's current implementation
<vpj-cd>: but now
<vpj-cd>: is throw new AdempiereException()
<CarlosRuiz>: so - you need to log first the error - is it logged (sorry , I'm not reading the code)
<teo_sarca>: if the MStorage
<teo_sarca>: fails
<CarlosRuiz>: now - can't we make an throw new AdempiereException("Cannot correct Inventory (MA)");
<teo_sarca>: then the Mstorage.add
<teo_sarca>: method will log an error
<teo_sarca>: (that's JJ aproach to error ghandling)
<teo_sarca>: @Carlos,
<teo_sarca>: the best approach
<teo_sarca>: is to let MStorage.add
<CarlosRuiz>: Teo - I think is better the message shown to user is constructed above
<vpj-cd>: my other question when the developer need implment a Adempiere Exception
<teo_sarca>: to throw exception is needed
<CarlosRuiz>: look at this scenario
<CarlosRuiz>: MStorage.add is called in MPPCostCollector in lines 100, 200, 300
<vpj-cd>: the msg Cannot correct Inventory (MA) should be translate from AD_Message?
<CarlosRuiz>: just with the message from MStorage you can't realize where MPPCostCollector failed
<teo_sarca>: carlos
<CarlosRuiz>: that's why JJ wrote
<CarlosRuiz>: "Cannot correct Inventory (MA) (1)"
<CarlosRuiz>: "Cannot correct Inventory (MA) (2)"
<teo_sarca>: and if
<teo_sarca>: great
<teo_sarca>: so now it's all clear for user :)
<teo_sarca>: better if\
<teo_sarca>: the mstorage.addf
<teo_sarca>: .add
<CarlosRuiz>: that way - if the user informs you the complete error you can know where it failed
<teo_sarca>: method will say WHY was a problem
<vpj-cd>: Carlos
<vpj-cd>: but the case for JJ do not solve the issue
<CarlosRuiz>: I think we give support mostly for users that don't have console open - or even don't know how to open console - or how to copy the console
<vpj-cd>: because I have a user only seack spanish
<vpj-cd>: when he see Cannot correct Inventory
<vpj-cd>: is the same result
<vpj-cd>: ;-)
<teo_sarca>: yes
<teo_sarca>: agree
<teo_sarca>: also
<teo_sarca>: we need to let
<teo_sarca>: the exception pass
<teo_sarca>: from the place that produce
<teo_sarca>: it to the use rwindow
<teo_sarca>: as a 'show details" button
<teo_sarca>: that is the target
<teo_sarca>: that's my vision ;)
<CarlosRuiz>: Teo - can't we do something like this?
<CarlosRuiz>: throw new AdempiereException("Cannot correct Inventory (MA)");
<CarlosRuiz>: and show the user that message - and below the message from MStorage.add
<CarlosRuiz>: can that be done?
<teo_sarca>: also in future we can extend this
<teo_sarca>: with suggestions
<vpj-cd>: then I understand
<vpj-cd>: that we need new ADempiere exceptions class
<teo_sarca>: hmmmm
<CarlosRuiz>: but you still haven't answered my question
<CarlosRuiz>: what is being notified to the user?
<CarlosRuiz>: ORA-9001 - Cryptic oracle error  ??
<vpj-cd>: and this should be in multi language
<teo_sarca>: why not throw exception from MStorage.add ?
<vpj-cd>: again this are my doubts as newbies developer :-)
<CarlosRuiz>: and as Victor said - even better if we do something like
<CarlosRuiz>: throw new AdempiereException(Msg.translate("CannotCorrectInventory1"));
<vpj-cd>: this the reason the create the ADempiere Best Practice
<CarlosRuiz>: but that must be done just for frequent errors - otherwise the message table will be really big and the development will be really hard
<vpj-cd>: I take your offer Teo Will great if you can set in wiki page
<vpj-cd>: :-)
<vpj-cd>: or other
<red1>: banym: i think i found it .. thaks to CarlosRuiz
<teo_sarca>: @victor, i will try in this weekend
<CarlosRuiz>: @Teo -> why not throw exception from MStorage.add ?
<CarlosRuiz>: what is notified to user? some cryptic oracle error?
<CarlosRuiz>: now, if you call MStorage.add in 50 parts of your program - how do you know where it failed just with the user message?
<CarlosRuiz>: red1 - and I don't know Mac  :-D
<vpj-cd>: Teo is important
<teo_sarca>: @Carlos, why criptic error ?
<vpj-cd>: the same way that the functional consultant need document
<banym>: red1: where? i am just uploading a smal howto where i found them here on my system.
<teo_sarca>: the MStorage.add
<vpj-cd>: the business logic for the other can undernstand
<teo_sarca>: should say WHY exactly can not add product
<teo_sarca>: which product
<teo_sarca>: what Qty
<teo_sarca>: etc
<teo_sarca>: etc
<red1>: i will reply in my thread... in SF.. pls read there :)
<vpj-cd>: or ie as Carlos ask me in the past where is the exemple for manufacturing
<CarlosRuiz>: Teo - I'm asking - what is shown to user when MStorage.add raise the exception
<vpj-cd>: I can said what do not undestand all is clear
<vpj-cd>: so I change my vision and now am create the examples
<vpj-cd>: or I try
<vpj-cd>: :-)
<banym>: red1: o.k i will add my video there, too
<teo_sarca>: @Carlos
<teo_sarca>: lookin inside MStorage.add method:
<teo_sarca>: and track possible errors (i.e. return false ;) ):
<teo_sarca>: if (storage.getM_Locator_ID() != M_Locator_ID
<teo_sarca>: && storage.getM_Product_ID() != M_Product_ID
<teo_sarca>: && storage.getM_AttributeSetInstance_ID() != M_AttributeSetInstance_ID)
<teo_sarca>: {
<teo_sarca>: s_log.severe ("No Storage found - M_Locator_ID=" + M_Locator_ID
<teo_sarca>: + ",M_Product_ID=" + M_Product_ID + ",ASI=" + M_AttributeSetInstance_ID);
<teo_sarca>: return false;
<teo_sarca>: }
<teo_sarca>: -------------
<teo_sarca>: return storage.save (trxName);
<teo_sarca>: so in first case
<teo_sarca>: the user will see : No Storage found - M_Locator_ID=" + M_Locator_ID
<teo_sarca>: <teo_sarca> + ",M_Product_ID=" + M_Product_ID + ",ASI=" + M_AttributeSetInstance_ID);
<teo_sarca>: ----
<teo_sarca>: in second case
<teo_sarca>: the user will see that .save method throws
<teo_sarca>: that means
<teo_sarca>: SQL ERROR (in 5% of cases)
<teo_sarca>: exceptions throwed by model validators
<teo_sarca>: etc
<teo_sarca>: WDYT ?
<CarlosRuiz>: I think the best would be if we can show the user some stack of logged errors
<CarlosRuiz>: and every layer catching the exception can add a message
<vpj-cd>: yes
<teo_sarca>: yes
<vpj-cd>: in general I go the tools and log history
<vpj-cd>: to try fonund the error
<teo_sarca>: but handling errors as boolean returns
<teo_sarca>: is a bad practice
<vpj-cd>: or the issue
<teo_sarca>: and you know it ;)
<CarlosRuiz>: so - if I write
<CarlosRuiz>: throw new AdempiereException("Cannot correct Inventory (MA) (1)");
<CarlosRuiz>: can the exception show this message - and the message from MStorage.add ??
<CarlosRuiz>: I mean
<CarlosRuiz>: if I write
<CarlosRuiz>: throw new AdempiereException("Cannot correct Inventory (MA)");
<CarlosRuiz>: the user must be notified like this:
<CarlosRuiz>: Cannot correct Inventory (MA)
<CarlosRuiz>: No Storage found - M_Locator_ID=" + M_Locator_ID+ ",M_Product_ID=" + M_Product_ID + ",ASI=" + M_AttributeSetInstance_ID
<teo_sarca>: @carlos, obviously we can make it to show that error
<CarlosRuiz>: that will be a big improvement on current user notification
<teo_sarca>: but my question is WHY ?
<teo_sarca>: why
<teo_sarca>: MStorage.add
<teo_sarca>: should return a boolean value
<teo_sarca>: signaling an erro
<teo_sarca>: error
<CarlosRuiz>: or the user will see this:
<CarlosRuiz>: Cannot correct Inventory (MA) (1)
<CarlosRuiz>: ORA-9001 ERROR - Primary MSTORAGE_PKEY key violated (just an example)
<teo_sarca>: that's bad practice
<teo_sarca>: @Carlos
<teo_sarca>: considering that you are a regular user
<CarlosRuiz>: Teo - I'm with you - but with an improvement
<teo_sarca>: and you get
<teo_sarca>: option 1: Cannot correct Inventory (MA) (1)
<teo_sarca>: option 2: > ORA-9001 ERROR - Primary MSTORAGE_PKEY key violated (just an example)
<teo_sarca>: which one will help you ?
<CarlosRuiz>: I know is better as you replaced the code by Victor to throw an exception - but that dropped a useful message for supporting users
<teo_sarca>: which one will help you ?
<CarlosRuiz>: no - it must not help the user - it must help us giving support
<teo_sarca>: ok
<CarlosRuiz>: both help us - one is incomplete
<teo_sarca>: so you as a support man
<teo_sarca>: which one will help you ?
<CarlosRuiz>: both - just showing one is incomplete
<teo_sarca>: IMHO best help is stack trace
<teo_sarca>: BEST!
<vpj-cd>: what this the way rigth
<teo_sarca>: IMHO: option 1: Cannot correct Inventory (MA) (1)
<CarlosRuiz>: the best is to show both errors - that will improve our support capabilities
<teo_sarca>: is nothing
<vpj-cd>: right?
<teo_sarca>: yes
<CarlosRuiz>: Teo - look the (1) I added at the end
<vpj-cd>: what is the best practice to management a exception
<CarlosRuiz>: that's intended to notify support WHERE in MPPCostCollector failed
<vpj-cd>: if exist will great if is document
<teo_sarca>: @Carlos,
<teo_sarca>: i am proposing to throw error
<teo_sarca>: from MStorage.add
<CarlosRuiz>: MStorage.add can be called in 50 lines in MPPCostCollector - you won't know where it failed
<teo_sarca>: method
<vpj-cd>: other newbies user as I can follow and impove your code
<teo_sarca>: and eliminate JJ;s error handling
<teo_sarca>: if (error)
<teo_sarca>: {
<teo_sarca>: log error
<teo_sarca>: and discard
<teo_sarca>: }
<CarlosRuiz>: Teo - I'm saying you're right - but we must have the ability to add useful messages above
<teo_sarca>: yes
<teo_sarca>: the problem is with displaying error
<CarlosRuiz>: just throwing AdempiereException() is not saying WHERE the MPPCostCollector failed
<teo_sarca>: the error window
<teo_sarca>: should have 'show details' button
<teo_sarca>: where you can get entire stack trace
<teo_sarca>: agree ?
<teo_sarca>: @Carlos,
<teo_sarca>: if we throw error from MStorage.add
<teo_sarca>: than we can drop
<teo_sarca>: the throw new AdempiereException()
<teo_sarca>: from cost collector class
<CarlosRuiz>: Teo - again - is important to know WHERE the MPPCostCollector failed
<CarlosRuiz>: as it's important to know WHERE MStorage.add failed - and WHY
<teo_sarca>: it's important for you as support
<teo_sarca>: so the error window
<teo_sarca>: should display the stack trace
<teo_sarca>: WHY? : the answer will be on root of stack trace
<CarlosRuiz>: for me the best scenario is to show Victor's message to the user:
<CarlosRuiz>: "Cannot correct Inventory (MA) (1)"
<CarlosRuiz>: and have the ability to read the entire stack of messages
<CarlosRuiz>: but that's a problem when the messages are not shown in an error window - but in status line (red messages in status line)
<teo_sarca>: @Carlos
<CarlosRuiz>: currently "Complete" errors are showin in red in status line
<teo_sarca>: i think you are trying to solve display issues in program logic ;)
<teo_sarca>: that's the problem of status bar
<teo_sarca>: maybe we need to improve that
<teo_sarca>: when you click on it
<teo_sarca>: to display
<teo_sarca>: more details
<teo_sarca>: or something
<teo_sarca>: but do not complicate business logic for that
<teo_sarca>: ie
<teo_sarca>: e golden rule
<teo_sarca>: regarding exceptions handling says
<teo_sarca>: that if you can not solve the issue
<teo_sarca>: than do not catch it;)
<CarlosRuiz>: not agree
<teo_sarca>: like JJ did in a lot of places in code
<CarlosRuiz>: messages are catched to show something meaningful to user
<CarlosRuiz>: to avoid showing users ORA-9001 XXX errors
<teo_sarca>: in this particular case
<teo_sarca>: you are avoiding the ORA-9001
<teo_sarca>: message
<teo_sarca>: by saying
<teo_sarca>: "cannot create MA"
<teo_sarca>: (1)
<CarlosRuiz>: yes - that's better FOR USER
<teo_sarca>: (2)
<teo_sarca>: (3)
<teo_sarca>: but the idea is that
<CarlosRuiz>: and with some meaningful messages it can be better for support
<teo_sarca>: he can not complete cost collector ;)
<CarlosRuiz>: look a golden rule - never show database errors to users
<CarlosRuiz>: never show a NPE to user
<teo_sarca>: for support, the best information is stack trace
<teo_sarca>: regarding DB errors
<teo_sarca>: if you get them
<teo_sarca>: than means:
<teo_sarca>: 1. you have a bug
<teo_sarca>: 2. integrity constraint error
<teo_sarca>: and this case
<CarlosRuiz>: yes - but the user must not be notified as ORA-9001 error
<CarlosRuiz>: the user must be notified as
<CarlosRuiz>: "Cannot add storage"
<teo_sarca>: the problem is that we do not have constraints metadata in application dictionary
<teo_sarca>: my opinion is that we are loading business logic with not necessary code just to solve other issues
<CarlosRuiz>: "Cannot correct inventory" is better than "No Storage for XXX" or even better than "ORA-9001"
<teo_sarca>: and this is not correct
<CarlosRuiz>: no Teo - I think what JJ did is to show meaningful errors to users
<teo_sarca>: ok
<CarlosRuiz>: just that he did with boolean - but we can do it with exceptinos
<teo_sarca>: i think we need to discuss this on forums too
<teo_sarca>: ;)
<CarlosRuiz>: it's the same - but we must avoid showing cryptic errors to users
<teo_sarca>: in 99% of cases
<teo_sarca>: we have criptic errors
<teo_sarca>: because we do not have constraints metadata info
<teo_sarca>: in application dictionary
<CarlosRuiz>: ok - that's a usability thing - AFAIU - users must be notified about the process failed - overwhelming them with database errors panics them
<vpj-cd>: ok until unique law here is the criterial of the developer
<vpj-cd>: :-)
<teo_sarca>: Carlos
<teo_sarca>: but again
<CarlosRuiz>: and support people must have a mechanism to determine the cause - that AD_Issue for
<teo_sarca>: this can be solved in error reporting window
<teo_sarca>: e.g.
<teo_sarca>: if (ex instanceof DBException) {
<teo_sarca>: show "bla bla bla";
<CarlosRuiz>: Teo - Victor - not criteria of developer
<teo_sarca>: }
<CarlosRuiz>: I think that's really important to show professional face
<CarlosRuiz>: you must not show database errors - Null Pointer Exception errors - etc - to users
<teo_sarca>: carlos
<teo_sarca>: but this is a displaying issue\
<CarlosRuiz>: JJ implemented via boolean - Teo is proposing to change it to exceptions
<CarlosRuiz>: I agree but following the golden rule - don't show system errors to users
<teo_sarca>: and not a business logic issue
<teo_sarca>: @Carlos
<CarlosRuiz>: Teo - your exception catching is potentially showing database errors to users
<teo_sarca>: you can hide system errors
<CarlosRuiz>: so - I propose you change that to throw new AdempiereException("Cannot ...")
<teo_sarca>: carlos, we can hide system exceptions
<teo_sarca>: from error reporting window
<teo_sarca>: i propose to throw errors from MStorage directly
<CarlosRuiz>: otherwise - Victor's way is better IMHO
<teo_sarca>: and catch then on top level ;)
<CarlosRuiz>: and show what?
<teo_sarca>: 1.
<teo_sarca>: No Storage found - M_Locator_ID=" + M_Locator_ID
<teo_sarca>: + ",M_Product_ID=" + M_Product_ID + ",ASI=" + M_AttributeSetInstance_ID);
<teo_sarca>: which can be improved
<teo_sarca>: and resolve the IDs
<teo_sarca>: 2. errors throwed by save method
<teo_sarca>: which can come from :
<teo_sarca>: 1. database layer
<teo_sarca>: 2. model validators
<CarlosRuiz>: now I understand there is an issue with the proposal of changing everything to exceptions - the top layers must catch and show meaningful messages to users - that's what I asked if Adempiere is prepared to do that - are we sure all top layers catch exceptions? I doubt it
<teo_sarca>: yes they are
<teo_sarca>: i use this in my verticals
<CarlosRuiz>: no - for me the message "No Storage found ..." is not good for user
<CarlosRuiz>: the message must be "Cannot add storage (1)" or something like that
<teo_sarca>: i always throw exceptions ;)
<CarlosRuiz>: yes - but in your code - I have seen NPE shown to users in adempiere
<CarlosRuiz>: so - there must be some top layer not converting such system messages
<teo_sarca>: yes\
<teo_sarca>: but we need to hide NPE there
<teo_sarca>: look
<teo_sarca>: ADempeire is process oriented
<CarlosRuiz>: exactly - as I understand that's the meaning of AdempiereException
<CarlosRuiz>: you must catch an SQLException and raise an AdempiereException with a proper message
<teo_sarca>: the meaning of new AdempiereException() is a temporary solution to migrate from JJ's error handling
<CarlosRuiz>: you can catch a NPEException and throw AdempiereException with a proper message for user
<vpj-cd>: Teo need create the documentation
<CarlosRuiz>: but what I doubt is Adempiere is prepared
<teo_sarca>: do you have some examples?
<CarlosRuiz>: Adempiere is showing "Null Pointer Exception" in red in status line
<CarlosRuiz>: when you have errors in ModelValidator
<teo_sarca>: we can improve that....
<CarlosRuiz>: exactly - that's what I say
<CarlosRuiz>: "Null Pointer Exception" message - must be catched and changed to "Cannot save record"
<CarlosRuiz>: or whatever explanatory message shown to user
<teo_sarca>: ok
<CarlosRuiz>: and support people must have a way to read somewhere that Null Pointer Exception was raised in ModelValidator line XXXX
<teo_sarca>: true
<teo_sarca>: ok
<CarlosRuiz>: good discussion we have had today  :-)
<teo_sarca>: yep
<teo_sarca>: guys i need to go
<teo_sarca>: victor,
<teo_sarca>: regarding the modified logic from
<teo_sarca>: cost collector
<teo_sarca>: i will review it tomorrow
<teo_sarca>: and send you an email
<teo_sarca>: ok ?
<teo_sarca>: ok
<teo_sarca>: then have a nice day
<teo_sarca>: bye bye
<CarlosRuiz>: I have to go also - c u later