Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

It might have been an ok-ish way of doing things... 15 years ago. This code is astonishingly similar to what one would be writing in - minus the Try..End Try block.

If you want to learn the .NET way of doing things, I strongly recommend you remove this from every VB.NET module you're writing:

Imports Microsoft.VisualBasic

Things in that namespace exist to make an easier transition between / and - pretty much everything in it has an actual -idiomatic equivalent elsewhere in the framework.

  • ADODB / ActiveX Data Objects is replaced with , itself rendered obsolete with , itself a predecessor of . But one step at a time - start with .
  • MsgBox is replaced with System.Windows.Forms.MessageBox
  • vbCrLf should have been vbNewLine, and both are replaced with Environment.NewLine

You're not including the module-scope fields, and yet the code you posted seems to use plenty... but I don't understand why. You're opening a module-scoped connection, fetching data, and then closing it. Why would that object ever need to exist beyond that? You don't need that Boolean bConnection.

ADODB being COM / unmanaged, it doens't implement the IDisposable interface. See this Stack Overflow answer this Stack Overflow answer for information about proper cleanup of COM objects... which should motivate you to use managed code ;-)

In short:

Use Marshal.ReleaseComObject to tidy up COM objects. ADODB is a COM library being used through COM Interop.


Now, what you have here is essentially a Smart UI pattern: the UI runs the show, and controls everything. That's good for learning how things work, fiddling around and prototyping things, but it's also one of the best ways to create a spaghetti mess out of production code.

Your form should only be responsible for one thing: being a user interface. Accessing a database is not the job of a UI, it's that of some data service.

You should create a class to act as your Model, that would expose a property for each value you're fetching from a database record.

Then the View (/form) wouldn't need to know about a database or connections or anything - it would simply display what the Model has in store for it. And then when the user makes changes, you can take the modified Model and pass it to the same data service that provided you with it, to write the modified values into the database.

Read up on the Model-View-Presenter pattern for more details.

It might have been an ok-ish way of doing things... 15 years ago. This code is astonishingly similar to what one would be writing in - minus the Try..End Try block.

If you want to learn the .NET way of doing things, I strongly recommend you remove this from every VB.NET module you're writing:

Imports Microsoft.VisualBasic

Things in that namespace exist to make an easier transition between / and - pretty much everything in it has an actual -idiomatic equivalent elsewhere in the framework.

  • ADODB / ActiveX Data Objects is replaced with , itself rendered obsolete with , itself a predecessor of . But one step at a time - start with .
  • MsgBox is replaced with System.Windows.Forms.MessageBox
  • vbCrLf should have been vbNewLine, and both are replaced with Environment.NewLine

You're not including the module-scope fields, and yet the code you posted seems to use plenty... but I don't understand why. You're opening a module-scoped connection, fetching data, and then closing it. Why would that object ever need to exist beyond that? You don't need that Boolean bConnection.

ADODB being COM / unmanaged, it doens't implement the IDisposable interface. See this Stack Overflow answer for information about proper cleanup of COM objects... which should motivate you to use managed code ;-)

In short:

Use Marshal.ReleaseComObject to tidy up COM objects. ADODB is a COM library being used through COM Interop.


Now, what you have here is essentially a Smart UI pattern: the UI runs the show, and controls everything. That's good for learning how things work, fiddling around and prototyping things, but it's also one of the best ways to create a spaghetti mess out of production code.

Your form should only be responsible for one thing: being a user interface. Accessing a database is not the job of a UI, it's that of some data service.

You should create a class to act as your Model, that would expose a property for each value you're fetching from a database record.

Then the View (/form) wouldn't need to know about a database or connections or anything - it would simply display what the Model has in store for it. And then when the user makes changes, you can take the modified Model and pass it to the same data service that provided you with it, to write the modified values into the database.

Read up on the Model-View-Presenter pattern for more details.

It might have been an ok-ish way of doing things... 15 years ago. This code is astonishingly similar to what one would be writing in - minus the Try..End Try block.

If you want to learn the .NET way of doing things, I strongly recommend you remove this from every VB.NET module you're writing:

Imports Microsoft.VisualBasic

Things in that namespace exist to make an easier transition between / and - pretty much everything in it has an actual -idiomatic equivalent elsewhere in the framework.

  • ADODB / ActiveX Data Objects is replaced with , itself rendered obsolete with , itself a predecessor of . But one step at a time - start with .
  • MsgBox is replaced with System.Windows.Forms.MessageBox
  • vbCrLf should have been vbNewLine, and both are replaced with Environment.NewLine

You're not including the module-scope fields, and yet the code you posted seems to use plenty... but I don't understand why. You're opening a module-scoped connection, fetching data, and then closing it. Why would that object ever need to exist beyond that? You don't need that Boolean bConnection.

ADODB being COM / unmanaged, it doens't implement the IDisposable interface. See this Stack Overflow answer for information about proper cleanup of COM objects... which should motivate you to use managed code ;-)

In short:

Use Marshal.ReleaseComObject to tidy up COM objects. ADODB is a COM library being used through COM Interop.


Now, what you have here is essentially a Smart UI pattern: the UI runs the show, and controls everything. That's good for learning how things work, fiddling around and prototyping things, but it's also one of the best ways to create a spaghetti mess out of production code.

Your form should only be responsible for one thing: being a user interface. Accessing a database is not the job of a UI, it's that of some data service.

You should create a class to act as your Model, that would expose a property for each value you're fetching from a database record.

Then the View (/form) wouldn't need to know about a database or connections or anything - it would simply display what the Model has in store for it. And then when the user makes changes, you can take the modified Model and pass it to the same data service that provided you with it, to write the modified values into the database.

Read up on the Model-View-Presenter pattern for more details.

added 288 characters in body
Source Link
Mathieu Guindon
  • 75.5k
  • 18
  • 194
  • 467

It might have been an ok-ish way of doing things... 15 years ago. This code is astonishingly similar to what one would be writing in - minus the Try..End Try block.

If you want to learn the .NET way of doing things, I strongly recommend you remove this from every VB.NET module you're writing:

Imports Microsoft.VisualBasic

Things in that namespace exist to make an easier transition between / and - pretty much everything in it has an actual -idiomatic equivalent elsewhere in the framework.

  • ADODB / ActiveX Data Objects is replaced with , itself rendered obsolete with , itself a predecessor of . But one step at a time - start with .
  • MsgBox is replaced with System.Windows.Forms.MessageBox
  • vbCrLf should have been vbNewLine, and both are replaced with Environment.NewLine

You're not including the module-scope fields, and yet the code you posted seems to use plenty... but I don't understand why. You're opening a module-scoped connection, fetching data, and then closing it. Why would that object ever need to exist beyond that? You don't need that Boolean bConnection.

Even with ADODB connections and commandsbeing COM / unmanaged, which AFAIK don'tit doens't implement the IDisposable interface that every .NET data access framework I know See this Stack Overflow answer for information about proper cleanup of does, you should scope yourCOM objects as tightly as possible... which should motivate you to use managed code ;-)

In short:

Use Marshal.ReleaseComObject to tidy up COM objects. ADODB is a COM library being used through COM Interop.


Now, what you have here is essentially a Smart UI pattern: the UI runs the show, and controls everything. That's good for learning how things work, fiddling around and prototyping things, but it's also one of the best ways to create a spaghetti mess out of production code.

Your form should only be responsible for one thing: being a user interface. Accessing a database is not the job of a UI, it's that of some data service.

You should create a class to act as your Model, that would expose a property for each value you're fetching from a database record.

Then the View (/form) wouldn't need to know about a database or connections or anything - it would simply display what the Model has in store for it. And then when the user makes changes, you can take the modified Model and pass it to the same data service that provided you with it, to write the modified values into the database.

Read up on the Model-View-Presenter pattern for more details.

It might have been an ok-ish way of doing things... 15 years ago. This code is astonishingly similar to what one would be writing in - minus the Try..End Try block.

If you want to learn the .NET way of doing things, I strongly recommend you remove this from every VB.NET module you're writing:

Imports Microsoft.VisualBasic

Things in that namespace exist to make an easier transition between / and - pretty much everything in it has an actual -idiomatic equivalent elsewhere in the framework.

  • ADODB / ActiveX Data Objects is replaced with , itself rendered obsolete with , itself a predecessor of . But one step at a time - start with .
  • MsgBox is replaced with System.Windows.Forms.MessageBox
  • vbCrLf should have been vbNewLine, and both are replaced with Environment.NewLine

You're not including the module-scope fields, and yet the code you posted seems to use plenty... but I don't understand why. You're opening a module-scoped connection, fetching data, and then closing it. Why would that object ever need to exist beyond that? You don't need that Boolean bConnection.

Even with ADODB connections and commands, which AFAIK don't implement the IDisposable interface that every .NET data access framework I know of does, you should scope your objects as tightly as possible.


Now, what you have here is essentially a Smart UI pattern: the UI runs the show, and controls everything. That's good for learning how things work, fiddling around and prototyping things, but it's also one of the best ways to create a spaghetti mess out of production code.

Your form should only be responsible for one thing: being a user interface. Accessing a database is not the job of a UI, it's that of some data service.

You should create a class to act as your Model, that would expose a property for each value you're fetching from a database record.

Then the View (/form) wouldn't need to know about a database or connections or anything - it would simply display what the Model has in store for it. And then when the user makes changes, you can take the modified Model and pass it to the same data service that provided you with it, to write the modified values into the database.

Read up on the Model-View-Presenter pattern for more details.

It might have been an ok-ish way of doing things... 15 years ago. This code is astonishingly similar to what one would be writing in - minus the Try..End Try block.

If you want to learn the .NET way of doing things, I strongly recommend you remove this from every VB.NET module you're writing:

Imports Microsoft.VisualBasic

Things in that namespace exist to make an easier transition between / and - pretty much everything in it has an actual -idiomatic equivalent elsewhere in the framework.

  • ADODB / ActiveX Data Objects is replaced with , itself rendered obsolete with , itself a predecessor of . But one step at a time - start with .
  • MsgBox is replaced with System.Windows.Forms.MessageBox
  • vbCrLf should have been vbNewLine, and both are replaced with Environment.NewLine

You're not including the module-scope fields, and yet the code you posted seems to use plenty... but I don't understand why. You're opening a module-scoped connection, fetching data, and then closing it. Why would that object ever need to exist beyond that? You don't need that Boolean bConnection.

ADODB being COM / unmanaged, it doens't implement the IDisposable interface. See this Stack Overflow answer for information about proper cleanup of COM objects... which should motivate you to use managed code ;-)

In short:

Use Marshal.ReleaseComObject to tidy up COM objects. ADODB is a COM library being used through COM Interop.


Now, what you have here is essentially a Smart UI pattern: the UI runs the show, and controls everything. That's good for learning how things work, fiddling around and prototyping things, but it's also one of the best ways to create a spaghetti mess out of production code.

Your form should only be responsible for one thing: being a user interface. Accessing a database is not the job of a UI, it's that of some data service.

You should create a class to act as your Model, that would expose a property for each value you're fetching from a database record.

Then the View (/form) wouldn't need to know about a database or connections or anything - it would simply display what the Model has in store for it. And then when the user makes changes, you can take the modified Model and pass it to the same data service that provided you with it, to write the modified values into the database.

Read up on the Model-View-Presenter pattern for more details.

Source Link
Mathieu Guindon
  • 75.5k
  • 18
  • 194
  • 467

It might have been an ok-ish way of doing things... 15 years ago. This code is astonishingly similar to what one would be writing in - minus the Try..End Try block.

If you want to learn the .NET way of doing things, I strongly recommend you remove this from every VB.NET module you're writing:

Imports Microsoft.VisualBasic

Things in that namespace exist to make an easier transition between / and - pretty much everything in it has an actual -idiomatic equivalent elsewhere in the framework.

  • ADODB / ActiveX Data Objects is replaced with , itself rendered obsolete with , itself a predecessor of . But one step at a time - start with .
  • MsgBox is replaced with System.Windows.Forms.MessageBox
  • vbCrLf should have been vbNewLine, and both are replaced with Environment.NewLine

You're not including the module-scope fields, and yet the code you posted seems to use plenty... but I don't understand why. You're opening a module-scoped connection, fetching data, and then closing it. Why would that object ever need to exist beyond that? You don't need that Boolean bConnection.

Even with ADODB connections and commands, which AFAIK don't implement the IDisposable interface that every .NET data access framework I know of does, you should scope your objects as tightly as possible.


Now, what you have here is essentially a Smart UI pattern: the UI runs the show, and controls everything. That's good for learning how things work, fiddling around and prototyping things, but it's also one of the best ways to create a spaghetti mess out of production code.

Your form should only be responsible for one thing: being a user interface. Accessing a database is not the job of a UI, it's that of some data service.

You should create a class to act as your Model, that would expose a property for each value you're fetching from a database record.

Then the View (/form) wouldn't need to know about a database or connections or anything - it would simply display what the Model has in store for it. And then when the user makes changes, you can take the modified Model and pass it to the same data service that provided you with it, to write the modified values into the database.

Read up on the Model-View-Presenter pattern for more details.

lang-vb

AltStyle によって変換されたページ (->オリジナル) /