11
\$\begingroup\$

Here's a class I'm designing:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Web;
using System.Data;
using System.Data.SqlClient;

namespace Artworking.classes
{

    // A group permission level
    public class PermissionGroup
    {
        public int ID;                  // Unique ID for this group
        public bool hasFullControl;     // Does this group have complete control
        public user creator;            // Who created this group
        public string groupName;        // Reference name for group

        // Constructor for when a permission group ID is passed
        public PermissionGroup(SqlConnection cn, int ID)
        {
            using (SqlCommand cmd = new SqlCommand("SELECT creatorUserID, group_name, fullControl FROM tblATPermissionGroups WHERE ID = " + ID, cn))
            {
                SqlDataReader rdr = cmd.ExecuteReader();
                if (rdr.Read())
                {
                    this.creator.ID = int.Parse(rdr["creatorUserID"].ToString());
                    this.groupName = rdr["group_name"].ToString();
                    this.hasFullControl = bool.Parse(rdr["fullControl"].ToString());
                }
                rdr.Close();
            }
        }
    }

}

Am I on the right track? Please note I'm not using the built in authentication as it needs to be backwards compatible for an old system. What I'm just checking is that I'm handling these classes properly and loading the data in correctly etc. As I understand it I'm meant to put my SQL commands in the classes and away from the actual pages right?

\$\endgroup\$
  • \$\begingroup\$ Comments are not bad, but why not go ahead and use the XML comments (msdn.microsoft.com/en-us/magazine/cc302121.aspx) and let them flow back through IntelliSense®? \$\endgroup\$ – Larry Smithmier Jan 29 '11 at 21:07
  • \$\begingroup\$ Thanks LArry, didn't know about XML comments, but for my project it would be overkill :) \$\endgroup\$ – Tom Gullen Jan 29 '11 at 22:43
11
\$\begingroup\$

It is generally not a good idea to do this type of processing in the .ctor. Move it to a Load (or some such name) method. This however means your object is not really loaded after instantiated which is another issue. As such, I would recommend separating the entity information (HasFullControl, Creator, GroupName, etc) type from the type that loads the information from the database. You could change PermissionGroup type to just have the data properties and then move the loading logic to a data layer that is responsible for creating the instance a loading the data.

Concatenating text to the end of a sql string leaves you open to Sql Injection attacks (though not in this specific case). Use a parameter instead:

using (SqlCommand cmd = new SqlCommand("SELECT creatorUserID, group_name, 
    fullControl FROM tblATPermissionGroups WHERE ID = @id", cn))
{
    cmd.Parameters.AddWithValue("@id", id);
    ...
}

You need to wrap the SqlDataReader in a using block.

Use public readonly properties rather than public fields.

Consider using a IDbConnection rather than SqlConnection. Doing so will allow you to more easily mock the data call when testing this method and makes it easier to support other RDBMS's should the need arise.

Throw helpful exceptions if something goes wrong when populating the data fields.

Follow the .Net Framework naming guidelines

| improve this answer | |
\$\endgroup\$
6
\$\begingroup\$

Consider C# accepted practices. As other answers mention, you have member fields marked public. In C#, you typically expose values as properties and keep members private. Starting with C# 3, you do not even need to explicitly create member fields to back your properties unless getters and setters are non-trivial. So consider replacing your fields with

public int ID { get; private set; }
public bool HasFullControl { get; private set; }

Note that I added private to the setter. This is on the assumption that you do not want code external to your class setting these values, overriding your permissions, as it were. Feel free to remove that modifier if that is not the case.

Note also that C# practices dictate that publicly visible names are Pascal cased instead of your camel casing. Capitalize the first character of your namespace (including nested namespaces), classes, properties, methods, etc.

@akmad has addressed an additional set of concerns, such as the constructor doing expensive work and the vulnerability to SQL injection in a general sense (though not specifically here, as akmad also points out). If you need to do a database call to construct your object, consider a factory approach.

public class PermissionGroup
{
    private PermissionGroup() { } // class cannot be constructed from outside

    public static PermissionGroup GetPermissionGroup(/* your parameters */)
    {
        // build the object starting here!
    }
}
| improve this answer | |
\$\endgroup\$
0
\$\begingroup\$

I would agree with Mark: I'd make the members private, and then add property accessors to them. You can then control access if you need to now or in the future.

For example, you might want to make one of the members read-only. Easy to do: implement the get() property but not the set() property.

Similarly, data range checking can easily be added to the set().

| improve this answer | |
\$\endgroup\$
0
\$\begingroup\$

I know nothing about C#, so this is just a comment based on the design. Is there any reason those members aren't private? Why don't you provide accessors, or (as I've said in many comments on this site already) actually move behavior into this class rather than exposing each individual data member? For example, this is a permission group, have a function called "isAuthorized" to which you can pass a command to find out if this group is allowed to do that thing. This way when you move forward, you can make the permissions possibly more fine grained and no one else has to even know that consumes this class.

| improve this answer | |
\$\endgroup\$
0
\$\begingroup\$

I would split up the Read operation to an repository and an domain object that holds the data. I would also send in an unit of work handler into the repository which handles the dbconnection and transaction

public class PermissionGroupRepository
{
    public PermissionGroupRepository(IUnitOfWork unitOfWork)
    { }
    public PermissionGroup GetPermissionGroup(/* your parameters */)
    { }
}


public class PermissionGroup
{
     /* Properties */
}
| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.