|
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. 
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;
}
}
}
|
|
|
|
|
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!
|
|
|
|
|
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
|
|
|
|
|
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!
|
|
|
|
|
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.
Microsoft MVP, Visual C#
My Articles
|
|
|
|
|
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);
}
|
|
|
|
|
There are no dumb questions, just dumb answers
No, I meant like this:
public void AddPortfolio(Portfolio portfolio)
{
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!
|
|
|
|
|
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 />
|
|
|
|
|
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
|
|
|
|
|
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
|
|
|
|
|
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
|
|
|
|
|
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)
{
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
|
|
|
|
|
yes, DataGrid.CurrentCell has the proper values in it. I will try your other suggestions - thank you!
|
|
|
|
|
plz help me out here dods..,
i wanna example a simple one please to connect to other pc in the local area network and perform operation like loging off or killing process that will help me alot
ADEL K Khalil
|
|
|
|
|
Uh, basically your going to need...
1. A server app for the remote computer
2. A client app for you
And that's it.
The server will sit on the remote computer listening for for requests on the specified port. This is done with the TcpListener.[^]
Then client app will be on your compter and will use the TcpClient class[^] and take the ip address of the server app and the port in the Connect method.
It's pretty simple. Uh, I think the best way is to look at examples here and on msdn. And, also for the server functions like logging off you can use the API ExitWindowsEx[^]. Uh.... and there are .net classes like Process for killing apps and stuff...
yeah.... SO here's the overview. Make the server and the client. Start the server on the remote machine and start it listening for incoming connections. then start your client then say... You have some preconfigurations in your server code that says if the incoming message says "logoff" then your server will run the ExitWindowsEx method. So, with your client enter the name/ip of your remote computer and then send "logoff" to the server then your remote comp. will logoff. there. done.
I hope my links work. Remember, theres lots or examples here and msdn.
/\ |_ E X E GG
|
|
|
|
|
thanks that help me alot
ADEL K Khalil
|
|
|
|
|
RealVNC.COM (free, easy, works).
|
|
|
|
|
This easiest way is to use the WSH and vbscript. Here is a quick sample
<br />
'strCompName = "." 'This is for the local host<br />
strCompName = "computer name" ' ip address may work as well<br />
Set objWMIService = GetObject("winmgmts:" _<br />
& "{impersonationLevel=impersonate}!\\" & strCompName & "\root\cimv2")<br />
Set colProcessList = objWMIService.ExecQuery _<br />
("Select * from Win32_Process Where Name = 'Notepad.exe'")<br />
For Each objProcess in colProcessList<br />
objProcess.Terminate()<br />
Next<br />
Save the code above as stopnotepad.vbs and call it from the commandline with cscript stopnotepad.vbs
You may want to check out the MSDN section on scripting, particullarly the scripting clinic and scripting guys.
|
|
|
|
|
Well, code project is great for my .NET resources.
I'm looking for a forum just as great as this, but for Java programming.
Any recommendation?
Thanks
|
|
|
|
|
http://forum.java.sun.com/
is pretty responsive, but people there seem to be a little more uptight and they tend to poke at you if you ask simple questions.
|
|
|
|
|
I use a TextBox to edit data in ListView(From a MSDN Article),but the TextBox height biger than listview ,that looks very urgly,how to make the height of ListView row equals with height of TextBox ?
|
|
|
|
|
If you use imagelist for your ListView, then you can force higher line height by having your images as large as needed.
It's a stupid workaround, but it works for me.
|
|
|
|
|
I'm trying to print a report created with Report Services, but I don't understand how it is devided into pages (ex 1/3) and how I can print all the pages, not only what fits into the first print page! it is possible to resize some how what I need to print?
//Report printing code
SHDocVw.OLECMDEXECOPT doOpt;
object o = new object();
doOpt = SHDocVw.OLECMDEXECOPT.OLECMDEXECOPT_PROMPTUSER;
axWebBrowser1.ExecWB(SHDocVw.OLECMDID.OLECMDID_PRINT , doOpt, ref o, ref o);
Thanks!!!
|
|
|
|
|
You're not making any sense. Using the IOleCommandTarget implementation - as you're doing - invokes the printing functionality in Internet Exporer, which - last time I checked - prints all pages unless the user specifies a range. Since you're specifying OLECMDEXECOPT_PROMPTUSER, they should be seeing this dialog.
So what exactly are you trying to do?
Microsoft MVP, Visual C#
My Articles
|
|
|
|
|
I am making an app where you can click on an image (in a picturebox) to get X, Y, Width, and Height values. I would like to make it draw a rectangle of the specifyed X Y Width Height values on the PictureBox/Form once selected. Is this possible?
Help is much appreciated,
~ZeldaFreak
|
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
