Ayende @ Rahien

It's a girl

Code review WTF, number N

  • Using pszSqlString prefix in .NET - BAD
  • Using a naming convention where the company name is prefixed to all local variables - BAD
  • 6 lines of code to log an exception - BAD
  • Rewriting basic parts of the framework to cover bad coding on your parts - BAD

Comments

Paul Stovell
11/13/2007 11:46 AM by
Paul Stovell

One subject in basic pure math that I wish all developers would take is factorization - taking an equation, recognising and removing the duplicates and simplifying.

I still can't fathom how a developer can see the same thing over and over again - like 6 lines of code repeated every single time an exception needs to be logged - and not recognize that they can factor it out.

Paul

Grimace of Despair
11/13/2007 11:49 AM by
Grimace of Despair

"Using a naming convention where the company name is prefixed to all local variables"

I've witnessed a Database that was prefixed with the DEVELOPERS initials, as were all tables in it...

I still shiver while writing this.

Markus Zywitza
11/13/2007 02:10 PM by
Markus Zywitza
  
try
  
{
  
	ActiveRecordMediator.Save(foo);
  
}
  
catch (Exception ex)
  
{
  
	Logger.WarnFormat(ex, "Error saving Foo {0}", foo.Name);
  
	while (ex.InnerException != null)
  
	{
  
		ex = ex.InnerException;
  
		Logger.Info("Inner Exception", ex);
  
	}
  
}
  

If you count the brace lines, it's 6 lines for the exception, too. But absolute necessary unless

Castle.ActiveRecord.Framework.ActiveRecordException: Could not perform Save for Foo at ... in ...

is enough for you to see the error's cause, which isn't the case for me.

Ayende Rahien
11/13/2007 02:18 PM by
Ayende Rahien

Markus, is there a reason that you are doing it this way?

ex.ToString() gives you the inner exception as well, after all

Markus Zywitza
11/13/2007 02:39 PM by
Markus Zywitza

Hmm, it seems that the fact that I always use Logging now avenges. Perhaps I should have just used

Console.WriteLine(exception)

:-(

Markus Zywitza
11/13/2007 02:46 PM by
Markus Zywitza

What is Logging?

No special product, I simply wrote it with a capital letter because I'm accustomed to that practice. German language capitalizes all nouns, hence "Logging" instead of logging.

Ayende Rahien
11/13/2007 02:58 PM by
Ayende Rahien

Well, most of the logging frameworks that I am aware of will handle this correctly.

Markus Zywitza
11/13/2007 03:08 PM by
Markus Zywitza

Windsor's LoggingFacility with loggingApi="console" obviously doesn't.

Paul Stovell
11/13/2007 11:15 PM by
Paul Stovell

@Markus:

class LogHelper

{

public static void Warn(Exception ex, string format, params object[] args) 

{

Logger.WarnFormat(ex, format, args);

while (ex.InnerException != null)

{

ex = ex.InnerException;

Logger.Info("Inner Exception", ex);

}

}

}

Now in the many places you use that code, you can have:

try

{

ActiveRecordMediator.Save(foo);

}

catch (Exception ex)

{

LogHelper.Log("Error saving Foo {0}", foo.Name, ex);

}

}

There are thousands of libraries which require multiple lines to do something. There's no reason you should write those multiple lines twice.

Comments have been closed on this topic.