> Leonardo said that the viewid algorithm isn't relying on the hash > code, at which point you changed the topic to inner classes.
The inner class discussion is unrelated. It's just that all the code was hidden inside a single class. I refactored out 8 of them and dropped 5 others. The ones I dropped have been 'specialized' (but effectively unused) versions for the same key. It seems I even missed one: SerializedViewCollection. Let's please move all that stuff into an own package. Imo it's much more readable to have an own package with a few smaller classes than having an undocumented 2000 LOC class with 14 inner classes. And please let's document it. I tried to add a few lines of comment for each of the caching strategies. But I'm not even sure if we need all of them. The point where the discussion changed is when Leo argued that the viewId hash Code is enough because the viewId is not needed at all. But then we should drop it from the key completely if we don't need it anyway. And if we need the viewId then we should treat it properly. > I agree with Leonardo that any time we can trim a few bytes off the > state, we need to try to do it. Fully agree if we don't need it. But either trim it completely or implement it properly if needed. See above. I really appreciate Leos effort to try to get rid of state. But each code can benefit from a review. > Not every site cares about > encryption. Or perhaps they are providing a custom encryption.Yes but > decode/encode is part of that logic for some of the key generator. All that > was done in a privat inner class. See SecureRandomKeyFactory For those we might not need the viewId if the entropy is large enough. But it needs a check. We might add the VOTE for the ProjectStage question I brought up earlier this week: Imo we should not switch so fundamental features between Development and Production. That code worked fine in Development and did blew up in Production. That is purely unexpected! Please let's only use ProjectStage.Production checks for enabling caches at all if we like. LieGrue, strub ----- Original Message ----- > From: Mike Kienenberger <[email protected]> > To: MyFaces Development <[email protected]>; Mark Struberg > <[email protected]> > Cc: > Sent: Thursday, November 15, 2012 12:35 AM > Subject: Re: [VOTE] technical and design code issues > >> 1.) How many inner classes do we like to use? > > Well, your first item isn't a technical or design issue. It's a style > issue. > > My preference is that the code be readable, and if these inner classes > are making it less readable, then they should be moved. > > So as a general, but not absolute rule, I haven't looked at the code > in question, so I am not saying you're right about it. > > [X] Inner classes [should] only be used for private data holders and > similar helper classes. Max 2 in a class, not longer than 50 LOC each. > > >> 2.) hashCode as key > > Obviously, we don't need votes on whether we want to write broken > code. You have a specific issue in mind, and this is what we should > be considering. > > I haven't closely followed the discussion, and I only have a few > minutes to spare tonight. However.... > > Leonardo said that the viewid algorithm isn't relying on the hash > code, at which point you changed the topic to inner classes. > > If it is relying on hashcode, and hashcode is not something we control > -- viewids are Strings, correct? -- then we should not assume that > hashcode is good enough. Hashocode + equals needs to be used. > > I agree with Leonardo that any time we can trim a few bytes off the > state, we need to try to do it. Not every site cares about > encryption. Or perhaps they are providing a custom encryption. > > > > On Wed, Nov 14, 2012 at 5:34 PM, Mark Struberg <[email protected]> wrote: >> Hi! >> >> While working on a few parts of the MyFaces codebase the last few times I > saw increasingly bad style imo. Some of it might be a personal taste, others > is > technically funded. Let's start with the design parts >> >> >> >> 1.) How many inner classes do we like to use? >> I found 10++ inner classes per class. For no whatever reason - just to > make the code unmaintainable it seems. One could argue with private access, > but > hey that's bollocks as everyone can tweak private classes via reflections > anyway. It' just stinks and is not maintainable that way. Also this leads to > bad design as it's not easy to see what classes we have in there. In > ServerSideStateCacheImpl there has been 10++ inner classes with 1000 LOC > which > might have EASILY filled an own package! >> >> [ ] Inner classes must only be used for private data holders and similar > helper classes. Max 2 in a class, not longer than 50 LOC each. >> >> [ ] I don't care >> [ ] Oh yes, inner classes for the win, even if a class hits 5000 LOC > because of that! >> >> >> 2.) hashCode as key >> a hashCode is NOT unique! Using it for a key which must be unique is just > dumb. >> >> [ ] Use hashCode + original value for keys. First checking the hashCode and > if it is the same equals on the original value >> [ ] I don't care >> [ ] Yea, pleas gimme the full random key collission stuff. I like to get > random errors in production which are not reproducible - makes my days so > sparkling! >> >> >> LieGrue, >> strub >> >> Oh, here is my vote: >> [X ] Inner classes must only be used for private data holders and similar > helper classes. Max 2 in a class, not longer than 50 LOC each. >> [X ] Use hashCode + original value for keys. First checking the hashCode > and if it is the same equals on the original value >
