myfaces-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark Struberg <strub...@yahoo.de>
Subject Re: [DISCUSS] how to get rid of tons of duplicated code
Date Sun, 23 Oct 2011 22:38:17 GMT
I've now read through the old mail archives and understand what the original problem was. But
actually I don't think we solved it correctly right now. Of course we solved to original problem,
but opened a can of worms causing other problems.

The problem as far as I remember has been that myfaces-shared had tons of duplicated code
in it. One for core, one for tomahawk, one for trinidad, etc. 


The shared part for core got moved to myfaces-core, but the deeper problem was that it was
not easily possible to have multiple different versions of myfaces-shared. This now got solved
by using the maven-shade-plugin. So we should rethink the practice to duplicate all the code
and aim for a _clean_ solution.

Also (being a maven guy) I cannot quite follow the argument about the release cycles. Running
a myfaces-shared release and then (with the same staging repo) a myfaces-core release is a
task of 15 minutes. + the time for running the TCK, but this gets run via CI anyway, right?
Thus this is barely a problem.
If it is then I'd happily volunteer to do the next release (do this for a few projects already)
As you know, performing a release really got _much_ easier nowadays with our new apache-parent
pom.
But maybe this argument was only meant for our old release process (which I agree was a lot
of work)?

If your answer is 'it's still needed' then can we just unify all other usages?

One question which bothers me with the 'shared' approach if what would happen to our build-tools
annotation scanning (@JSFWebConfigParam, etc)? Does this already work with dependencies? Do
we have this problem already due to the fact that we import such annotated classes via dependency?

> Additionally, we increase the risk of "side effects",
> because a change done in core could introduce a bug in other parts.
Imo it's exactly the opposite. If you use the same code in 7 projects, then it is more likely
that a bug gets found and fixed. 
And the opposite case is (sadly) absolutely unlikely. If you have a class duplicated 7 times
and find a bug in one project, it is highly unlikely that all 6 other projects will get this
fix applied :(

LieGrue,
strub



----- Original Message -----
> From: Leonardo Uribe <lu4242@gmail.com>
> To: MyFaces Development <dev@myfaces.apache.org>
> Cc: Mark Struberg <struberg@yahoo.de>
> Sent: Sunday, October 23, 2011 9:08 PM
> Subject: Re: [DISCUSS] how to get rid of tons of duplicated code
> 
> Hi
> 
> Ok, let's check the proposal
> 
> MS>> So the suggestion is:
> MS>>
> MS>> 1.) cleanup myfaces-shared. mf-shared has almost no checkstyle
> rules applied.
> 
> Yes, sounds good.
> 
> MS>> 2.) add unit tests for myfaces-shared. Currently there are not 
> many...
> 
> Ok, sounds good too.
> 
> MS>> 3.) move the shared code parts back to myfaces-shared and add unit 
> tests.
> 
> So, this means do one step back and move the code from myfaces-core
> "shared" to myfaces-shared project? This breaks effectively the
> changes done some months ago to make easier work with myfaces core
> itself.
> 
> In that time the conclusion was: "core has priority over anything
> else, so shared code must live in core, but myfaces-shared project
> should just copy the code from there and have its own lifecycle"
> (these are my own words as I understood).
> 
> So this point does not have practical sense, and go against everything
> discussed earlier.
> 
> MS>> 4.) import myfaces-shared via maven dependency and use
> <minimizeJar> and <relocations> to package the stuff
> 
> maven-shade-plugin is a good "tool" but doesn't fit well in this
> scenario. The reason is we need an alternate release lifecycle for the
> shared code between myfaces core and other projects.
> 
> Historically that was the very first intention behind myfaces-shared
> project. Any myfaces core release requires some additional steps to do
> (TCK), so that becomes a problem when you try to release other
> libraries that depends of shared. So, to fix that, "shared" was
> created, so the code can be released in a independent way, and prevent
> myfaces core becomes an obstacle to release any other project
> (tomahawk, portlet-bridge, ... ). So, to release tomahawk you release
> shared first and then tomahawk.
> 
> maven-shade-plugin requires a released artifact to do its job. So, use
> it impose that restriction. In "shared" case, preserve the original
> intention becomes "imperative", and that's the reason why a goal 
> was
> created to copy the code from myfaces-core shared, so the release
> manager can run this goal, commit the changes and then run a release.
> 
> My proposal in this case is do the same we did for shared, but for
> "myfaces commons" case. Then we can use maven-shade-plugin in other
> projects, but not over shared, instead over a released version of
> myfaces-commons-utils. Keep tomahawk or portlet-bridge as is, using
> shared project, because by its nature, those projects require classes
> that are not meant to be used outside those cases.
> 
> Note do any hack in this part makes a little bit "obscure" how to make
> changes, because everything becomes "centralized", but makes easier
> maintain code. Additionally, we increase the risk of "side effects",
> because a change done in core could introduce a bug in other parts. So
> at the end this is a matter of how to keep our code "balanced", even
> if some times it becomes a decision about "choose the less
> inconvenient alternative".
> 
> regards,
> 
> Leonardo Uribe
> 
> 2011/10/23 Leonardo Uribe <lu4242@gmail.com>:
>>  Hi
>> 
>>  2011/10/23 Jakob Korherr <jakob.korherr@gmail.com>:
>>>  Hi Mark,
>>> 
>>>  +1 - that's exactly what I have been trying to accomplish some time
>>>  ago (introducing common-shades [1]). Unfortunately, I was not
>>>  successful back then.
>>> 
>> 
>>  It is clear we need to "split" myfaces-impl into multiple 
> modules. There
>>  are some parts that are useful for other projects. The code you did
>>  on commons-shade was the attempt to solve the problem of the
>>  duplicate code used on myfaces-test.
>> 
>>  Now the objective is find a way about how to reuse code in myfaces
>>  core between multiple projects effectively.
>> 
>>>  However, there is a slight problem with moving all this stuff into
>>>  MyFaces shared, which I want to point out: code size. If we really put
>>>  all the code that is shared across any MyFaces subproject into shared,
>>>  it will get fat and ugly (even more than it is right now). In
>>>  addition, if we continue including the whole shared project into
>>>  MyFaces core, MyFaces core impl will get bigger and bigger.
>>> 
>> 
>>  Yes, the problem basically is MyFaces shared does not have any order
>>  or any notion of API. There are code that is used only in tomahawk but not
>>  intended to use in any other place. There are some useful utitlities but
>>  sometimes without documentation, and there are some other code that is
>>  just obsolete. It it clear a cleanup of that location is
>>  necessary, but note priorities comes first, so this task has been delayed 
> in
>>  order to deal with other important stuff. Now it is a good time to fix 
> this.
>> 
>>>  Thus I'd like to suggest something similar which I wanted to
>>>  accomplish with common-shades: Introduce a new shared module, which
>>>  consists of many submodules that each handle a specific functionality
>>>  instead of being one fat module. With this approach each MyFaces
>>>  subproject would be able to pick out only the stuff it really needs.
>>>  Furthermore we would see more easily which code in shared is not used
>>>  anymore (I guess at the moment there is a lot of it), just by checking
>>>  which modules are still used in our poms.
>>> 
>> 
>>  That is the big question, how to split myfaces-impl and shared. Precisely
>>  the intention of myfaces-commons-utils projects was take the stuff that is
>>  useful from shared and build an usable API for developers outside MyFaces.
>> 
>>  For example, MyFaces HTML5 subproject was a good experiment to see
>>  which code is useful and should be added in a API. Some weeks ago I checked
>>  and removed all duplicate code to use myfaces-commons-utils. So the 1.0.2
>>  release contains those classes taken from shared.
>> 
>>  regards,
>> 
>>  Leonardo Uribe
>> 
>>>  Regards,
>>>  Jakob
>>> 
>>>  [1] https://svn.apache.org/repos/asf/myfaces/common-shades/
>>> 
>>>  2011/10/23 Mark Struberg <struberg@yahoo.de>:
>>>>  Hi!
>>>>  While working on the mafyces-commons cleanup I figured that we have 
> tons of
>>>>  duplicated code spread over MyFaces.
>>>> 
>>>> 
>>>>  As an example I like to mention myfaces-commons-resourcehandler. 
> There are
>>>>  43 classes in total, and 35 of them are just 1:1 copied from other 
> projects
>>>>  to provide resource management, zip, etc. For me this is an 
> absolute no-go.
>>>>  Those classes have neither tests nor any documentation where they 
> got forked
>>>>  from. Nor will any bug which gets fixed in another module make 
> it's way over
>>>>  to all the other projects containing that very forked code. 
> That's just ...
>>>>  unbelievable unmaintainable.
>>>> 
>>>>  There are 2 different ways to solve this (depending on the 
> problem):
>>>> 
>>>>  A.) drop the functionality and provide a generalized solution. The 
> GZIP of
>>>>  myfaces-commons-resourcehandleris an obvious example:
>>>>  We now copy this code over the 4th time or even more. Instead of 
> doing this,
>>>>  we should rather do it in the classic unix fashion: do one thing, 
> but do it
>>>>  well.
>>>>  Which means I'd rather see all the GZIP stuff factored out into 
> an own
>>>>  mf-commons module as a Servlet Filter. This can then get applied to 
> what
>>>>  ever other mechanism you like. This could also (commonly) cover 
> cases like
>>>>  detecting http UserAgents which are not able to handle zipped 
> resources,
>>>>  etc. That way we could provide this logic ONCE and have complete 
> freedom
>>>>  over the configuration.
>>>> 
>>>>  B.) code reusable components once and use them in other projects 
> (ev via
>>>>  shading it in).
>>>>  ClassLoaderResourceLoader would be a perfect candidate! I grepped 
> through
>>>>  only the few pits which I have checked out locally and found this 
> class 7
>>>>  SEVEN times! I just can't believe that we can't move this 
> stuff to a shared
>>>>  modul...
>>>> 
>>>>  Same for FacesServletMapping. 6 times copied around,
>>>>  WebConfigProviderFactory 5 times, ...
>>>>  There are whole packages with 10++ classes which got copied 1:1!
>>>> 
>>>>  I really could cry seeing this :(
>>>> 
>>>> 
>>>>  What can we do to solve this?
>>>> 
>>>>  Theoretically myfaces-shared should contain this stuff. This is 
> exactly what
>>>>  it is for!
>>>>  Historically there have been some hand forged tweeks and ugly 
> hacks, but
>>>>  nowadays we have the maven-shade-plugin to make our live easier.
>>>> 
>>>>  So the suggestion is:
>>>> 
>>>>  1.) cleanup myfaces-shared. mf-shared has almost no checkstyle 
> rules
>>>>  applied.
>>>>  2.) add unit tests for myfaces-shared. Currently there are not 
> many...
>>>>  3.) move the shared code parts back to myfaces-shared and add unit 
> tests.
>>>>  4.) import myfaces-shared via maven dependency and use 
> <minimizeJar> and
>>>>  <relocations> to package the stuff
>>>> 
>>>>  [+1] fine go ahead (ideally: yes, what parts can I help with?)
>>>>  [0] dont care
>>>>  [-1] wont work because ...
>>>> 
>>>> 
>>>>  I've attached a file which contains all Classes which name 
> exists multiple
>>>>  times in MyFaces. The number is the cound how often they exist in 
> MyFaces. I
>>>>  excluded current20.
>>>>  Please note that classes with the same name do not necessarily have 
> the same
>>>>  content - but quite a lot actually do have! (scroll to the bottom 
> of the
>>>>  file ...)
>>>> 
>>>>  LieGrue,
>>>>  strub
>>>> 
>>> 
>>> 
>>> 
>>>  --
>>>  Jakob Korherr
>>> 
>>>  blog: http://www.jakobk.com
>>>  twitter: http://twitter.com/jakobkorherr
>>>  work: http://www.irian.at
>>> 
>> 
>

Mime
View raw message