apex-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gaurav Gupta <gau...@datatorrent.com>
Subject Re: Thoughts on refactoring large functions
Date Thu, 05 Nov 2015 05:03:52 GMT
I agree that having big monolithic functions is not good but I think we
should have some guidelines for this o/w it becomes very subjective what is
readable and what is not etc..

Thanks
-Gaurav

On Wed, Nov 4, 2015 at 12:09 PM, Vlad Rozov <v.rozov@datatorrent.com> wrote:

> +1 even JVM hotspot optimizer does not like long functions. Smaller
> functions are more likely to be compiled and lead to performance gains.
>
> It may be a good initial exercise for new contributors as well assuming
> that it will require proper unit test code coverage.
>
>
>
> Thank you,
> Vlad
>
> Отправлено с iPhone
>
> > On Nov 4, 2015, at 11:36, Sandesh Hegde <sandesh@datatorrent.com> wrote:
> >
> > -1 Going forward we can have the restriction on the function size. If the
> > reason for refactoring is 'hard to read', it is not a strong enough
> reason
> > to refactor a core piece.
> >    If there is some issue in that part of the code and it is hard to
> debug
> > then it is fine.
> >
> > On Wed, Nov 4, 2015 at 11:16 AM Pramod Immaneni <pramod@datatorrent.com>
> > wrote:
> >
> >> +1 as long as it doesn't come at the expense of performance where it is
> >> critical like data flow.
> >>
> >> Thanks
> >>
> >> On Wed, Nov 4, 2015 at 10:42 AM, Timothy Farkas <tim@datatorrent.com>
> >> wrote:
> >>
> >>> +1
> >>>
> >>> On Wed, Nov 4, 2015 at 10:38 AM, Chandni Singh <
> chandni@datatorrent.com>
> >>> wrote:
> >>>
> >>>> +1 for avoiding large monolithic functions and I think if the effort
> is
> >>>> already done to break an existing hard-to-understand function then we
> >>>> should merge that change (provided it passes all the checks).
> >>>>
> >>>> I think writing functions that are not monolithic cannot be enforced
> by
> >>> any
> >>>> tool. We can create a document that lays down certain guide lines but
> >>> other
> >>>> than that this needs to be part of code review process.
> >>>>
> >>>> Thanks,
> >>>> Chandni
> >>>>
> >>>>
> >>>> On Wed, Nov 4, 2015 at 10:16 AM, Ganelin, Ilya <
> >>>> Ilya.Ganelin@capitalone.com>
> >>>> wrote:
> >>>>
> >>>>> Hello all – in doing some refactoring of the code base, I’ve
noticed
> >>> that
> >>>>> there are several large functions (several screens in length) with
> >>>> numerous
> >>>>> nested calls (as many as five deep). While there are no explicit
> >>>> guidelines
> >>>>> in the code style guide or in other common guidelines, it seems
to be
> >>>> good
> >>>>> programming practice to avoid large monolithic functions since these
> >>>> become
> >>>>> harder to analyze and maintain. To that end, I submitted a PR to
> >>> refactor
> >>>>> one such function but this triggered discussion as to whether it’s
> >>>>> warranted to do this in the first place. Would love to get people’s
> >>>>> thoughts as to whether this is a good idea and if so, how could
we
> >>> update
> >>>>> the style guide to reflect this requirement?
> >>>>>
> >>>>> The PR is below:
> >>
> https://github.com/apache/incubator-apex-core/pull/129#issuecomment-153807144
> >>>>> ________________________________________________________
> >>>>>
> >>>>> The information contained in this e-mail is confidential and/or
> >>>>> proprietary to Capital One and/or its affiliates and may only be
used
> >>>>> solely in performance of work or services for Capital One. The
> >>>> information
> >>>>> transmitted herewith is intended only for use by the individual
or
> >>> entity
> >>>>> to which it is addressed. If the reader of this message is not the
> >>>> intended
> >>>>> recipient, you are hereby notified that any review, retransmission,
> >>>>> dissemination, distribution, copying or other use of, or taking
of
> >> any
> >>>>> action in reliance upon this information is strictly prohibited.
If
> >> you
> >>>>> have received this communication in error, please contact the sender
> >>> and
> >>>>> delete the material from your computer.
> >>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message