View source for IRC Meeting Full 20090109

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


[[Category:Meetings]]