Skip to main content
added 1258 characters in body
Source Link
dfhwze
  • 14.2k
  • 3
  • 40
  • 101

Review

  • If the purpose is to output each of the descendant inner exceptions, you are missing all except the first inner exception in case of an AggregateException. And this is a common exception.
  • Don't use \n as new line unless you specifically need to comply to this style of new line. Prefer Environment.NewLine.
  • I am not convinced of the exception != null -> string.Empty clause. This seems to be coded just so you can make a one-liner method implementation. I would check for exception.InnerException == null to short-circuit the return value.
  • msgCount is not hierarchical, so it does not make much sense when dealing with exception trees (see AggregateException). Consider using a count tree ("1.1.4", etc..) or not using a count at all. Perhaps an indentation suits the layout better.
  • Does exception.ToString() provide sufficient information for you? It handles inner exceptions and inner exception trees.

Alternative

This is your code reworked to take into account all the above.

public static string GetExceptionMessages(Exception exception, string indent = "\t") { exception = exception ?? throw new ArgumentNullException(nameof(exception)); indent = indent ?? string.Empty; var builder = new StringBuilder(); GetExceptionMessages(exception, builder, indent, new Stack<string>()); return builder.ToString(); } private static void GetExceptionMessages( Exception exception, StringBuilder builder, string indent, Stack<string> currentIndent) { if (exception == null) return; builder.AppendLine($"{string.Join(string.Empty, currentIndent)}{exception.Message}"); currentIndent.Push(indent); if (exception is AggregateException aggregateException) { foreach (var innerException in aggregateException.InnerExceptions) { GetExceptionMessages(innerException, builder, indent, currentIndent); } } else { GetExceptionMessages(exception.InnerException, builder, indent, currentIndent); } currentIndent.Pop(); } 

Review

  • If the purpose is to output each of the descendant inner exceptions, you are missing all except the first inner exception in case of an AggregateException. And this is a common exception.
  • Don't use \n as new line unless you specifically need to comply to this style of new line. Prefer Environment.NewLine.
  • I am not convinced of the exception != null -> string.Empty clause. This seems to be coded just so you can make a one-liner method implementation. I would check for exception.InnerException == null to short-circuit the return value.
  • msgCount is not hierarchical, so it does not make much sense when dealing with exception trees (see AggregateException). Consider using a count tree ("1.1.4", etc..) or not using a count at all. Perhaps an indentation suits the layout better.
  • Does exception.ToString() provide sufficient information for you? It handles inner exceptions and inner exception trees.

Review

  • If the purpose is to output each of the descendant inner exceptions, you are missing all except the first inner exception in case of an AggregateException. And this is a common exception.
  • Don't use \n as new line unless you specifically need to comply to this style of new line. Prefer Environment.NewLine.
  • I am not convinced of the exception != null -> string.Empty clause. This seems to be coded just so you can make a one-liner method implementation. I would check for exception.InnerException == null to short-circuit the return value.
  • msgCount is not hierarchical, so it does not make much sense when dealing with exception trees (see AggregateException). Consider using a count tree ("1.1.4", etc..) or not using a count at all. Perhaps an indentation suits the layout better.
  • Does exception.ToString() provide sufficient information for you? It handles inner exceptions and inner exception trees.

Alternative

This is your code reworked to take into account all the above.

public static string GetExceptionMessages(Exception exception, string indent = "\t") { exception = exception ?? throw new ArgumentNullException(nameof(exception)); indent = indent ?? string.Empty; var builder = new StringBuilder(); GetExceptionMessages(exception, builder, indent, new Stack<string>()); return builder.ToString(); } private static void GetExceptionMessages( Exception exception, StringBuilder builder, string indent, Stack<string> currentIndent) { if (exception == null) return; builder.AppendLine($"{string.Join(string.Empty, currentIndent)}{exception.Message}"); currentIndent.Push(indent); if (exception is AggregateException aggregateException) { foreach (var innerException in aggregateException.InnerExceptions) { GetExceptionMessages(innerException, builder, indent, currentIndent); } } else { GetExceptionMessages(exception.InnerException, builder, indent, currentIndent); } currentIndent.Pop(); } 
Source Link
dfhwze
  • 14.2k
  • 3
  • 40
  • 101

Review

  • If the purpose is to output each of the descendant inner exceptions, you are missing all except the first inner exception in case of an AggregateException. And this is a common exception.
  • Don't use \n as new line unless you specifically need to comply to this style of new line. Prefer Environment.NewLine.
  • I am not convinced of the exception != null -> string.Empty clause. This seems to be coded just so you can make a one-liner method implementation. I would check for exception.InnerException == null to short-circuit the return value.
  • msgCount is not hierarchical, so it does not make much sense when dealing with exception trees (see AggregateException). Consider using a count tree ("1.1.4", etc..) or not using a count at all. Perhaps an indentation suits the layout better.
  • Does exception.ToString() provide sufficient information for you? It handles inner exceptions and inner exception trees.