Hello Yash, Inline ... On Wed, May 2, 2018, 8:59 AM Yash Sharma <yash.sha...@hotwaxsystems.com> wrote:
> 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 Actually I think it is still called refactoring because you are changing code, not system behavior. [1] (it will not be an > OFBiz Update with new tricks it will be an OFBiz Upgrade with workflow and > may involve some new tricks). > It will be the same system with the same behavior but with some improved code > 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. > Exactly! it will be challenging, it _should_ be challenging. Coding is not something to be taken lightly especially with a massive interconnected system like OFBiz. > Looking forward to your approval. It would be a great help if you could > propose an action plan. My approval is irrelevant, we are a community and we value consensus and teamwork and we all play on an equal level here :) I am merely expressing thoughts that might be helpful. So what should you do? May I recommend a simple approach? For example, instead of making a patch for a 100 different classes, make a patch for only one or few related classes. The smaller the commit size the better because it will allow proper peer-review. It will also increase the speed of getting your work committed. Your enthusiasm and dedication is great, I look forward to working with you in the coming days. > Thanks & Regards, > -- > *Pradhan Yash Sharma* > [1] https://en.m.wikipedia.org/wiki/Code_refactoring > > 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 > > >>> >>> > > >>> >> > > >>> >> > > >>> > > >> > > >> > > >
