Hi Peter, On 10/27/2014 03:45 PM, Peter Levart wrote: > I merged the functionality of the method-signature-key with the > linked-list node, holding just a reference to Method object and a link > to 'next' node. I think this is the most compact temporary datastructure > representation for that purpose that leverages standard LinkedHashMap.
I see. I wonder when somebody come with a stress test using multiple interfaces with the same signature method :) Seems very unlikely. >> * Formatting and logic in MethodList.consolidateWith gives me chills. I >> think at very least introducing variables for m.getDeclaringClass() and >> ml.m.getDeclaringClass() improve readability. Also, moving the comments >> at the end of the line should improve readability and better highlight >> the boolean expression you are spelling out. Below is the >> straight-forward rewrite: >> ... > > Excellent. I'll take your formatting if you don't mind ;-) Sure. Check if I had captured it right first. I think we need to be more strict with parentheses to avoid precedence overlooks: (A || B && C) should better be ((A || B) && C) or (A || (B && C)) >> * Should we use just methods.put() here? >> 2851 MethodList ml0 = methods.putIfAbsent(ml, ml); > > It really doesn't matter as the exception is thrown if (ml0 != null) and > LinkedHashMap is forgotten... > > if (ml0 == null), put and putIfAbsent are equivalent. I used putIfAbsent > to make the 3 loops (declared methods, superclass methods, > (super)interface methods) more uniform. Okay. >> >> * I wonder if the code would be simpler if we push the Map<MethodList, >> MethodList> maintenance to some internal utility class? That would >> probably simplify the loops in privateGetPublicMethods? > > I thought MethodList.add()/consolidateWith() were close to those utility > methods. I could extract the logic in each of 3 loops to 3 void methods > taking 'methods' Map and a Method instance, but I don't want to pollute > the method namespace in j.l.Class any more than necessary and making a > special inner class for that is an overkill. Previously the holder for > such methods was MethodArray, but it's gone now. I could subclass > LinkedHashMap if I wanted to avoid an unnecessary indirection, but > that's not a good practice. It's not that bad as it is - 38 lines of > code for 3 loops with comments and spacing. I could improve comments > though... Yes. I had to read the entire thing a couple of times to understand how this works. Containing most of the logic into MethodList seems like a way to improve this. Not sure though. Thanks, -Aleksey.
