Do I need to put SQL commands in a separate class?

Question

0.00/5 (No votes)

See more:

Hello everyone,

I have this code:

VB

Private Sub btnSave_Click(sender As Object, e As EventArgs) Handles btnSave.Click

    Using connection As New OleDb.OleDbConnection(conSTR)
        Using command As New OleDb.OleDbCommand("INSERT INTO Judges (FullName, Contact, UserName, [Password]) VALUES (@FullName, @Contact, @UserName, @Password)",
                                connection)
            command.Parameters.AddWithValue("@FullName", txtFullNameJudge.Text)
            command.Parameters.AddWithValue("@Contact", txtContactJudge.Text)
            command.Parameters.AddWithValue("@UserName", txtUserNameJudge.Text)
            command.Parameters.AddWithValue("@Password", txtPasswordJudge.Text)

            connection.Open()

            command.ExecuteNonQuery()
            MessageBox.Show("DATA HAS BEEN ADDED", "SAVE", MessageBoxButtons.OK, MessageBoxIcon.Information)

        End Using
    End Using

What I have tried:

I'm just wondering is this right practice for sql commands? Can I put this in a separate class? If yes, then what is the advantage of that?

Posted 28-Apr-20 19:27pm

lelouch_vi 2

Updated 28-Apr-20 21:31pm

Add a Solution

2 solutions

Add a Solution

Add your solution here

Treat my content as plain text, not as HTML

Preview 0

…

Existing Members

Sign in to your account

...or Join us

Download, Vote, Comment, Publish.

Your Email
Password
Forgot your password?

Your Email
This email is in use. Do you need your password?
Optional Password

I have read and agree to the Terms of Service and Privacy Policy
Please subscribe me to the CodeProject newsletters

When answering a question please:

Read the question carefully.
Understand that English isn't everyone's first language so be lenient of bad spelling and grammar.
If a question is poorly phrased then either ask for clarification, ignore it, or edit the question and fix the problem. Insults are not welcome.
Don't tell someone to read the manual. Chances are they have and don't get it. Provide an answer or move on to the next question.

Let's work to help developers, not make them feel stupid.

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)

phil.o · Accepted Answer · 2020-04-28T19:37:00

Solution 1

There is no real added value in placing this specific piece of code in a separate class. Since it has to access controls on the form, you would have to add boilerplate code to grant access to these values from outside the form.

You could, at a pinch, put it in its own method, instead of directly placing it in the event handler; this would allow you to trigger saving by several ways (the button click, the click on a menu item, etc.).

Finally, usage of Using blocks, as well as the parameterized query, is definitely the way to go.

But, the problem is you seem to record passwords in your database, which is a no go. You should use salting and hashing for it to be more secure. Storing passwords in plain text is one of the worst common practices.

Posted 28-Apr-20 19:37pm

phil.o

Updated 28-Apr-20 19:44pm

v3

Comments

lelouch_vi 2 29-Apr-20 7:42am

Hi phil.o,

Thanks for pointing out the concern about passwords, actually I intend to deal with that later on since I'm just starting to explore vb.net. My primary goal for now is to learn how to write code in a cleaner manner, adapt the best practices of how to write codes efficiently (if there is any please provide me a link) because my code kinda looks messy.

Still thanks for the help!

phil.o 29-Apr-20 8:06am

You're welcome! Your code does not look messy at all. On the contrary, it is quite clear for a rookie's one.

If I had to give another advise, that would be to avoid using AddWithValue method. Better use Add() method[^] instead; this allows to clearly specify the database type you are working with (and prevents some issues with string fields between unicode and non-unicode fields). Applied to your code, this would give:

command.Parameters.Add("@FullName", SqlDbType.NVarChar).Value = txtFullNameJudge.Text
command.Parameters.Add("@Contact", SqlDbType.NVarChar).Value = txtContactJudge.Text
command.Parameters.Add("@UserName", SqlDbType.NVarChar).Value = txtUserNameJudge.Text
command.Parameters.Add("@Password", SqlDbType.NVarChar).Value = txtPasswordJudge.Text

Note that you may have to correct the SqlDbType value according to your database schema.
As to how to write code efficiently, this is the kind of question which is impossible to answer in a Quick Answers question, let alone a comment. Experience is what will bring most answers to it, and remaining curious and always be able to question one's own practices are key elements, too. Wandering through CodeProject and reading questions, answers and comments can help, too.

lelouch_vi 2 29-Apr-20 8:21am

Experience is what will bring most answers to it, and remaining curious and always be able to question one's own practices are key elements, too.

Thanks for these closing remarks and about that Add I tried to avoid it when I'm writing my code above because I can't seem to find the importance of it before and I find the AddWithValue to be simple and easy to write but now that you pointed it out also and looks like AddWithValue will lead into problems in the future then I'll study how to use the Add method.

Thanks again for all the help!

OriginalGriff · Accepted Answer · 2020-04-28T21:33:00

Just to add to what Phil has said, ever store passwords in clear text - it is a major security risk. There is some information on how to do it here: Password Storage: How to do it.[^]

And remember: if you have any European Union users then GDPR applies and that means you need to handle passwords as sensitive data and store them in a safe and secure manner. Text is neither of those and the fines can be .... um ... outstanding. In December 2018 a German company received a relatively low fine of €20,000 for just that.

And while he's right that you can't really put that code in it's own class, you are right that there is a mistake in your "whole app" design in that you have no real separation of concerns: it's a much better idea to adopt a "three layer" model, where the "presentation layer" (which deals with the user directly) is separate from the "business layer" (which deals with the rules for handling and manipulating data) and both are separate from the "data layer" (which deals with the basics of manipulating databases and other forms of storage). It's worth reading up on these: Multitier architecture - Wikipedia[^] will give you the basics.