Commons CSV
  1. Commons CSV
  2. CSV-113

Check whether ISE/IAE are being used appropriately

    Details

    • Type: Task Task
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: Discussion, 1.x
    • Component/s: None
    • Labels:
      None

      Description

      The code throws a lot of IllegalStateExceptions (ISE).
      It also throws some IllegalArgumentExceptions (IAE).

      These need reviewing to check that ISE really does apply to the current state, and IAE is used for reporting an incorrect argument.

      Generally if ISE applies, no argument values will be valid.
      IAE should be used when a specific argument or set of arguments is invalid.

        Issue Links

          Activity

          Hide
          Sebb added a comment -

          CSVFormat.validate() should throw IAE not ISE
          CSVParser.initializeHeader() throws ISE. The header probably ought to be validated in the CSVFormat constructor.

          Should null parameters throw IAE or NPE?
          Needs discussion.

          Show
          Sebb added a comment - CSVFormat.validate() should throw IAE not ISE CSVParser.initializeHeader() throws ISE. The header probably ought to be validated in the CSVFormat constructor. Should null parameters throw IAE or NPE? Needs discussion.
          Hide
          Benedikt Ritter added a comment -

          CSVFormat.validate() should throw IAE not ISE

          CSVFormat.validate() doesn't take parameters. I guess that's the rational for not throwing IAE. What sense does throwing IAE make if no arguments have been passed.

          CSVParser.initializeHeader() throws ISE. The header probably ought to be validated in the CSVFormat constructor.

          yes or in withHeader(String...) if possible

          Should null parameters throw IAE or NPE?

          In my opinion if a null reference is passed to a method that can not handle null, this is an illegal argument and IAE should be thrown. But since the JDK itself handles null inputs by throwing NPE it may be better to align the design this way.

          Show
          Benedikt Ritter added a comment - CSVFormat.validate() should throw IAE not ISE CSVFormat.validate() doesn't take parameters. I guess that's the rational for not throwing IAE. What sense does throwing IAE make if no arguments have been passed. CSVParser.initializeHeader() throws ISE. The header probably ought to be validated in the CSVFormat constructor. yes or in withHeader(String...) if possible Should null parameters throw IAE or NPE? In my opinion if a null reference is passed to a method that can not handle null, this is an illegal argument and IAE should be thrown. But since the JDK itself handles null inputs by throwing NPE it may be better to align the design this way.
          Hide
          Sebb added a comment -

          The validate() Javadoc says it validates the parameters; it checks the stored values to make sure they are self-consistent. It is not a public method; it is basically an implicit build() method for the fluent API.

          Since the values are directly derived from the arguments to the withXXX() methods, I think it is more natural to expect IAE here, rather than ISE. The values don't constitute state, they are just saved arguments.

          Show
          Sebb added a comment - The validate() Javadoc says it validates the parameters; it checks the stored values to make sure they are self-consistent. It is not a public method; it is basically an implicit build() method for the fluent API. Since the values are directly derived from the arguments to the withXXX() methods, I think it is more natural to expect IAE here, rather than ISE. The values don't constitute state, they are just saved arguments.
          Hide
          Benedikt Ritter added a comment -

          The problem with validate() throwing IAE is, that it may be called at a different point in time, location or even system (CSVFormat implements Serializable). validate is not called by CSVFormat itself, but by the Parser/Printer. So it is not coupled to the construction of the Format itself. I think we decided to use ISE for this reason, because by the time validate() is called, that construction have be long over and calling validate() shows the fact that the system now is in an illegal state.

          Show
          Benedikt Ritter added a comment - The problem with validate() throwing IAE is, that it may be called at a different point in time, location or even system (CSVFormat implements Serializable). validate is not called by CSVFormat itself, but by the Parser/Printer. So it is not coupled to the construction of the Format itself. I think we decided to use ISE for this reason, because by the time validate() is called, that construction have be long over and calling validate() shows the fact that the system now is in an illegal state.
          Hide
          Sebb added a comment -

          I see. However, state is normally something that changes throughout the lifetime of an object, so I still have reservations about throwing ISE for this. It's also a bit inconsistent to use ISE just because the validate() may be called some while after using the fluent interface. In most cases, the format will be validated immediately after construction.

          CSVParser.initializeHeader() is called from the constructor, and so IAE is appropriate (though can only now occur for the file header). Fixed in r1593148.

          Show
          Sebb added a comment - I see. However, state is normally something that changes throughout the lifetime of an object, so I still have reservations about throwing ISE for this. It's also a bit inconsistent to use ISE just because the validate() may be called some while after using the fluent interface. In most cases, the format will be validated immediately after construction. CSVParser.initializeHeader() is called from the constructor, and so IAE is appropriate (though can only now occur for the file header). Fixed in r1593148.
          Hide
          Benedikt Ritter added a comment -

          Moved to 1.x

          Show
          Benedikt Ritter added a comment - Moved to 1.x

            People

            • Assignee:
              Unassigned
              Reporter:
              Sebb
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:

                Development