+1 on general sentiment of splitting big functions into smaller ones. It makes it easier to mock functionality, if needed, for unit testing.
However, I still think it should be based on case by case basis. I can think of one case where splitting may not add much advantage: If the smaller methods can only be called in certain order to make sense. Like if we split a giant method in functions like: firstMethodToInvoke(), secondMethod() and so on. It probably does not improve readability much. (But then this could also be a candidate where we need mocking of only secondMethod() for testing functionality :). Thanks, Isha On Wed, Nov 4, 2015 at 9:53 PM, Priyanka Gugale <[email protected]> wrote: > +1. > Having small functions is important for following reasons: > 1. Readability: Code cleanliness as well as to avoid code repeatation. > 2. Unit testing: We can write unit tests for small logical units rather > than writing it for big functions which might have many test paths > (multiple conditions etc) > 3. Extensiblility. If we write small function for each logical code unit, > one can overwrite that small piece to override behavior rather than > overriding entire big function. > > -Priyanka > > On Thu, Nov 5, 2015 at 10:33 AM, Gaurav Gupta <[email protected]> > wrote: > > > 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 <[email protected]> > > 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 <[email protected]> > > 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 < > > [email protected]> > > > > 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 < > [email protected]> > > > >> wrote: > > > >> > > > >>> +1 > > > >>> > > > >>> On Wed, Nov 4, 2015 at 10:38 AM, Chandni Singh < > > > [email protected]> > > > >>> 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 < > > > >>>> [email protected]> > > > >>>> 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. > > > >> > > > > > >
