[jira] [Commented] (NIFI-13989) Refactor JsonTreeRowRecordReader constructor to take a parameter object
[ https://issues.apache.org/jira/browse/NIFI-13989?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17897581#comment-17897581 ] ASF subversion and git services commented on NIFI-13989: Commit f15b7caaa483588869553cc7f84f98dfcbbd475a in nifi's branch refs/heads/main from dan-s1 [ https://gitbox.apache.org/repos/asf?p=nifi.git;h=f15b7caaa4 ] NIFI-13989 Reduced constructors in JsonTreeRowRecordReader to one (#9506) Signed-off-by: David Handermann > Refactor JsonTreeRowRecordReader constructor to take a parameter object > --- > > Key: NIFI-13989 > URL: https://issues.apache.org/jira/browse/NIFI-13989 > Project: Apache NiFi > Issue Type: Improvement >Reporter: Daniel Stieglitz >Assignee: Daniel Stieglitz >Priority: Minor > Time Spent: 1h 10m > Remaining Estimate: 0h > > When attempting a fix for NIFI-13963 I found it very hard to add another > argument as there are already 13 arguments for the current constructor. This > is a code smell Martin Fowler identifies as a long parameter list > [https://luzkan.github.io/smells/long-parameter-list.|https://luzkan.github.io/smells/long-parameter-list] > One of the solutions is to create a parameter object to encompass all the > arguments. -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Commented] (NIFI-13989) Refactor JsonTreeRowRecordReader constructor to take a parameter object
[ https://issues.apache.org/jira/browse/NIFI-13989?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17896761#comment-17896761 ] Daniel Stieglitz commented on NIFI-13989: - (y) > Refactor JsonTreeRowRecordReader constructor to take a parameter object > --- > > Key: NIFI-13989 > URL: https://issues.apache.org/jira/browse/NIFI-13989 > Project: Apache NiFi > Issue Type: Improvement >Reporter: Daniel Stieglitz >Assignee: Daniel Stieglitz >Priority: Minor > Time Spent: 10m > Remaining Estimate: 0h > > When attempting a fix for NIFI-13963 I found it very hard to add another > argument as there are already 13 arguments for the current constructor. This > is a code smell Martin Fowler identifies as a long parameter list > [https://luzkan.github.io/smells/long-parameter-list.|https://luzkan.github.io/smells/long-parameter-list] > One of the solutions is to create a parameter object to encompass all the > arguments. -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Commented] (NIFI-13989) Refactor JsonTreeRowRecordReader constructor to take a parameter object
[ https://issues.apache.org/jira/browse/NIFI-13989?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17896728#comment-17896728 ] David Handermann commented on NIFI-13989: - I still lean against introducing a parameter object for its own sake in this scenario, as it seems that reducing the number of constructors would address most of the maintenance concerns. > Refactor JsonTreeRowRecordReader constructor to take a parameter object > --- > > Key: NIFI-13989 > URL: https://issues.apache.org/jira/browse/NIFI-13989 > Project: Apache NiFi > Issue Type: Improvement >Reporter: Daniel Stieglitz >Assignee: Daniel Stieglitz >Priority: Minor > > When attempting a fix for NIFI-13963 I found it very hard to add another > argument as there are already 13 arguments for the current constructor. This > is a code smell Martin Fowler identifies as a long parameter list > [https://luzkan.github.io/smells/long-parameter-list.|https://luzkan.github.io/smells/long-parameter-list] > One of the solutions is to create a parameter object to encompass all the > arguments. -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Commented] (NIFI-13989) Refactor JsonTreeRowRecordReader constructor to take a parameter object
[ https://issues.apache.org/jira/browse/NIFI-13989?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17896727#comment-17896727 ] Daniel Stieglitz commented on NIFI-13989: - That is what I was planning to do once I had the parameter object I could then reduce the number of constructors down to one. > Refactor JsonTreeRowRecordReader constructor to take a parameter object > --- > > Key: NIFI-13989 > URL: https://issues.apache.org/jira/browse/NIFI-13989 > Project: Apache NiFi > Issue Type: Improvement >Reporter: Daniel Stieglitz >Assignee: Daniel Stieglitz >Priority: Minor > > When attempting a fix for NIFI-13963 I found it very hard to add another > argument as there are already 13 arguments for the current constructor. This > is a code smell Martin Fowler identifies as a long parameter list > [https://luzkan.github.io/smells/long-parameter-list.|https://luzkan.github.io/smells/long-parameter-list] > One of the solutions is to create a parameter object to encompass all the > arguments. -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Commented] (NIFI-13989) Refactor JsonTreeRowRecordReader constructor to take a parameter object
[ https://issues.apache.org/jira/browse/NIFI-13989?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17896725#comment-17896725 ] David Handermann commented on NIFI-13989: - As a first pass, I would consider reducing the number of constructors to 1, since the others appear to be basically for convenience in testing. Having a shared test method would obviate the need for multiple constructors, making it easier to maintain across the board. > Refactor JsonTreeRowRecordReader constructor to take a parameter object > --- > > Key: NIFI-13989 > URL: https://issues.apache.org/jira/browse/NIFI-13989 > Project: Apache NiFi > Issue Type: Improvement >Reporter: Daniel Stieglitz >Assignee: Daniel Stieglitz >Priority: Minor > > When attempting a fix for NIFI-13963 I found it very hard to add another > argument as there are already 13 arguments for the current constructor. This > is a code smell Martin Fowler identifies as a long parameter list > [https://luzkan.github.io/smells/long-parameter-list.|https://luzkan.github.io/smells/long-parameter-list] > One of the solutions is to create a parameter object to encompass all the > arguments. -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Commented] (NIFI-13989) Refactor JsonTreeRowRecordReader constructor to take a parameter object
[ https://issues.apache.org/jira/browse/NIFI-13989?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17896721#comment-17896721 ] Daniel Stieglitz commented on NIFI-13989: - YamlTreeRowRecordReader is also affected with this as it inherits from JsonTreeRowRecordReader. In addition this refactoring would help if we ever considered an implementation for TOML NIFI-13919 which I believe would also inherit from JsonTreeRowRecordReader. > Refactor JsonTreeRowRecordReader constructor to take a parameter object > --- > > Key: NIFI-13989 > URL: https://issues.apache.org/jira/browse/NIFI-13989 > Project: Apache NiFi > Issue Type: Improvement >Reporter: Daniel Stieglitz >Assignee: Daniel Stieglitz >Priority: Minor > > When attempting a fix for NIFI-13963 I found it very hard to add another > argument as there are already 13 arguments for the current constructor. This > is a code smell Martin Fowler identifies as a long parameter list > [https://luzkan.github.io/smells/long-parameter-list.|https://luzkan.github.io/smells/long-parameter-list] > One of the solutions is to create a parameter object to encompass all the > arguments. -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Commented] (NIFI-13989) Refactor JsonTreeRowRecordReader constructor to take a parameter object
[ https://issues.apache.org/jira/browse/NIFI-13989?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17896720#comment-17896720 ] David Handermann commented on NIFI-13989: - Thanks for the reply [~dstiegli1]. It is worth noting that several of the overloaded constructors have usage limited to test classes, so there may be some value in refactoring test references to streamline the number of constructors. However, although the number of constructor arguments has grown over time, this is not a public-facing class. As such, I would not expect it to change very often. > Refactor JsonTreeRowRecordReader constructor to take a parameter object > --- > > Key: NIFI-13989 > URL: https://issues.apache.org/jira/browse/NIFI-13989 > Project: Apache NiFi > Issue Type: Improvement >Reporter: Daniel Stieglitz >Assignee: Daniel Stieglitz >Priority: Minor > > When attempting a fix for NIFI-13963 I found it very hard to add another > argument as there are already 13 arguments for the current constructor. This > is a code smell Martin Fowler identifies as a long parameter list > [https://luzkan.github.io/smells/long-parameter-list.|https://luzkan.github.io/smells/long-parameter-list] > One of the solutions is to create a parameter object to encompass all the > arguments. -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Commented] (NIFI-13989) Refactor JsonTreeRowRecordReader constructor to take a parameter object
[ https://issues.apache.org/jira/browse/NIFI-13989?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17896719#comment-17896719 ] Daniel Stieglitz commented on NIFI-13989: - I am fine with that. It just seems very hard to add parameters without having to juggle a bunch of different constructors. It would be simpler with an argument object. > Refactor JsonTreeRowRecordReader constructor to take a parameter object > --- > > Key: NIFI-13989 > URL: https://issues.apache.org/jira/browse/NIFI-13989 > Project: Apache NiFi > Issue Type: Improvement >Reporter: Daniel Stieglitz >Assignee: Daniel Stieglitz >Priority: Minor > > When attempting a fix for NIFI-13963 I found it very hard to add another > argument as there already 13 arguments for the current constructor. This is a > code smell Martin Fowler identifies as a long parameter list > [https://luzkan.github.io/smells/long-parameter-list.|https://luzkan.github.io/smells/long-parameter-list] > One of the solutions is to create a parameter object to encompass all the > arguments. -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Commented] (NIFI-13989) Refactor JsonTreeRowRecordReader constructor to take a parameter object
[ https://issues.apache.org/jira/browse/NIFI-13989?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17896715#comment-17896715 ] David Handermann commented on NIFI-13989: - [~dstiegli1] I do not recommend making this change. Although a large number of arguments can be a problem in some cases, simply refactoring that for its own sake does not seem worth the effort right now. > Refactor JsonTreeRowRecordReader constructor to take a parameter object > --- > > Key: NIFI-13989 > URL: https://issues.apache.org/jira/browse/NIFI-13989 > Project: Apache NiFi > Issue Type: Improvement >Affects Versions: 2.0.0 >Reporter: Daniel Stieglitz >Assignee: Daniel Stieglitz >Priority: Minor > > When attempting a fix for NIFI-13963 I found it very hard to add another > argument as there already 13 arguments for the current constructor. This is a > code smell Martin Fowler identifies as a long parameter list > [https://luzkan.github.io/smells/long-parameter-list.|https://luzkan.github.io/smells/long-parameter-list] > One of the solutions is to create a parameter object to encompass all the > arguments. -- This message was sent by Atlassian Jira (v8.20.10#820010)
