The Big Programming Thread - Page 204

for (i = stepsToTry; ((i > 0) && (atHome == false)); i = i - 1)

try
{
	using (OracleConnection oracleConnection = new OracleConnection(s3ConnString))
	using (OracleCommand oracleCommand = new OracleCommand(cmdText, oracleConnection))
	using (DataSet dataSet = new DataSet())
	using (OracleDataAdapter oracleDataAdapter = new OracleDataAdapter(oracleCommand))
	{
		oracleCommand.Connection.Open();
		oracleDataAdapter.Fill(dataSet);

		if (dataSet.Tables.Count == 0)
			throw new SomeCustomException("No tables returned.");
		else if (dataSet.Tables.Count > 1)
			throw new SomeCustomException("Too many tables returned.");
		else
			return dataSet.Tables[0];
	}
}
catch (SomeCustomException ex) // Is this even worth customizing?
{
	// Handling
}
catch (OracleException ex)
{
	// Handling
}
catch (Exception ex)
{
	// Handling
}

• Exceptions are for exceptional things, something you can check with a simple length equality doesn't meet those criteria. If your function is named specifically to denote it explicitly expects 1 and only 1 table that's fine, but then there would be no point in catching the exception here. So as it is, you're basically using exceptions for flow control (and unnecessarily so).
  
• There's basically no good reason to catch Exception, don't do this. And if your "handling" would simply be "throw ex;" or some other such stupidity, I'm going to kill you.
  
• You should probably read up on what the using statement does. If you're using the DataSet class from System.Data, I don't believe it implements IDisposable so a using block for that is useless. No idea about the Oracle classes, but this is typically only used for things where there is a finite resource involved, so I'd imagine its only needed for OracleConnection.
  
• This one is more controversial for stupid reasons, but use 'var' in your using statements.
  

  using (OracleCommand oracleCommand = new OracleCommand(cmdText, oracleConnection))

  is far less readable than
  

  using (var oracleCommand = new OracleCommand(cmdText, oracleConnection))

  
  You already know its an OracleCommand from both the constructor and the variable name, no point in typing that a 3rd time. I'd probably ditch all the 'variable name same as the type' stuff too, the type isn't what's important about variables. Rather, their function is.
  

using(var connection = new OracleConnection(s3ConnString)) {
    var command = new OracleCommand(commandText, connection);
    var dataSet = new DataSet();
    var dataAdapter = new OracleDataAdapter(command);

    command.Connection.Open();
    dataAdapter.Fill(dataSet);
    if(dataSet.Tables.Count == 1)
        return dataSet.Tables[0];
    
    throw new InvalidOperationException("Expected 1 table, got: " + dataSet.Tables.Count);
}