current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

I have a piece of code which grabs data from an SQL database and displays it in a DataGridView. This code works perfectly fine for me.

It has been pointed out to me that the below code is bad programming:

Imports System.Data.SqlClient


Public Class Form1

Dim dbConnection As SqlConnection
Dim dbCommand As SqlCommand
Dim dbAdapter As SqlDataAdapter
Dim DataSet As New DataSet
Dim strSQL As String
Dim dbCount As String

Public Sub SQLConnect()
dbConnection = New SqlConnection("Data Source=connectionhere\sqlexpress;Initial Catalog=line_log;Integrated Security=True")
dbConnection.Open()
End Sub

Public Sub SQLCommand()
dbCommand = New SqlCommand(strSQL, dbConnection)
End Sub

Public Sub SQLAdapter()
dbAdapter = New SqlDataAdapter(strSQL, dbConnection)
End Sub

Public Sub SQLDisconnect()
dbConnection.Close()
End Sub

Public Sub DGVLoad()
Try
SQLConnect()
strSQL = "SELECT * FROM [Products]"
SQLAdapter()
DataSet.Clear()
dbAdapter.Fill(DataSet)
SQLDisconnect()
DataGridView1.DataSource = DataSet.Tables(0)

Catch ex As Exception
MsgBox(ex.ToString)
End Try
End Sub

Private Sub Form1_Load(sender As Object, e As EventArgs) Handles MyBase.Load
DGVLoad()
End Sub

Could someone explain to me why what I am doing is bad practice?

share|improve this question
    
It's hard to believe that "SELECT [item] FROM [table] WHERE ([item] = 'value')" is real code from a real project. How/where is the value of 'value' specified in the real code? Could you post real code instead of example code? –  ChrisW Mar 26 '14 at 10:27
    
Of course the SQL statement it's not real code it was an example, I didn't think a simple SELECT statement would be the problem. I have no problems with the SQL statements to which I am grabbing the information. All this code works perfectly for me, but it was also pointed out to me that the code above portrays bad programming patterns and unmaintainable code. I just wanted someone to show me and explain to me how that is the case. I will edit the select statement to make it understandable. –  James Mar 26 '14 at 10:33
1  
The FAQ about on- and off-topic questions asks, Is it actual code from a project rather than pseudo-code or example code? -- Asking for a review of 'example code' code instead of 'actual code' is like asking a medical doctor to examine an 'example person' instead of an 'actual person'. –  ChrisW Mar 26 '14 at 10:41
    
I understand what you are saying, I do. But lets assume that this code works perfectly, to which it actually does. And I have edited the code above to reflect 'actual code' in the SQL statement, which is now does. Now like I said before, this code is working perfectly, nothing wrong with it for me what so ever. But can you see why or explain why how I have coded it with bad programming practice? I cant see why it is bad practice but the guys at StackOverflow seem to think it is and I was advised to post HERE to get an answer to why it is bad programming to get an answer. –  James Mar 26 '14 at 10:49

2 Answers 2

up vote 2 down vote

Three possible problems that I see.

dbConnection = New SqlConnection("Data Source=connectionhere\sqlexpress;Initial Catalog=line_log;Integrated Security=True")

You have a hard-coded connection string. If you want a different database location then you need to edit the code. Perhaps it's better to have the connection string in an editable config file.

The original version which prompted this review ...

strSQL = "SELECT [item] FROM [table] WHERE ([item] = 'value')"

... seems to build the select statement by string manipulation of the value and therefore may be vulnerable to SQL injection.

You have SQL code and Form code in the same source file. It may be better to separate these into different classes so that they can be tested and re-used independently.

share|improve this answer
    
Thank you very much I appreciate your answer, the hard coded string I have already changed, and as for the select statement I will look into parameterized queries instead. Thank you for your constructive feedback. –  James Mar 26 '14 at 11:35
up vote 0 down vote

I'll try not to rehash what others have said already.

  • Indentation matters. The harder it is to read the code, the harder it is to maintain. Everything inside of Sub...End Sub should be indented one level. (Whether that's one tab or four spaces, I don't care. Just one level.) The same goes for Try...End Try, If...End If, etc.
  • All of your data access code is bound to a Form. Even if this is the only form in your entire project (which I doubt), the code to connect to your database doesn't belong here. It belongs in it's own class. Your form should only be concerned with interacting with the user. It shouldn't care or know how to connect to or query a database.

  • Don't catch exceptions if you don't know what to do with them. 

    Catch ex As Exception
MsgBox(ex.ToString)

    Seriously, stop doing this. You should be as specific as possible about what exceptions you catch. As it is, you're catching potentially fatal exceptions that you probably can't do anything about.

    Fatal exceptions are not your fault, you cannot prevent them, and you cannot sensibly clean up from them. They almost always happen because the process is deeply diseased and is about to be put out of its misery. Out of memory, thread aborted, and so on. There is absolutely no point in catching these because nothing your puny user code can do will fix the problem. Just let your "finally" blocks run and hope for the best.

    Eric Lippert

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.