Skip to main content
replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

A very good answer about the usage of comments can be found here: http://codereview.stackexchange.com/a/90113/29371https://codereview.stackexchange.com/a/90113/29371

A very good answer about the usage of comments can be found here: http://codereview.stackexchange.com/a/90113/29371

A very good answer about the usage of comments can be found here: https://codereview.stackexchange.com/a/90113/29371

replaced http://programmers.stackexchange.com/ with https://softwareengineering.stackexchange.com/
Source Link

A very good answer about the usage of regions can be found here: http://programmers.stackexchange.com/a/53114https://softwareengineering.stackexchange.com/a/53114

A very good answer about the usage of regions can be found here: http://programmers.stackexchange.com/a/53114

A very good answer about the usage of regions can be found here: https://softwareengineering.stackexchange.com/a/53114

Source Link
Heslacher
  • 51k
  • 5
  • 83
  • 177

Comments

You have a lot of useless comments in your code which should be deleted because thy don't add any value to the code but reduce the readability.

A very good answer about the usage of comments can be found here: http://codereview.stackexchange.com/a/90113/29371

Bottom line, though, is that the comments in your code should fill in the blanks that your code does not. In addition, your comments should give details on the motivation, and not the application of your code. You should, in general, comment only on why your code does things, not what your code is doing.


Regions

Using regions is mostly a code smell and should be avoided.

A very good answer about the usage of regions can be found here: http://programmers.stackexchange.com/a/53114

The reviewed code also contained a lot of regions grouping all the fields together, all the properties together, etc. This had an obvious problem: source code growth.

When you open a file and see a huge list of fields, you are more inclined to refactor the class first, then work with code. With regions, you take an habit of collapsing stuff and forgetting about it.

Another problem is that if you do it everywhere, you'll find yourself creating one-block regions, which doesn't make any sense. This was actually the case in the code I reviewed, where there were lots of #region Constructor containing one constructor.


Constructor

Currently you have duplicated code in your constructors which can be reduced by using constructor chaining. Constructor chaining is an approach where a constructor calls another constructor in the same or base class.

This would lead to

private Cat_Table(bool isUpdate)
{
    SetConnectString();
    m_IsUpdate = isUpdate;
}

public Cat_Table()
    : this(false)
{ }

// This is the default constructor called when passing a primary key.
// It basically does the same thing as the empty constructor, but it also
// loads the record passed and fills the instances variables
public Cat_Table(int pkPrimaryKey)
    : this(true)
{
    this.Get(pkPrimaryKey);
}

// This constructor is the default constructor called when
// a datarow is passed. It loads the member variables with the row data
public Cat_Table(DataRow dtrRow)
    : this(true)
{
    this.Fill(dtrRow);
}  

Instead of using "default" comments you should consider to use correct xml documentation for commenting the purpose of the methods. In this way you will see the comments in intellisense too.

So this

// This is the default constructor called when passing a primary key.
// It basically does the same thing as the empty constructor, but it also
// loads the record passed and fills the instances variables
public Cat_Table(int pkPrimaryKey)  

should look like

/// <summary>
/// This is the default constructor called when passing a primary key....
/// </summary>
/// <param name="pkPrimaryKey">The primary key to fill the instance variables</param>
public Cat_Table(int pkPrimaryKey)  

The usage of the m_IsDirty flag can be optimized. Right now you don't check wether the new value is different from the current value. If the value which is set is the same, then you shouldn't need to set this flag and therefor you won't need to save the current row either.


protected void SetConnectString()  

this does not do what the method name implies. It does not Set the connection string but read it from a file. You should consider to rename it to e.g FillConnectionString() or ReadConnectionString().