Grade my connection logic..

  • (4 Pages)
  • +
  • 1
  • 2
  • 3
  • Last »

47 Replies - 2058 Views - Last Post: 30 September 2013 - 02:42 PM Rate Topic: -----

#1 synlight  Icon User is offline

  • D.I.C Addict
  • member icon

Reputation: 89
  • View blog
  • Posts: 582
  • Joined: 14-September 11

Grade my connection logic..

Posted 18 September 2013 - 01:07 PM

I'm working with application databases for the first time (WHY didn't we learn that in school?!?).

Writing software in VS 2012, WPF, using SQLite with System.Data.SQLite. I've used tutorials to get me this far, and so far I can add to my tables, and query them to make sure fields (i.e. username/password) match what is in the database.

Now I'm onto more complex operations, such as filling a DataGrid with query results, so the user can select a device to delete from the database. So. Here's what I was thinking, and hoping to get feedback on.

I built a class called Database, that stores my connection string, and methods to retrieve and manipulate data. I want my forms code behind to call these methods, and use the data returned to populate controls for the end user.

Here is a small example:

From the database class:

public class Database
    {
        
        private SQLiteConnection dbConnection;
        
        //DEFAULT CONSTRUCTOR
        public Database()
        {
            dbConnection = new SQLiteConnection("Data Source = CervellaDB.sqlite; Version=3;");
        }

#region DataTable
        public DataTable GetDataTable(string sql)
	    {
	        DataTable data = new DataTable();
	        try
	        {
	            SQLiteConnection cnn = new SQLiteConnection(dbConnection);
	            cnn.Open();
	            SQLiteCommand mycommand = new SQLiteCommand(cnn);
	            mycommand.CommandText = sql;
	            SQLiteDataReader reader = mycommand.ExecuteReader();
	            data.Load(reader);
                reader.Close();
	            cnn.Close();
                return data;
	        }
	        catch (Exception e)
	        {
	            throw new Exception(e.Message);
	        }
	        
	    }//END GETDATATABLE
#endregion

    }//END DATABASE CLASS





And from a WPF from code behind:

 private void DeleteDevice_Loaded(object sender, RoutedEventArgs e)
        {
            Database db = new Database();
            DataTable data = new DataTable();
            string query = "SELECT devHostName, devLocation FROM Devices;";
            data = db.GetDataTable(query);

            SQLiteDataAdapter adapter = new SQLiteDataAdapter();
            SQLiteCommand mycommand = new SQLiteCommand();
            dataGridData.ItemsSource = data.DefaultView;
            this.dataGridData.DataContext = data;
     
         }//END LOADED HANDLER



As I'm sure you can tell, my Loaded event instantiates a new db object and datatable, then calls the getDataTable method of the database class, passing it the sql I want the method to execute. The method executes the sql, puts the results in a datatable, which it then returns. Then I'm trying to display the datatable in the dataGridView control.

This is my first professional project, and I have no one to ask advice. Seeing as how I had never touched C# or WPF a month ago, I guess I'm doing okay. My ultimate goal is to write a clean, efficient, reliable professional application. Eventually I will switch it all up into a MVVM model, but for now, I just want to get the basics down.

So.. what do you think? Am I making any glaring mistakes with my logic here? Right now the datagrid only displays the column names LOL, so if you want to give me a pointer there, I'd gladly take it (although I'll figure it out on my own, no big deal). The real reason I'm posting is to get some feedback on my choices with the logic - am I over complicating things?


Edit to add: I have about 10 WPF forms in this software that use the Database class. Also, I think I'll look back on this code in a year and laugh my butt off. I'm climbing a mountain here with this learning curve.

This post has been edited by synlight: 18 September 2013 - 01:12 PM


Is This A Good Question/Topic? 0
  • +

Replies To: Grade my connection logic..

#2 Michael26  Icon User is offline

  • DIC-head, major DIC-head
  • member icon

Reputation: 358
  • View blog
  • Posts: 1,525
  • Joined: 08-April 09

Re: Grade my connection logic..

Posted 18 September 2013 - 01:18 PM

When you make instance of Database class you are calling your default constructor which is fine, but you don't need to make 2 instances in there, one in your constructor and 1 in your GetDataTable method.

I would instead do
public Database()
        {
            dbConnection = new SQLiteConnection("Data Source = CervellaDB.sqlite; Version=3;");
            dbConnection.Open();
        }

And delete line 18 and 19 in your GetDataTable method, you would still have access to your dbConnection since it is declared at class level scope. And change line 25 from cnn.close to dbConnection.close

This post has been edited by Michael26: 18 September 2013 - 01:20 PM

Was This Post Helpful? 1
  • +
  • -

#3 synlight  Icon User is offline

  • D.I.C Addict
  • member icon

Reputation: 89
  • View blog
  • Posts: 582
  • Joined: 14-September 11

Re: Grade my connection logic..

Posted 18 September 2013 - 01:22 PM

Thank you for your feedback!!!!!

Okay so I would open the database when I create a dbobject.. but then I would need to close it within each method. How would that work if I wanted to call 2 different methods? Like.. say I called a Query (which would lose the connection)and then an Insert right after? If I had closed the db in the Query method, I wouldn't be able to access it without opening again, correct?

I'm not arguing, I just want to make sure I understand correctly.

Edit!! Oh duh I can't believe I did that (the repeat connections, and using cnn instead of dbConnection).. Thank you!

This post has been edited by synlight: 18 September 2013 - 01:25 PM

Was This Post Helpful? 0
  • +
  • -

#4 Michael26  Icon User is offline

  • DIC-head, major DIC-head
  • member icon

Reputation: 358
  • View blog
  • Posts: 1,525
  • Joined: 08-April 09

Re: Grade my connection logic..

Posted 18 September 2013 - 01:32 PM

You can do this
using(Database db = new Database())
 {
  DataTable data = new DataTable();
  string query = "SELECT devHostName, devLocation FROM Devices;";
  data = db.GetDataTable(query); //Also this method calls the close method on your connection

  SQLiteDataAdapter adapter = new SQLiteDataAdapter();
  SQLiteCommand mycommand = new SQLiteCommand();
  dataGridData.ItemsSource = data.DefaultView;
  this.dataGridData.DataContext = data;
 }


When you are using an object that encapsulates any resource, you have to make sure that when you are done with the object, the object's Dispose method is called. This can be done more easily using the using statement in C#. The using statement simplifies the code that you have to write to create and then finally clean up the object. The using statement obtains the resource specified, executes the statements and finally calls the Dispose method of the object to clean up the object.

Also you can implement the Singleton patern

Quote

This implementation has two main advantages:
Because the instance is created inside the Instance property method, the class can exercise additional functionality (for example, instantiating a subclass), even though it may introduce unwelcome dependencies.
The instantiation is not performed until an object asks for an instance; this approach is referred to as lazy instantiation. Lazy instantiation avoids instantiating unnecessary singletons when the application starts.
The main disadvantage of this implementation, however, is that it is not safe for multithreaded environments. If separate threads of execution enter the Instance property method at the same time, more that one instance of the Singleton object may be created. Each thread could execute the following statement and decide that a new instance has to be created:

if (instance == null)

This post has been edited by Michael26: 18 September 2013 - 01:38 PM

Was This Post Helpful? 1
  • +
  • -

#5 synlight  Icon User is offline

  • D.I.C Addict
  • member icon

Reputation: 89
  • View blog
  • Posts: 582
  • Joined: 14-September 11

Re: Grade my connection logic..

Posted 18 September 2013 - 01:36 PM

whoa. mind == blown.

I thought using statements were only for including references for the class or whatnot.

*off to research using statements*

Oh!! And I fixed my datatable.. it shows the records now instead of just the column headings! :shuriken:/>

This post has been edited by synlight: 18 September 2013 - 01:36 PM

Was This Post Helpful? 0
  • +
  • -

#6 Michael26  Icon User is offline

  • DIC-head, major DIC-head
  • member icon

Reputation: 358
  • View blog
  • Posts: 1,525
  • Joined: 08-April 09

Re: Grade my connection logic..

Posted 18 September 2013 - 01:38 PM

I updated my post.

Quote

*off to research using statements*

you can do this
Understanding the 'using' statement in C#(CodeProject)
using Statement (C# Reference)(MSDN)

This post has been edited by Michael26: 18 September 2013 - 01:43 PM

Was This Post Helpful? 0
  • +
  • -

#7 Curtis Rutland  Icon User is online

  • (╯□)╯︵ (~ .o.)~
  • member icon


Reputation: 4490
  • View blog
  • Posts: 7,822
  • Joined: 08-June 10

Re: Grade my connection logic..

Posted 18 September 2013 - 01:48 PM

I do have some advice. First, if all these forms are using the Database class, are any of them sharing data? If so, you should have your Database class cache the results so you don't hit the DB more than you have to.

Also, you shouldn't make your forms any smarter than they need to be. For example, in your DeleteDevice_Loaded method/handler, you shouldn't have the query text there. If you're making a database class, you should abstract all the database interactions into that.

Also, I'd suggest a few other things. Use using statements to automatically dispose of resources. The way you're doing it, if there was an exception thrown, your database connection will not close.

Last, this is just preference, but I'd probably call my class DataManager or something like that. The class isn't a database, it's to manage interactions with a database, so it's probably better to be more descriptive.

Here's a sort of example that puts all these suggestions in play. It's not using Sqlite, but I think you'll see what's the same and what's different:

class Program
{
    static void Main(string[] args)
    {
        var manager = new DataManager();
        var people = manager.GetPeople();
        Console.ReadKey();
    }
}

public class DataManager
{
    private const string ConnectionString = "Data Source=TestDb.sdf;Persist Security Info=False;";

    private static class Queries
    {
        public const string SelectPeople = "SELECT Id, FirstName, LastName FROM Person";
    }

    public List<Person> GetPeople()
    {
        var result = new List<Person>();
        using (var conn = new SqlCeConnection(ConnectionString))
        using (var cmd = new SqlCeCommand(Queries.SelectPeople, conn))
        {
            conn.Open();
            var reader = cmd.ExecuteReader();
            while (reader.Read())
            {
                var person = new Person
                {
                    FirstName = (string) reader["FirstName"],
                    LastName = (string) reader["LastName"],
                    Id = (int) reader["Id"]
                };
                result.Add(person);
            }
        }
        return result;
    }
}

public class Person
{
    public int Id { get; set; }
    public string FirstName { get; set; }
    public string LastName { get; set; }
}



This is how I would structure it if I were you. The idea is that your data manager knows everything it needs to to do its job. Your windows shouldn't have to know SQL statements. They should just know that there is a class, and when I ask that class for information, it does whatever it needs to do and gives me back what I want. I added the child class Queries as a convenient place to keep them. But if you really want to do this right, you could externalize those queries to your App.Config file, or to some other resource.

Also, this link might come in handy later:

http://www.dreaminco...ery-a-database/

Also, if you're interested in going a completely different direction, you could use Entity Framework with Sqlite: http://brice-lambson...-on-sqlite.html

There's other tutorials out there for that too. That one's a code-first example, but there are some others that let you make a schema first.
Was This Post Helpful? 2
  • +
  • -

#8 synlight  Icon User is offline

  • D.I.C Addict
  • member icon

Reputation: 89
  • View blog
  • Posts: 582
  • Joined: 14-September 11

Re: Grade my connection logic..

Posted 18 September 2013 - 01:52 PM

@Curtis, what do you mean by sharing data?

I have 2 tables right now, users and devices. I have a user class and a device class that hold the fields for each one.

So far, a user can register, then they can add/delete devices from the db. SO, if they want to add a device, they fill out a form. When they click Add, the code behind takes the form contents and puts it in a device object, then the object properties are inserted into the database to create a new record.

I do need more separation between forms and data, for sure.

I also like the DataManager suggestion, I'll implement that before I leave work today!

As an example... here is the handler for the Add Device click event... which is pretty fail going by what I'm seeing from you guys. But, that's why I posted here.. I felt like I was doing it all wrong after skimming Curtis and Thln Toq's WPF tuts.

Oh and don't mind my weird calling of MessageBox, I wrote a new MessageBox class so I could customize it to match my GUI.

#region click handler
        //Verify valid IP format, add to database
        private void btnAddDevice_Click(object sender, RoutedEventArgs e)
        {

            bool valid = false;
            bool goodIP = false;
            bool goodInsert = false;

            valid = validData();

            if (valid)
            {
                //INSTANTIATE NEW DEVICE
                //TODO: ADD TO DATABASE
                Device newDevice = new Device();
                newDevice.deviceHostName = txtHostname.Text;
                newDevice.deviceLocation = txtLocation.Text;
                newDevice.deviceLoginName = txtLoginName.Text;
                newDevice.devicePassword = txtPassword.Text;
                newDevice.ipAddressString = txtDeviceIPAddress.Text;
                goodIP = newDevice.parseIP(newDevice.ipAddressString);

                if (goodIP)
                {
                    
                    Database db = new Database();

                    //Add to database
                    Dictionary<String, String> data = new Dictionary<String, String>();
                    data.Add("devHostName", newDevice.deviceHostName);
                    data.Add("devLoginName", newDevice.deviceLoginName);
                    data.Add("devPassword", newDevice.devicePassword);
                    data.Add("devLocation", newDevice.deviceLocation);
                    data.Add("devIPString", newDevice.ipAddressString);
               
                    //attempt database insert of new device
                    try
                    {
                        
                     goodInsert = db.Insert("Devices", data);
                    }
                    catch (Exception)
                    {
                        MessageBox alert = new MessageBox("Device addition failed. Please contact CervellaAM Support.");
                        alert.ShowDialog();
                    }

                    if(goodInsert)
                    {
                        //if insert is good, open Dashboard
                        MessageBox alert2 = new MessageBox("New device registered.");
                        alert2.ShowDialog();
                        Dashboard dashboard = new Dashboard();
                        this.Close();
                        dashboard.ShowDialog();
                    }
                }//ENDIF GOODIP
           }//ENDIF Valid

           else
           {
                txtDeviceIPAddress.Text = "";
           }
        }//END CLICK HANDLER
#endregion

This post has been edited by synlight: 18 September 2013 - 01:59 PM

Was This Post Helpful? 0
  • +
  • -

#9 Curtis Rutland  Icon User is online

  • (╯□)╯︵ (~ .o.)~
  • member icon


Reputation: 4490
  • View blog
  • Posts: 7,822
  • Joined: 08-June 10

Re: Grade my connection logic..

Posted 18 September 2013 - 02:14 PM

Quote

@Curtis, what do you mean by sharing data?


I edited my post to be a bit clearer, but I'm basically saying that if any two areas are going to request the same data sets, you should probably store them in memory instead of going back to the database again, assuming the data isn't likely to change in between the loads. Not a critical thing, and if you don't implement it it's no big deal. But if you're loading lots of data, it can save lots of performance.

I do highly suggest that you make models for your data objects, like the Person class I used as an example, if you haven't already. That way, your forms don't have to understand datatables either. They just have to understand simple models. You create instances of models (or lists of instances) in the DataManager and return them instead of the data table. The idea is to keep each class as specialized as possible. No sense in scattering the DB logic all over the code. Keep it in one area.

Last bit of advice, you're doing way too much in your event handlers. Extract chunks of functionality as methods, so that you have both re-usability and clarity of code. You can call these methods from your event handlers. This has the added benefit of letting you use these methods from other points of code, not just the event handler. And if you have a logical grouping of those methods, it's probably worth making a class for.
Was This Post Helpful? 1
  • +
  • -

#10 Curtis Rutland  Icon User is online

  • (╯□)╯︵ (~ .o.)~
  • member icon


Reputation: 4490
  • View blog
  • Posts: 7,822
  • Joined: 08-June 10

Re: Grade my connection logic..

Posted 18 September 2013 - 02:29 PM

Quote

I felt like I was doing it all wrong after skimming Curtis and Thln Toq's WPF tuts.


Oh, missed that part. I'm really happy to be able to help someone willing to learn, and glad that our advice is useful to people Try not to get discouraged, because no more than four years ago my code was a lot like yours. You get better with practice and time. And real projects. That's the best way to learn. Examples only take you so far, but when you need to learn something, it tends to stick. And real-world cases always stick concepts in your mind better than abstract examples.

Quote

Oh and don't mind my weird calling of MessageBox, I wrote a new MessageBox class so I could customize it to match my GUI.


One last bit of advice: I'd suggest against using names that already exist in the Framework for your own components, mainly because of the confusion. There's no conflict because of namespaces, but it does get a bit tricky to keep them straight in your mind, or worse the mind of other people reading your code.

Quick tip for renaming: use the built-in Refactoring commands. They do things like smart, scope-aware renaming. So you can re-name your variables in one method automatically without affecting the ones in other methods. Or if you change the name of a class with this, it will be changed in all other files that use it in the project. The shortcuts for this are F2 or Ctrl-R,R. You can also right-click and use the Refactor menu.
Was This Post Helpful? 1
  • +
  • -

#11 Michael26  Icon User is offline

  • DIC-head, major DIC-head
  • member icon

Reputation: 358
  • View blog
  • Posts: 1,525
  • Joined: 08-April 09

Re: Grade my connection logic..

Posted 19 September 2013 - 03:59 AM

Curtis Rutland
look at this
 using (var conn = new SqlCeConnection(ConnectionString))
        using (var cmd = new SqlCeCommand(Queries.SelectPeople, conn))

Is that intended to be that way? I though that when statement is executed in the using parenthesis it is disposed, and i don't see where did you declared your conn outside of the using, in other words after execution conn will be null.

I think what you meant to say is
using (var conn = new SqlCeConnection(ConnectionString)){
        var cmd = new SqlCeCommand(Queries.SelectPeople, conn)
}

This post has been edited by Michael26: 19 September 2013 - 04:01 AM

Was This Post Helpful? 0
  • +
  • -

#12 MrShoes  Icon User is offline

  • D.I.C Regular
  • member icon

Reputation: 312
  • View blog
  • Posts: 488
  • Joined: 13-June 12

Re: Grade my connection logic..

Posted 19 September 2013 - 04:13 AM

Michael26, what Curtis Rutland did is perfectly acceptable, if not so easy to read.

It works because there is only a single line of code below the first, so the {} are note needed; it's a single line nested below it. However, that line then needs the {} because there are multiple lines nested under it.

Basically, there's no difference between
using(disposableObject1)
{
    using(disposableObject2)
    {
        // Do stuff
    }
}    



and

using(disposableObject1)
    using(disposableObject2)
    {
        // Do stuff
    }  


This post has been edited by MrShoes: 19 September 2013 - 04:27 AM

Was This Post Helpful? 1
  • +
  • -

#13 synlight  Icon User is offline

  • D.I.C Addict
  • member icon

Reputation: 89
  • View blog
  • Posts: 582
  • Joined: 14-September 11

Re: Grade my connection logic..

Posted 19 September 2013 - 05:26 AM

Just wanted to let you guys know I really appreciate the input. It's a lot to take in at once, so it's going to take me some time to read it, digest it, and figure out what I want to/should apply to my coding style. There seems to be a general consensus with a few variations. Right now my database stuff works, but I want it to do more than just work, I want it to be RIGHT. I have given myself until Monday to bring it up to standard, then I have to move on to IP Connectivity in my software. I've scheduled testing to start the first week of October.

After the first release, I have a few months to refine it and add more features.

ETA: I'll probably pop back in today with questions as research, and implement suggestions you guys have made. My database code is kind of patched together from several different examples (that I used to write my own, of course).. so I feel like it's not consistent. Anyway.. hope you guys don't mind me picking your brains, it is MUCH appreciated.

This post has been edited by synlight: 19 September 2013 - 05:34 AM

Was This Post Helpful? 0
  • +
  • -

#14 MrShoes  Icon User is offline

  • D.I.C Regular
  • member icon

Reputation: 312
  • View blog
  • Posts: 488
  • Joined: 13-June 12

Re: Grade my connection logic..

Posted 19 September 2013 - 05:52 AM

I'd like to suggest the use of an ORM (Object Relational Mapping) to take care of your DB code. I've found Entity Framework to be great, using the EF Code Generator to create classes from my EDMX (in fact, I plan on writing a tutorial about the very thing soon). Some prefer NHibernate. Either way, an ORM can mean the Data Access Layer is almost automatically generated, leaving you with the logic required to interact with it in simple C# code.
Was This Post Helpful? 1
  • +
  • -

#15 synlight  Icon User is offline

  • D.I.C Addict
  • member icon

Reputation: 89
  • View blog
  • Posts: 582
  • Joined: 14-September 11

Re: Grade my connection logic..

Posted 19 September 2013 - 05:56 AM

I'm suddenly very frustrated with how little I know.

So.. @Curtis made a great suggestion about storing my datasets in memory if they will be shared (which they will).

I'm struggling to find a way to communicate my needs without violating my confidentiality clause..

Okay.. so a user will likely store several hundred records in the device db. They will need access to these records on a near constant basis. So it seems like that is a case where I should keep the dataset available at all times. But will maintaining such a large dataset cause more performance issues than it prevents??

View PostMrShoes, on 19 September 2013 - 07:52 AM, said:

I'd like to suggest the use of an ORM (Object Relational Mapping) to take care of your DB code. I've found Entity Framework to be great, using the EF Code Generator to create classes from my EDMX (in fact, I plan on writing a tutorial about the very thing soon). Some prefer NHibernate. Either way, an ORM can mean the Data Access Layer is almost automatically generated, leaving you with the logic required to interact with it in simple C# code.


Haha I have no idea what you just said, but I'll add it to list of things I need to look into today!

Perhaps I need to take a step back.. go to the root of application database logic here. In theory I kind of know what a Database Access Layer would do.. but it's a very nebulous concept. That's can't be a good thing. Crawl before you walk, and such.
Was This Post Helpful? 0
  • +
  • -

  • (4 Pages)
  • +
  • 1
  • 2
  • 3
  • Last »