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 <[email protected]> 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 <[email protected] >> 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 < >> [email protected]> 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 >>> <[email protected]> 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 < >>> [email protected] >>> >> 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 < >>> >> [email protected]> 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 >>> >>> <[email protected]> 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 >>> >>> >>> >> >>> >> >>> >> >>
