> On May 2, 2013, 12:19 a.m., Jarek Cecho wrote: > > Hi Vasanth, > > thank you very much for working on this! > > > > I was thinking about the cloneForms() methods and it's usage of > > "instanceof" operator. What about creating a clone method on each model > > object and then just call that method instead of quite long if-elif*-else > > statement? I think that this way we would be able to overcome the need for > > instanceof. We can make the method parametric to see if user also wants to > > copy over the values or not. > > > > Jarcec > > vasanthkumar wrote: > Hi Jarcec, > In cloneForms method, if-elif*-else statement used only for identifying > MInput type. Even if you provide clone method on each model object while > copying its Forms which contains multiple MInput type. So requires > "instanceof" to identify the MInput type for creating new instance and > copying values. > > For example: while copying/cloning MConnection, contains two > MConnectionForms then MConnectionForms contain MForm list. List may have > multiple forms with difference type. Here we have to use "instanceof" for > identifying the MInput type. > > Either making Method parametric with constructor (Eg: > MConnection(MConnection, boolean copyValue)) or with clone method > clone(boolean copyValue) return model object ? > > Kindly suggest. > > Thanks, > Vasanth kumar
Hi Vasanth, I would put the clone(boolean value) method to all model objects including MInput<?>. If we will require implementing the method clone(boolen) in abstract MInput, then each concrete implementation will have to create proper cloning method and I believe that in such case we won't need the if-elif*-else statement at all. I'm not a big fan of "copy constructors", so I would personally prefer to expose the cloning interface only using the "clone" method. Jarcec - Jarek ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/10886/#review20038 ----------------------------------------------------------- On May 1, 2013, 9:01 p.m., vasanthkumar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/10886/ > ----------------------------------------------------------- > > (Updated May 1, 2013, 9:01 p.m.) > > > Review request for Sqoop. > > > Description > ------- > > Cloning class by copy constructor > > > This addresses bug SQOOP-995. > https://issues.apache.org/jira/browse/SQOOP-995 > > > Diffs > ----- > > common/src/main/java/org/apache/sqoop/model/FormUtils.java 80b10c8 > common/src/main/java/org/apache/sqoop/model/MConnection.java c31eafd > common/src/main/java/org/apache/sqoop/model/MConnector.java 9207c62 > common/src/main/java/org/apache/sqoop/model/MFramework.java f5252ef > common/src/main/java/org/apache/sqoop/model/MJob.java 5b50bfd > common/src/test/java/org/apache/sqoop/model/TestMConnection.java c3469ff > common/src/test/java/org/apache/sqoop/model/TestMConnector.java 716b124 > common/src/test/java/org/apache/sqoop/model/TestMFramework.java a5366ca > common/src/test/java/org/apache/sqoop/model/TestMJob.java dbc791e > > Diff: https://reviews.apache.org/r/10886/diff/ > > > Testing > ------- > > Done > > > Thanks, > > vasanthkumar > >
