Re: [IMAGING] Logging vs Throwing exceptions

[Gary Gregory]

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  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

[Peter Hull]

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  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-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [IMAGING] Logging vs Throwing exceptions

[Miguel Muñoz]

  
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

[Gilles Sadowski]

Le mer. 10 mai 2023 à 12:29, Gilles Sadowski  a écrit :
>
> Hello.
>
> Le mar. 9 mai 2023 à 23:43, Gary D. Gregory  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-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [IMAGING] Logging vs Throwing exceptions

[Gilles Sadowski]

Hello.

Le mar. 9 mai 2023 à 23:43, Gary D. Gregory  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-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

[IMAGING] Logging vs Throwing exceptions

[Gary D. Gregory]

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-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org