Hello Taher, Yes certainly you cleared my doubts, and Yes I am confirming your point with the feedback.
What I get after this entire discussion is, I was concentrating on Refactoring and you are emphasizing on Re-Engineering (it will not be an OFBiz Update with new tricks it will be an OFBiz Upgrade with workflow and may involve some new tricks). If this is what you meant, then I observe the pathway will be challenging but, I guess everyone will understand it will be pleasant to do so. Looking forward to your approval. It would be a great help if you could propose an action plan. Thanks & Regards, -- *Pradhan Yash Sharma* On Tue, May 1, 2018 at 3:59 PM Taher Alkhateeb <slidingfilame...@gmail.com> wrote: > Hello Yash, > > So you are proving my point with your feedback. What do I mean by that? > > You are focusing on things like the below and then you are submitting > a patch accordingly. > - use lambda expressions > - Type inference > - Using quotes > - whatever else pattern you seek > > My recommendation instead is to rework this into: > - I want to refactor this class, and make sure all the code is cleaned up. > - I want to make sure all the methods in this class have a correct > simple signature and I need to make sure everything else works > - These three classes over there are responsible for initiating X, but > they are too complex and have many flaws, we need to set public fields > to private and do A B C ... > > The difference between the two above approaches is subtle but very > important. In the first case you are looking at a pattern and you want > to apply it everywhere. The problem is that with this mentality you > are not looking deeply into the code because you are focused on the > pattern. In the second case, you are in fact isolating a single class > or group of related classes and refactoring them very carefully, you > don't care about patterns, you just want to improve the code. > > My recommendation is to change the strategy such that your refactoring > one-piece-at-a-time, not one-pattern-at-a-time. I hope this clears up > my perspective > > On Mon, Apr 30, 2018 at 9:21 AM, Yash Sharma > <yash.sha...@hotwaxsystems.com> wrote: > > * > > I am not pen downing things, but yea I am really full on high energy to > > work on these front. > > > > > > Thanks & Regards, > > -- > > *Pradhan Yash Sharma* > > > > On Mon, Apr 30, 2018 at 11:37 AM, Yash Sharma < > yash.sha...@hotwaxsystems.com > >> wrote: > > > >> Hello, > >> > >> Thank you for the response, I was wondering about the volume of > >> refactoring we can do at each component, so let's apply Divide and > Conquer > >> approach for each component upgrading work. > >> > >> I can see a few patterns for the update which I've listed down in my > >> previous mail. We can pick any piece of code and apply focused > refactoring > >> on each component, and then we can do it with others as well when we are > >> through with one. It would be a great help if you could suggest a > sequence > >> to do so for example : > >> *I --> 4.) Use of single quote for a character *(this is a > >> straightforward work and the easiest one :) ). > >> *II --> **3.) Using Type Inference *(We can pick this as it will never > >> impact any working code). > >> III --> *5.) Updated Variable Declaration.* > >> *IV --> **1.) **Downsize Accessibility Scope *(If tested this is also a > >> not a big deal). > >> *V --> **2.) Using **Lambda Expressions *(This can impact on working > >> hence put at last) > >> > >> This is what I can think :) Please mentor me on this and suggest any > >> better action plan we can opt for. > >> > >> I am very much excited to work on and implement some really cool things > >> that Java Ecosystem can offer us like Functional Programming, Jigsaw > >> <http://openjdk.java.net/projects/jigsaw/>, or Local Variable Type > >> Inference > >> <https://developer.oracle.com/java/jdk-10-local-variable-type-inference > >, > >> I am not penning dowing > >> > >> > >> Thanks & Regards, > >> -- > >> *Pradhan Yash Sharma* > >> > >> On Sat, Apr 28, 2018 at 6:59 PM, Taher Alkhateeb < > >> slidingfilame...@gmail.com> wrote: > >> > >>> Hello Yash, > >>> > >>> Thank you for your work on this so far. It's great to see people > >>> focusing on refactoring, which I think should probably be the top > >>> priority for all of us. > >>> > >>> I will review the JIRAs some more over the coming days, but I have a > >>> concern that some of the patches are very large. > >>> > >>> We had many discussions in the past about focused refactoring vs. > >>> general trends. Focused refactoring means you go after a specific > >>> piece of code like a class or group of related classes / artifacts and > >>> fixing them. General trends, on the other hand, means that you > >>> identify a certain pattern and then making a sweeping change across > >>> the entire code base. > >>> > >>> General trends refactorings can be very dangerous, because you are > >>> running after a "trend" not isolating a specific piece of code and > >>> fixing it. > >>> > >>> So my recommendation, especially for the bigger patches that you have, > >>> is to redesign / refactor so that it is a topic-based, not > >>> trend-based. We need to make these commits in isolated bite-sized > >>> chunks that focus on a specific area instead of a specific trend (make > >>> public to private, add try-with-resources, or whatever else) > >>> > >>> Cheers, > >>> Taher > >>> > >>> On Thu, Apr 19, 2018 at 3:24 PM, Yash Sharma > >>> <yash.sha...@hotwaxsystems.com> wrote: > >>> > Hi Devs, > >>> > > >>> > Here is the detailed information about the things I am working on for > >>> > performance optimization in our OFBiz code. > >>> > > >>> > *1.) Downsize Accessibility Scope* > >>> > I've tried to downsize accessibility scope of classes, interfaces, > >>> abstract > >>> > class, declared member variables, enumerations, methods, and > >>> constructors > >>> > to as minimum as possible as per OFBIz current implementation, still > >>> there > >>> > is a lot of scope for improvement but it would require changes at the > >>> > granular level. I've used this > >>> > < > https://docs.oracle.com/javase/tutorial/java/javaOO/accesscontrol.html> > >>> as > >>> > my reference point. example: > >>> > > >>> > - public void noteKeyRemoval(UtilCache<K, V> cache, K key, V > >>> oldValue); > >>> > + void noteKeyRemoval(UtilCache<K, V> cache, K key, V oldValue); > >>> > > >>> > Limiting the scope of the method from public modifier to package > level. > >>> > > >>> > *2.) Using Lambda Expressions* > >>> > Then tried to use lambda expressions on simple functional work to > >>> leverage > >>> > implicit type of coding an example: > >>> > > >>> > - Map<String, String> initParameters = new > >>> LinkedHashMap<>(); > >>> > - for (Element e : initParamList) { > >>> > - initParameters.put(e.getAttribute("name"), > >>> > e.getAttribute("value")); > >>> > - } > >>> > + Map<String, String> initParameters = > >>> > initParamList.stream().collect(Collectors.toMap(e -> > >>> > e.getAttribute("name"), e -> e.getAttribute("value"), (a, b) -> b, > >>> > LinkedHashMap::new)); > >>> > > >>> > > >>> > Some of the key benefits of using lambdas will introduce Functional > >>> > style over Imperative style > >>> > <https://stackoverflow.com/questions/2078978/functional-prog > >>> ramming-vs-object-oriented-programming>, > >>> > we can use method referencing > >>> > <https://docs.oracle.com/javase/tutorial/java/javaOO/methodr > >>> eferences.html>, > >>> > usage of aggregate operations, and it will help developers to write > >>> > memory efficient code. > >>> > > >>> > > >>> > *3.) Using Type Inference* > >>> > Java uses type inference so to make code lightweight I've updated > >>> > code constructs as shown in the example for more on this refer this > >>> article > >>> > <https://docs.oracle.com/javase/tutorial/java/generics/genTy > >>> peInference.htmlv> > >>> > . > >>> > > >>> > - Map<String, ? extends Object> systemProps = > >>> > UtilGenerics.<String, Object> checkMap(System.getProperties()); > >>> > + Map<String, ?> systemProps = > >>> > UtilGenerics.checkMap(System.getProperties()); > >>> > > >>> > > >>> > *4.) Use of single quote for character* > >>> > There is a significant usage of <"Single Character"> in the codebase > for > >>> > example: > >>> > > >>> > - throw new GenericConfigException("Error opening file at > >>> > location [" + fileUrl.toExternalForm() + "]", e); > >>> > + throw new GenericConfigException("Error opening file at > >>> > location [" + fileUrl.toExternalForm() + ']', e); > >>> > > >>> > > >>> > "]" is comparativlelly slower then ']' Java internally uses Flyweight > >>> > Design pattern to create String literals so for every call it will > not > >>> > create a new Object and used an existing one this will improve > >>> > performace to some extend an study can be seen on this > >>> > <https://stackoverflow.com/questions/24859500/concatenate- > >>> char-literal-x-vs-single-char-string-literal-x> > >>> > page. > >>> > > >>> > > >>> > *5.) Updated Variable Declaration* > >>> > > >>> > Lastly some of the variable declaration is updated this doesn't > create > >>> > a huge difference but helps JVM at the from implicit conversion. > >>> > > >>> > - private long cumulativeEvents = 0; > >>> > + private long cumulativeEvents = 0L; > >>> > > >>> > > >>> > Based on above findings, I have done some code improvement and > >>> > provided following patches. *And need community help for reviewing > >>> > these changes.* > >>> > Kindly provide any improvents or suggestion you have in mind :) > >>> > > >>> > > >>> > 1. [OFBIZ-10344] Refactoring Variable Scope for > >>> > org.apache.ofbiz.base package > >>> > <https://issues.apache.org/jira/browse/OFBIZ-10344> > >>> > 2. [OFBIZ-10345] Refactoring Variable Scope for > >>> > org.apache.ofbiz.catalina.container > >>> > <https://issues.apache.org/jira/browse/OFBIZ-10345> > >>> > 3. [OFBIZ-10346] Refactoring Variable Scope for > >>> > org.apache.ofbiz.common > >>> > <https://issues.apache.org/jira/browse/OFBIZ-10346> > >>> > 4. [OFBIZ-10347] Refactoring Variable Scope for > >>> > org.apache.ofbiz.datafile > >>> > <https://issues.apache.org/jira/browse/OFBIZ-10347> > >>> > 5. [OFBIZ-10348] Refactoring Variable Scope for > >>> > org.apache.ofbiz.entity > >>> > <https://issues.apache.org/jira/browse/OFBIZ-10348> > >>> > > >>> > > >>> > P.S. Apart from this I am also working on performance matrix and will > >>> share > >>> > it soon. > >>> > > >>> > > >>> > Thanks & Regards, > >>> > > >>> > -- > >>> > *Pradhan Yash Sharma* > >>> > > >>> > > >>> > On Tue, Apr 17, 2018 at 11:28 AM, Yash Sharma < > >>> yash.sha...@hotwaxsystems.com > >>> >> wrote: > >>> > > >>> >> Thank you for the feedback I've created a Jira ticket OFBIZ-10343 > >>> >> <https://issues.apache.org/jira/browse/OFBIZ-10343> and I will add > >>> >> patches for the same for your review. > >>> >> > >>> >> Thanks & Regards, > >>> >> -- > >>> >> *Pradhan Yash Sharma* > >>> >> > >>> >> On Fri, Apr 13, 2018 at 5:50 PM, Taher Alkhateeb < > >>> >> slidingfilame...@gmail.com> wrote: > >>> >> > >>> >>> Hello Pradhan, > >>> >>> > >>> >>> Refactoring is exactly what we need and is a welcomed activity. I > >>> >>> think we should, however, try to avoid "big ideas" across the > entire > >>> >>> code base. The subject of your message is the reason why I say > that. > >>> >>> > >>> >>> So, if you want to start refactoring, I suggest to start with one > >>> >>> piece of code, study it careful, issue a JIRA, and provide a patch. > >>> >>> This should be focused similar to your notes on UtilCache. > >>> >>> > >>> >>> On Fri, Apr 13, 2018 at 12:14 PM, Pradhan Yash Sharma > >>> >>> <pradhanyashsharm...@gmail.com> wrote: > >>> >>> > Hello, > >>> >>> > > >>> >>> > While I was working on UtilCache.java file came across some > >>> >>> improvements, > >>> >>> > they are as follows: > >>> >>> > > >>> >>> > 1) Method and Variable access modifiers can be narrowed down to > >>> private > >>> >>> > access modifier. > >>> >>> > > >>> >>> > 2) Then AtomicLong can be given value 0L instead of 0. > >>> >>> > > >>> >>> > 3) Some Variables is used in both synchronized and unsynchronized > >>> >>> blocks, > >>> >>> > so they can be declared final. eg, > >>> >>> > > >>> >>> > > >>> >>> > > >>> >>> > *protected AtomicLong hitCount = new AtomicLong(0); > >>> >>> private > >>> >>> > final AtomicLong hitCount = new AtomicLong(0L);* > >>> >>> > One variable was able to get most of my attention is > >>> >>> > > >>> >>> > * protected ConcurrentMap<Object, CacheLine<V>> > >>> >>> memoryTable > >>> >>> > = null;* > >>> >>> > > >>> >>> > This is used in synchronized and unsynchronized blocks, this > Object > >>> can > >>> >>> be > >>> >>> > converted into ThreadLocal or AtomicReference but it would > require > >>> >>> changes > >>> >>> > in the current implementation as well. > >>> >>> > > >>> >>> > Lastly, there is extensive use of for loops for iteration we can > use > >>> >>> Java 8 > >>> >>> > Streams, Collector, and other functions to leverage implicit > looping > >>> >>> > mechanism. > >>> >>> > > >>> >>> > > >>> >>> > -- > >>> >>> > Pradhan Yash Sharma > >>> >>> > >>> >> > >>> >> > >>> > >> > >> >
