You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by "Gary D. Gregory" <gg...@apache.org> on 2023/05/09 21:43:47 UTC
[IMAGING] Logging vs Throwing exceptions
Hi All,
The method org.apache.commons.imaging.icc.IccProfileParser.getICCProfileInfo(ByteSource) looks like:
public IccProfileInfo getICCProfileInfo(final ByteSource byteSource) {
// TODO Throw instead of logging?
final IccProfileInfo result;
try (InputStream is = byteSource.getInputStream()) {
result = readICCProfileInfo(is);
} catch (final Exception e) {
LOGGER.log(Level.SEVERE, e.getMessage(), e);
return null;
}
//
try {
for (final IccTag tag : result.getTags()) {
final byte[] bytes = byteSource.getBlock(tag.offset, tag.length);
// Debug.debug("bytes: " + bytes.length);
tag.setData(bytes);
// tag.dump("\t" + i + ": ");
}
// result.fillInTagData(byteSource);
return result;
} catch (final Exception e) {
// Debug.debug("Error: " + file.getAbsolutePath());
LOGGER.log(Level.SEVERE, e.getMessage(), e);
}
return null;
}
I find it odd and dubious that we would short-circuit the propagation of exceptions with logging in such a low-level code. Unless someone can argue otherwise, I would like to propagate the exceptions.
TY,
Gary
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [IMAGING] Logging vs Throwing exceptions
Posted by Gary Gregory <ga...@gmail.com>.
Miguel,
Don't cut off the thread in the body of the email please.
Gary
On Wed, May 31, 2023, 01:49 Miguel Muñoz <sw...@gmail.com> wrote:
>
> In addition to logging and swallowing the exception, this method also then
> returns null. This is also a bad practice.
>
>
>
> The caller has to check for null. One of the reasons exceptions were
> invented was to free the user from needing to check for null or error codes.
>
> — Miguel
>
Re: [IMAGING] Logging vs Throwing exceptions
Posted by Peter Hull <pe...@gmail.com>.
That whole class looks like it needs a bit of TLC (or Javadoc at least!)
On Wed, 31 May 2023 at 06:49, Miguel Muñoz <sw...@gmail.com> wrote:
>
>
> In addition to logging and swallowing the exception, this method also then returns null. This is also a bad practice.
>
>
>
> The caller has to check for null. One of the reasons exceptions were invented was to free the user from needing to check for null or error codes.
>
> — Miguel
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [IMAGING] Logging vs Throwing exceptions
Posted by Miguel Muñoz <sw...@gmail.com>.
In addition to logging and swallowing the exception, this method also then returns null. This is also a bad practice.
The caller has to check for null. One of the reasons exceptions were invented was to free the user from needing to check for null or error codes.
— Miguel
Re: [IMAGING] Logging vs Throwing exceptions
Posted by Gilles Sadowski <gi...@gmail.com>.
Le mer. 10 mai 2023 à 12:29, Gilles Sadowski <gi...@gmail.com> a écrit :
>
> Hello.
>
> Le mar. 9 mai 2023 à 23:43, Gary D. Gregory <gg...@apache.org> a écrit :
> >
> > Hi All,
> >
> > The method org.apache.commons.imaging.icc.IccProfileParser.getICCProfileInfo(ByteSource) looks like:
> >
> > public IccProfileInfo getICCProfileInfo(final ByteSource byteSource) {
> > // TODO Throw instead of logging?
> > final IccProfileInfo result;
> > try (InputStream is = byteSource.getInputStream()) {
> > result = readICCProfileInfo(is);
> > } catch (final Exception e) {
> > LOGGER.log(Level.SEVERE, e.getMessage(), e);
> > return null;
> > }
> > //
> > try {
> > for (final IccTag tag : result.getTags()) {
> > final byte[] bytes = byteSource.getBlock(tag.offset, tag.length);
> > // Debug.debug("bytes: " + bytes.length);
> > tag.setData(bytes);
> > // tag.dump("\t" + i + ": ");
> > }
> > // result.fillInTagData(byteSource);
> > return result;
> > } catch (final Exception e) {
> > // Debug.debug("Error: " + file.getAbsolutePath());
> > LOGGER.log(Level.SEVERE, e.getMessage(), e);
> > }
> > return null;
> > }
> >
> > I find it odd and dubious that we would short-circuit the propagation of exceptions
>
> In principle, exception swallowing is not good.
> Perhaps in this case, the result is considered "optional" (?). [In which
> case, the API should be upgraded to use
https://docs.oracle.com/javase/8/docs/api/java/util/Optional.html
> Perhaps the method should not be "public" (?).
>
> > with logging in such a low-level code. Unless someone can argue otherwise, I would like to propagate the exceptions.
>
> Unchecked ones then, I suppose.
>
> Regards,
> Gilles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [IMAGING] Logging vs Throwing exceptions
Posted by Gilles Sadowski <gi...@gmail.com>.
Hello.
Le mar. 9 mai 2023 à 23:43, Gary D. Gregory <gg...@apache.org> a écrit :
>
> Hi All,
>
> The method org.apache.commons.imaging.icc.IccProfileParser.getICCProfileInfo(ByteSource) looks like:
>
> public IccProfileInfo getICCProfileInfo(final ByteSource byteSource) {
> // TODO Throw instead of logging?
> final IccProfileInfo result;
> try (InputStream is = byteSource.getInputStream()) {
> result = readICCProfileInfo(is);
> } catch (final Exception e) {
> LOGGER.log(Level.SEVERE, e.getMessage(), e);
> return null;
> }
> //
> try {
> for (final IccTag tag : result.getTags()) {
> final byte[] bytes = byteSource.getBlock(tag.offset, tag.length);
> // Debug.debug("bytes: " + bytes.length);
> tag.setData(bytes);
> // tag.dump("\t" + i + ": ");
> }
> // result.fillInTagData(byteSource);
> return result;
> } catch (final Exception e) {
> // Debug.debug("Error: " + file.getAbsolutePath());
> LOGGER.log(Level.SEVERE, e.getMessage(), e);
> }
> return null;
> }
>
> I find it odd and dubious that we would short-circuit the propagation of exceptions
In principle, exception swallowing is not good.
Perhaps in this case, the result is considered "optional" (?). [In which
case, the API should be upgraded to use
Perhaps the method should not be "public" (?).
> with logging in such a low-level code. Unless someone can argue otherwise, I would like to propagate the exceptions.
Unchecked ones then, I suppose.
Regards,
Gilles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org