Re: Code Critique Requested - C# Discussion Boards

Re: Where is the rest of my Datagrid?

23-Jun-04 9:02

What do you mean "the other half didn't print"? Please be specific.

Note that the method you're using will only work for controls that GDI+ draws. Most of the Windows Forms controls, for example, rely on the GDI drawing in the common controls they encapsulate (most of the .NET controls are just wrappers to existing window classes) so the controls won't draw in their entirety.

Microsoft MVP, Visual C#
My Articles

kornstyle23-Jun-04 9:17

kornstyle

23-Jun-04 9:17

The datagrid is wide enough to fill two pages. Only the first page prints out. I need to find out how to print the entire thing(2 pages).

Re: Where is the rest of my Datagrid?

Heath Stewart23-Jun-04 10:32

23-Jun-04 10:32

You need to use the dimensions for the PrintDocument to calculate the viewport on the DataGrid. There's nothing to help you, really, you just have to perform the calculations yourself.

Microsoft MVP, Visual C#
My Articles

Code Critique Requested

mealnumberone23-Jun-04 7:49

mealnumberone

23-Jun-04 7:49

I am re-writing an old financial application of mine using ASP.NET. At the same time, I am trying to break into programming using OOP. Below is a segment of my application that I wanted to be critiqued by more experienced developers. Did I implement OOP concepts correctly? Am I wasteful with resources? Any other suggestions? Thank you. Smile | :)

The underlying database table looks like this:
create table Positions
(
SystemId int primary key identity(1,1) not null,
Name varchar(25) not null ,
)

There are two classes:
- Portfolio class which right now only contains properties for my business object;
- PositionsDb class which is responsible for database operations of the application.

namespace Positions
{
using System;
using System.Data;
using System.Data.SqlClient;
using System.Collections;

// Portfolio class.
public class Portfolio
{
private int systemId;
private string name;

public Portfolio() { ; }

public int SystemId
{
get { return systemId; }
set { systemId = value; }
}

public string Name
{
get { return name; }
set { name = value; }
}
}

// PositionsDb class. Handles database operations of the application.
public class PositionsDb
{
private SqlConnection connection;

public PositionsDb()
{
;
}

~PositionsDb()
{
connection.Close();
}

public void ConnectToDb()
{
connection = new SqlConnection("Data Source=localhost;Initial Catalog=Positions;User Id=PositionsUser;Password=whatever");
connection.Open();
}

public void AddPortfolio(Portfolio portfolio)
{
// Adds a new portfolio to the database
SqlCommand myCommand;
myCommand = new SqlCommand("AddPortfolio", connection);
myCommand.CommandType = CommandType.StoredProcedure;

myCommand.Parameters.Add(new SqlParameter("@Name", SqlDbType.VarChar, 25));

myCommand.Parameters["@Name"].Value = portfolio.Name;
myCommand.ExecuteNonQuery();
}

public ArrayList GetAllPortfolios()
{
// Gets all portfolios from the database
ArrayList myArrayList = new ArrayList();
Portfolio tempPortfolio;

SqlCommand myCommand;
myCommand = new SqlCommand("GetAllPortfolios", connection);
myCommand.CommandType = CommandType.StoredProcedure;

SqlDataReader myReader;

myReader = myCommand.ExecuteReader();

while (myReader.Read())
{
tempPortfolio = new Portfolio();
tempPortfolio.SystemId = myReader.GetInt32(0);
tempPortfolio.Name = myReader.GetString(1);
myArrayList.Add(tempPortfolio);
}

myReader.Close();
return myArrayList;
}
}
}

Daniel Turini23-Jun-04 8:38

Daniel Turini

23-Jun-04 8:38

Although not a critique about your OOP design, which is, BTW, very hard to do without a complete model: a single class can hardly be called "OOP".
I'd like to point you to some scalability factors:
1. ADO.NET implements connection pooling. So, don't get a connection and keep it on a class field. A connection should always be opened as late as possible and "closed" (actually, returned to the pool) as late as possible.
2. C# doesn't have destructors. The syntax you used, ~PositionsDB() is for finalizers, which is a very different thing. Read more about the IDisposable interface (often referred as IDisposable pattern) or you'll kill your scalability.
3. SqlCommand, SqlDataReader and SqlConnection are IDisposable objects: you should either call Dispose on them, or, better, use the "using" statement. As a rule of thumb, for every "new" or method that returns a reference, it's the caller responsibility to call Dispose when done. Even in a situation of an error, so if you can't use the "using" statement, put the Dispose on a try/finally block.

Yes, even I am blogging now!

Heath Stewart23-Jun-04 8:50

23-Jun-04 8:50

Actually, calling Dispose on an IDisposable implementation isn't always necessary. As you said, if you don't, it could kill scalability.

Disposing objects accomplishes two things (well, depending on implementation, but here's what supposed to happen): close/free native resources, and possibly close/free managed resources. It's when classes use native resources (like the FileStream) that disposing becomes necessary (otherwise you get memory leaks). Disposing completely managed resources cleans-up the object immediately and typically surpresses finalization by the GC. It's this that can help scalability.

Still, though, it's typically a good idea to dispose disposable objects when you're finished with them regardless of what is necessary. The using block statement will do that, even in an exception is thrown inside the block:

using (FileStream file = new FileStream(...))
{
  file.Write(...);
}

BTW, Daniel, this is mostly an extension of your comment to the original poster - it's not really directed at you.

Microsoft MVP, Visual C#
My Articles

Daniel Turini23-Jun-04 9:00

Daniel Turini

23-Jun-04 9:00

Heath Stewart wrote:
Actually, calling Dispose on an IDisposable implementation isn't always necessary.
While I understand why you say this (I can't find a better example than MemoryStream, a completely managed resource), I disagree.
If a class implements an IDisposable interface, it is part of the design contract of that specific class. Your code should never depend on the implementation details of a class, so, if the class author asked you to dispose the object, you should call it: on a newer version or SP, your perfectly good code may suddenly stop working.

Yes, even I am blogging now!

Heath Stewart23-Jun-04 9:06

23-Jun-04 9:06

A good guideline to follow, I agree, but there's also a guideline that disposable implementors should follow where you declare Dispose(bool) as a virtual method. The finalize and IDisposable.Dispose pass false and true respectively.

Still, I agree with what you say, but just pointing out that it isn't always necessary (for example, the MemoryStream you mentioned); it's still a good idea, though. Smile | :)

Microsoft MVP, Visual C#
My Articles

mealnumberone23-Jun-04 9:11

mealnumberone

23-Jun-04 9:11

Daniel,

Sorry if I am asking a dumb question but on your second point, do you mean I should do this?

public class PositionsDb: IDisposable
//Implement IDisposable.
public void Dispose()
{
Dispose(true);
GC.SuppressFinalize(this);
}

Daniel Turini23-Jun-04 10:25

Daniel Turini

23-Jun-04 10:25

There are no dumb questions, just dumb answers Smile | :)

No, I meant like this:

public void AddPortfolio(Portfolio portfolio)
{
  // Adds a new portfolio to the database
  using (SqlCommand myCommand = new SqlCommand("AddPortfolio", connection))
  {
        myCommand.CommandType = CommandType.StoredProcedure;

        myCommand.Parameters.Add(new SqlParameter("@Name", SqlDbType.VarChar, 25));

        myCommand.Parameters["@Name"].Value = portfolio.Name;
        myCommand.ExecuteNonQuery();
  }
}

Yes, even I am blogging now!

Steven Campbell23-Jun-04 9:42

Steven Campbell

23-Jun-04 9:42

PositionsDB class should not be handling the connection itself. There are several reasons for this, but the most obvious is that the code would be duplicated in all of your database objects.

Better is to have the connection object created outside of your PositionsDB class, and then to pass that object as a parameter to PositionsDB. This allows much better flexibility in terms of transactions, as well as allowing other code to control the connection. The connection itself can be created by a Factory Method, (which just means a single piece of code that knows how to create the connection).

Experience has taught me that relying on terminators and dispose methods to close the database connection is not reliable; one or two slips and you will be leaking connections all over.

For handling the scope of the connection, I have found the C# using statement to be helpful, i.e. (pseudocode)

using (myConn = new Conn())<br />
{<br />
..do stuff with myConn<br />
}<br />

DataGrid and end edit

cje23-Jun-04 6:44

cje

23-Jun-04 6:44

I have a simple windows form with a DataGrid bound to a dataset from a database. When the form closes (event 'Closing') I want to flush all changes made in the grid to the database. Everything is fine except the current cell...if I have not left the cell, the data is not updated.
So far I have tried

<br />
private void OnClosing(object sender, System.ComponentModel.CancelEventArgs e)<br />
{<br />
System.Windows.Forms.DataGridCell cell = this.dataGrid1.CurrentCell;<br />
DataGridColumnStyle dgc = this.dataGrid1.TableStyles[0].GridColumnStyles[cell.ColumnNumber];<br />
bool good = this.dataGrid1.EndEdit(dgc, this.dataGrid1.CurrentRowIndex, false );<br />
<br />
this.oleDbDataAdapter1.Update( this.dataSetFeature1 );<br />
}<br />

without luck. Does any one know how to get the current state of the cell into the dataset during 'Closing'?

thanks in advance
cje

Heath Stewart23-Jun-04 8:40

23-Jun-04 8:40

If you want help, be specific. Why is it not working? Are you geting an exception? What exception type are you getting? What message does it contain?

Code similar to what you have is not only recommended in the .NET Framework SDK, but has worked for me and others in the past. More information would be helpful to help you solve your problem.

Microsoft MVP, Visual C#
My Articles

cje23-Jun-04 8:49

cje

23-Jun-04 8:49

Thanks for you interest. I would have been happier if an excpetion was thrown...there are no exceptions thrown, it just simply doesn't work. The text that was entered in to the current cell was not updated to the database. All other cells with modified text were updated correctly. If I modify the text, click on another cell and then close, the text updates properly it is only when I have not left the current cell that I have the problem. When you have had this work, was it in the 'Closing' event of the form? is there a better place to do this processing?

thanks

Heath Stewart23-Jun-04 9:00

23-Jun-04 9:00

Does DataGrid.CurrentCell return anything? Also, in DataGridColumnStyle.EndEdit, just use DataGridCell.RowNumber instead of DataGridCell.CurrentRowIndex. The number of internal instructions is a little less, giving you a small increase in performance.

The Closing event is a good place to do this, although if you're doing this in the Form that contains your DataGrid, don't handle the Closing event; simply override OnClosing:

protected override void OnClosing(CancelEventArgs e)
{
  // Do your stuff, perhaps even setting e.Cancel if needs be
  base.OnClosing(e);
}

This results in much faster code, since the IL instruction callvirt is used instead of many calls to a MulticastDelegate, enumeration of the delegates (handlers), and invocation of the handlers. Overriding the event handler also gives you a little more control. While you should typically call the base class's method, there are some times you might not want to (like not calling base.WndProc for a particular method in order to surpress it from being handled at all).

Note that with control events, if you don't call the method on the base class, the event will not be fired so any other handlers won't be invoked.

Microsoft MVP, Visual C#
My Articles