[raziel_]

hi to everyone

i have this code and i was wondering if this is the right way to use try-catch phrase.

        private void Save_URight()
        {
            try
            {
                if (Program.cnn.State == ConnectionState.Closed) { Program.cnn.Open(); }
              'do some stuf
            }
            catch (OleDbException ex)
            {
               'error and exit'
                return;
            }
           'do some stuff'
            try
            {
              'do some stuff here'
                try
                {
                     'then do some other stuff'
                    try
                    {
                       'another try-catch'
                    }
                    catch (OleDbException ex)
                    {
                      'error msg.'
                        return;
                    }
                }
                catch (OleDbException ex)
                {
                'another catch and exit'
                    return;
                }
            }
            catch (OleDbException ex)
            {
               'catch again'
            }
        }

more important is i always catch OleDBExceptions because i only use this to save data in the database, can i make a 1 catch not using 3 of them.

any clarification and help will be much appreciated

This post has been edited by NoBrain: 10 September 2010 - 05:22 AM

[jens]

Hi!
'
Depends on what 'do other stuff implies. Would say that you should end one try before starting the next since you are catching the same kind of error. Maybe this and the following tutorials can be useful even though they are written in VB.NET.

Regards
Jens

[ragingben]

You can catch multiple exceptions from a try block and handle them differently, and the try block applies for all code within it. With these two things in mind, you only need to have one try/catch/finally block. Also as your catching the exceptions you should really return the result of your save_URight method as a boolean, so the app can handle the result of the method however you want. Here;s how I'd go about it...

private Boolean save_URight()
{
    try
    {
        // do whatever
    }
    catch (OleDbException ex)
    {
        // handle the OleDbException
        
        // return fail
        return false;
    }
    catch (Exception ex)
    {
        // handle all other exceptions
        
        // return fail
        return false;
    }

    // if no errors then return true
    return true;
}

This post has been edited by ragingben: 10 September 2010 - 05:43 AM

[DaneAU]

Just looking at your code, you return each time an exception occurs. Saying that, how many try catch's you have can be limited to one simply because you return the moment an exception occurs. For finding the problem it would be handy to keep multiple ones however saying that something that i do when i get sick of seeing so many blocks is use a simple index of some kind and that way i can determine where my code broke or an exeption occured, it is ghetto but does work.

some pseudo code

private Boolean functionABC() {
  int index = 0;

  try {
     // statment 1
     index++;
     do { 
        //star jumps
     } while ( still_has_enough_energy );
     index++;
     //etc..

  } catch ( Exception ecp ) {
       // print out the ecp and the index ?//
       return false;
  }
  return true; // no exceptions caught

}

This post has been edited by DaneAU: 10 September 2010 - 06:06 AM

[raziel_]

see my program do this:

        private void Save_URight()
        {
            try
            {
                if (Program.cnn.State == ConnectionState.Closed) { Program.cnn.Open(); }
            }
            catch (OleDbException ex)
            {
                MessageBox.Show("Error opening DB" + Environment.NewLine + ex.Message,
                    "PL", MessageBoxButtons.OK, MessageBoxIcon.Error);
                return;
            }
            string strSQL = "INSERT INTO URight (OpRights) VALUES(@orgh)";
            OleDbCommand CMD = new OleDbCommand(strSQL, Program.cnn);
            CMD.Parameters.AddWithValue("@orgh", txtNameRight.Text);
            int intRecAff = 0;
            try
            {
                intRecAff = CMD.ExecuteNonQuery();
                if (intRecAff != 1)
                {
                    MessageBox.Show("Error in saving" + Environment.NewLine +
                                        intRecAff.ToString() + " records affected", "PL",
                                                        MessageBoxButtons.OK, MessageBoxIcon.Error);
                    if (Program.cnn.State == ConnectionState.Open) { Program.cnn.Close(); }
                    return;
                }
                CMD.Dispose();
                CMD=null;
                if(Program.cnn.State==ConnectionState.Closed){Program.cnn.Open();}
                CMD=new OleDbCommand("SELECT MAX(ID) FROM URight",Program.cnn);
                try
                {
                    strSQL = "INSERT INTO URType (URID, fName, cName, bView, bEdit, bNew) VALUES (@udid, " +
                           "@fnm, @cnm, @bv, @be, @bn)";
                    if (Program.cnn.State == ConnectionState.Closed) { Program.cnn.Open(); }
                    CMD = new OleDbCommand(strSQL, Program.cnn);
                    //Not yet finished!!!!!!!!!!!!!!!!!!!!
                    //CMD.Parameters.AddWithValue();
                    OleDbDataReader DR;
                    try
                    {
                        DR = CMD.ExecuteReader();
                        if (DR.HasRows)
                        {
                            while (DR.Read())
                            {
                                lngIDOpR = Convert.ToInt64(DR[0]);
                            }
                        }
                    }
                    catch (OleDbException ex)
                    {
                        MessageBox.Show("Error DB" + Environment.NewLine +
                                        ex.Message, "PL", MessageBoxButtons.OK,
                                                                                MessageBoxIcon.Error);
                        if (Program.cnn.State == ConnectionState.Open) { Program.cnn.Close(); }
                        return;
                    }
                }
                catch (OleDbException ex)
                {
                    MessageBox.Show("Error in DB" + Environment.NewLine +
                                    ex.Message, "PL", MessageBoxButtons.OK,
                                                                            MessageBoxIcon.Error);
                    if (Program.cnn.State == ConnectionState.Open) { Program.cnn.Close(); }
                    return;
                }
            }
            catch (OleDbException ex)
            {
                MessageBox.Show("Error in db" + Environment.NewLine +
                                ex.Message, "PL", MessageBoxButtons.OK, MessageBoxIcon.Error);
                if (Program.cnn.State == ConnectionState.Open) { Program.cnn.Close(); }
            }
        }

so as you can see i mainly write in the database and all the errors are from the OleDB. do i need to do it like so or just one exception for the OleDBException is needed. i am not in the phase to catch the other errors yet.

[ragingben]

Well, what you have should work.

I don't see a problem with nested try/catch/finally blocks, and it does allow other sections of code within the method to continue even if one section fails, although in your code that is not applicable as there is no code after any of the catch blocks (except for the block right at the start).

Nesting catch blocks is not something I normally do however, but your code should function well enough, and as far as OleDbException's go it looks pretty robust

Personally I would go for a one try/catch/finally block for the whole method, and I would interrorgate the code with if statements for debugging, and possibly throw exceptions if theres an error. If each section needs to have a try/catch block I would consider breaking the method down into seperate methods for each bit of the task, and then each of those methods would have a try/catch block of their own.

[Imdsm]

You could do it that way, but if you don't need to then don't.

For example, you're returning on error, so you don't really need so many. You could just have a switch or couple of ifs in the catch and return the error there.

If, however, you was not returning when you caught the errors, and had reason to, then it would be fine. If you wanted to have some code try and do something, and then try and do something else, etc, without one thing stopping all the others, then you could use it. However, it is ugly and bad code, and I'm sure there are much nicer ways to implement such a method.

As for your comments, please remember this isn't basic based, so REM and ' won't work.

[raziel_]

come to think of it i will do this. break it to little methods with his own throw catch. ty for your help

[DaneAU]

Thats a better idea, makes it easier to manage.

This post has been edited by DaneAU: 10 September 2010 - 06:26 AM

[Imdsm]

DaneAU, on 10 September 2010 - 05:25 AM, said:

Thats a better idea, makes it easier to manage.

Not only is it easier to manage, but a good way to code. Each time your code does 3 different things, split it up into 3 different methods and call each, if some of that code has to do some other stuff, split that up too, so when you're reading a block of code, it has ONE focus, ONE function. Also, by doing this, you avoid duplication - which can actually cause programmers brains to set on fire and melt their brains..

Off-topic but to read more about code that melts peoples brains click here.

[Ace26]

First of all, I'll give you one advice that'll be helpful to you forever(as it's been for me): Always write code keeping in mind that
someone apart from you might use it. And they might not have you around to explain it.

That said, the try...catch...finally have their specific purposes and how they work.

try
{
 //you put (every) code that might raise exceptions here.
 //No need writing more than one try...catch sequence in the
 //same method block; that's absolutely redundant.
 
}
catch(SpecificExceptionType)
{
 //catch and handle all exceptions of type SpecificExceptionType
 //here.
}
catch(AnotherSpecificExecptionType)
{
 //catch and handle all exceptions of type AnotherSpecificExecptionType
 //here.
}
.
.
.
catch(Exception)
{
 //Any type of exception can be caught and handled by this.
 //Include this to catch and handle any other exceptions you have
 //not specifically accounted for. 
 //ALWAYS MAKE THIS THE LAST AFTER SPECIFIC CATCH BLOCKS.
}
finally
{
 //Close and release delicate resources like file handles, network connections,
 //stream objects e.t.c here as this will DEFINITELY run inspite of the raised
 //exception.
}

With this in place, the very moment an exception is encountered and raised execution will cease in that method.
The runtime then looks for the appropriate handler first. In the absence of the specific handler of that exception,
it will resort to the general exception catch handler.
As for closing your DBConnection object, always do that in the finally block.

Goodluck as you restructure your code using the appropriate standard cause I honestly think it needs it.

Cheers Amigo.

[baavgai]

The big thing I would suggest is not having a global connection object. You want to connect and disconnect as quickly as possible. Create connections and always close them. They shouldn't survive outside a method.

Also, exceptions "bubble up". If they're all the same, catch them in one place.

Here's how I'd lay such a thing out:

private OleDbConnection GetDbConnection() { return new OleDbConnection(this.connStr); }

private int SafeExec(OleDbCommand cmd) {
	int rows = 0;
	try {
		cmd.Connection.Open();
		rows = cmd.ExecuteNonQuery();
	} finally {
		cmd.Connection.Close();
	}
	return rows;
}

private int saveOpRights(string name) {
	OleDbCommand cmd = GetDbConnection().CreateCommand();
	cmd.CommandText = "INSERT INTO URight (OpRights) VALUES(@orgh)";
	CMD.Parameters.AddWithValue("@orgh", name);
	return SafeExec(cmd);
}

private void insertURType(???) {
	OleDbCommand cmd = GetDbConnection().CreateCommand();
	cmd.CommandText = "INSERT INTO URType (URID, fName, cName, bView, bEdit, bNew) VALUES (@udid, @fnm, @cnm, @bv, @be, @bn)";
	//Not yet finished!!!!!!!!!!!!!!!!!!!!
	//CMD.Parameters.AddWithValue();
	try {
		cmd.Connection.Open();
		OleDbDataReader reader = cmd.ExecuteReader();
		while (reader.Read()) {
			// is there a point to storing this value?
			// just once?
			// globally?
			lngIDOpR = Convert.ToInt64(reader[0]);
		}
	} finally {
		cmd.Connection.Close();
	}
}


private string Save_URight() {
	/* stop hanging onto a global connection, make it as you need it
	if (Program.cnn.State == ConnectionState.Closed) { Program.cnn.Open(); }
	*/
	
	// break discrete tasks into methods
	// don't tie data store code to form code
	// the only possible return values are 0 or 1
	if (saveOpRights(txtNameRight.Text)!=1) { return "Error in saving\n0 records affected"; }
	
	// you don't seem to use this CMD=new OleDbCommand("SELECT MAX(ID) FROM URight",Program.cnn);

	insertURType(???);
	
	// not sure what else to do...
}

private bool Save_URightWithCatch() {
	string errorMessage = null;
	try {
		errorMessage = Save_URight()
	} catch (OleDbException ex) {
		errorMessage = "Error in db\n" + ex.Message;
	}
	if(errorMessage != null) {
		MessageBox.Show(errorMessage, "PL", MessageBoxButtons.OK, MessageBoxIcon.Error);
	}
	return (errorMessage == null);

}

[raziel_]

why should i not use global connection? other wise i was asking about the same thing ty

just dont get it why not to use global connection? it`s easy to just make it once and open and close it in the rest of the program

[baavgai]

NoBrain, on 16 September 2010 - 02:45 AM, said:

just dont get it why not to use global connection?

Simple: you might leave it open. If you don't have it, you can't do that. Also, in my experience, the checking if a given connection is open or closed can be quirky. I'd rather have a fresh, shiny, connection object and not have to worry.

Ultimately, you don't use the connection so much as a command that's created from it. You're always creating new commands, right? I like to have one command per method. I also like having all the database code in a single class to isolate datastore logic from application logic. A basic "DataManager" will usually start out with something like:

abstract class DataManager {
	protected abstract string ConnectionString { get; }

	protected OleDbConnection GetDbConnection() { return new OleDbConnection(ConnectionString); }

	protected OleDbCommand GetDbCommand() { return GetDbConnection().CreateCommand(); }

	protected OleDbCommand GetDbCommand(string sql) {
		OleDbCommand cmd = GetDbCommand();
		cmd.CommandText = sql;
		return cmd;
	}
}

Then, you want to do some solid code, everthing will usually start out with:

OleDbCommand cmd = GetDbCommand("select userId, fullname from users where username=? and passhash=?");
//...

Every "global" in an object should reflect some element of state. The connection object should not be a part of that. The connection should only be valid for the lifetime of a method call. I simply see no reason to maintain a connection object when it offers no advantage and significant disadvantages.

[raziel_]

a very very nice idea ty so much for your time and help

EDIT: 1 more question if i may. since i`m new to the C# thing how can i access them when they are protected?

This post has been edited by NoBrain: 16 September 2010 - 04:33 AM