current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 4 down vote favorite

What can be improved in my code:

        using (var oracleConnection = new OracleConnection(ConnectionString))
        {
            using (var oracleCommand = oracleConnection.CreateCommand())
            {
                oracleConnection.Open();

                oracleCommand.CommandText = 
                "SELECT * 
                 FROM table_sample Table_Sample 
                 WHERE table_sample.id > 1000";

                using (var reader = oracleCommand.ExecuteReader())
                {
                    while (reader.Read())
                    {
                        int id = reader.GetValue(0);
                    }
                }
            }
        }

As the most "dangerous" thing I see the ugly string statement for database query.

I dont want to use Entity Framework for Oracle - cause its not as actual as one for MS SQL Server. And also some Oracle features are not supported.

share|improve this question
1  
You mean like having parametrized queries? Check the docs: msdn.microsoft.com/en-us/library/… – ChrisWue Dec 5 '13 at 8:45
    
@ChrisWue maybe something that looks like EF syntax. Or anything else but not such a long string – MikroDel Dec 5 '13 at 8:49
    
@ChrisWue have look at your link - as I understand it is only the opportunity to use parameters, but string itself stay as ugly as before :) But thanks for this link! – MikroDel Dec 5 '13 at 8:51
1  
AFAIK, you can use EF with other databases, including Oracle. – svick Dec 5 '13 at 11:03
    
@svick its true. But the support for Oracle is not the best one. It is possible that in case of DB First some Oracle features cannot be used. – MikroDel Dec 5 '13 at 11:05
up vote 5 down vote accepted

You're using using blocks to dispose your disposables, which is excellent. However these blocks increase the nesting of your code; since there's nothing between using (var oracleConnection = new OracleConnection(ConnectionString)) and using (var oracleCommand = oracleConnection.CreateCommand()) you could drop the curly braces and "stack" them, like this:

    using (var oracleConnection = new OracleConnection(ConnectionString))
    using (var oracleCommand = oracleConnection.CreateCommand())
    {
        ...
    }

Within that scope, you're reassigning variable Id at each row that gets read; ultimately the value of Id will be that of the last row that was read. I doubt this is the intended behavior.

As for the string query, I agree it's "dangerous" - I prefer (by far) to use an object-relational mapper such as Entity Framework (which as @svick has mentioned has an Oracle provider), but never used it for anything other than SQL Server. I believe you could look into NHibernate as well, or shop around - something like ".net ORM for oracle" should find you some interesting links :)

share|improve this answer
up vote 5 down vote

You have one or two issues in your SQL query:

  • Pointless table aliasing: You aliased the table table_sample as Table_Sample, which is pointless since identifiers in Oracle are case-insensitive.
  • Reliance on case-insensitivity of identifiers: Once you alias the table name, you are supposed to use the alias, not the original. Therefore, if you keep the pointless alias, your WHERE-clause should be WHERE Table_Sample > 1000 to be consistent. Your query happens to work, but only because Oracle identifiers are case-insensitive.

The most obvious solution is to get rid of the alias:

"SELECT * FROM table_sample WHERE table_sample.id > 1000"
share|improve this answer
up vote 2 down vote

I would not use select * as this will retrieve all the columns, instead you should be explicit as to what columns you want to retrieve.

Another potential issue with select * and then calling reader.getValue([index]) is that you are relying on the index you are using for each column to be the same as the the order which they are in the table - this isn't maintainable and open to bugs, again explicitly stating the columns will ensure that the order you are expecting the column data to be in is correct.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.