CRUD operation class

Question

This code is a chunk of big project, so I am sorry in advance for not adding everything as it's just not possible.

[DataContract]
public class Companionship
{
    [DataMember]
    public int ID;
    [DataMember]
    public string ClientID;
    [DataMember]
    public string UserSID;
    [DataMember]
    public string Role;
    [DataMember]
    public string Status;
    [DataMember]
    public string StartDate;
    [DataMember]
    public string EndDate;
    [DataMember]
    public bool AddedInBP;

    public int Add(Companionship pC)
    {
        int clientID = -1;
        var gda = new USDataAccess();
        if (string.IsNullOrEmpty(pC.ClientID))
            clientID = new gDataAccess().GetClientByURL(SPContext.Current.Web.Site.Url).ClientID;
        else
            clientID = Convert.ToInt32(pC.ClientID);

        pC.Status = "Active";
        DateTime? dtEnd = string.IsNullOrEmpty(pC.EndDate) ? (DateTime?)null : DateTime.Parse(pC.EndDate);
        DateTime dtStart = string.IsNullOrEmpty(pC.StartDate) ? DateTime.Now : DateTime.Parse(pC.StartDate);
        gda.AddCompanionship(clientID, pC.UserSID, pC.Role, pC.Status, dtStart, dtEnd);
        new DirectoryGroup().Add(clientID, pC.UserSID, "ChangeMe In Companionship");

        var cDetails = new Company().Get(clientID);
        if (cDetails != null && !string.IsNullOrEmpty(cDetails.CompanyID) && pC.AddedInBP)
        {
            SendApprovalRequest();
        }

        return 1;
    }

    public int AddCollection(Companionship[] pCs)
    {
        foreach (var c in pCs)
            this.Add(c);

        return 1;
    }



    private void SendApprovalRequest()
    {
        string url = "https://portal.gov.com/CompanyManagement/_vti_bin/sync/sync.svc";
        ICSync ws = Utils.GetProxy<ICSync>(url);

        var user = Utils.ADUserToObject(ADUser.LoadBySid(this.UserSID));
        user.Companionships[0] = this;
        var dd = Utils.UserToDirectoryGroupDetails(user);

        ws.AddDirectoryGroup(SPContext.Current.Web.Site.Url, SPContext.Current.Web.CurrentUser.Email, dd);
    }
}

This code is in a utilities project, it's called from WCF services to interact with Companionships, it adds them to the database with some business logic and sends a message to another web service.

Looking at the structure I think this can be improved.

I am calling it like this:

 new Companionship().AddCollection(pCPs);

I am more worried about the way I am interacting with Companionship class.

Answer 1

Naming

I won't list every bad name but you need to think more carefully about the names you choose.

ID should be Id Microsoft have very specific guidelines for this.

The two abbreviations that can be used in identifiers are ID and OK. In Pascal-cased identifiers they should appear as Id, and Ok. If used as the first word in a camel-cased identifier, they should appear as id and ok, respectively.

Now on to some more of your names in general:

public int Add(Companionship pC) - What is pC? Full name please. I'd suggest one but I actually haven't been able to figure out what it might mean.

var gda = new USDataAccess(); Again - What is gda? I can't make a suggestion again...

new gDataAccess() - Types should be Pascal case and not start with a prefix.

dtEnd - Don't abbreviate in names - endDate is much more readable.

cDetails - wouldn't companyDetails be easier to read?

To be honest, 80% of your names are less than ideal - it's an easy thing to fix though!

Weirdness

This looks very odd to me:

new Company().Get(clientID)

I'd expect to be able to get a company by id without newing up a Company

Company.GetByClientId(clientId);

Note that would be even better on another class - e.g. CompanyRepository.

---

What on earth does this do:

new DirectoryGroup().Add(clientID, pC.UserSID, "ChangeMe In Companionship");

You don't use the result so I can only assume this persists something somewhere - that's not obvious at all from the code.

Parsing

This sort of code:

DateTime? dtEnd = string.IsNullOrEmpty(pC.EndDate) ? (DateTime?)null : DateTime.Parse(pC.EndDate);

Is fragile, you might have a string but that's no gauranteee that it's a valid date. You should also specify a culture or a pattern.

Other things

Why are you using public fields? Make them properties.

Why are you returning 1 from your methods? It's C# you don't need to return an exit code. Throw an exception if something goes wrong and if you don't need a return value, set the return type of the method as void.

Answer 2

There are numerous ways in which this code could be improved:

1. You have a class that stores a bunch of information (id, dates etc). You have a class that performs actions upon that data. At the moment they are one class, when they should be two (Single responsibility principle).
2. The naming of just about every variable and parameter is incomprehensible. What is pC? Even Companionship is a bit vague (though the code implies it has a distinct meaning in a wider context and thus may be fine). Code should be written to be read by people first, the compiler second, so the variable names should be improved, eg pCs becomes companions or CompanionCollection or some such).
3. The code is very tightly coupled to other classes, eg USDataAccess, with it directly creating instances of those classes within itself. All should be referenced via interfaces and injected in to the instance via a constructor.
4. It has a hardcoded URL in it; hardcoded access to AD etc, making isolated testing ridiculously difficult, if not impossible. Once again these should be injected.
5. AddCollection() and Add() are written as if they were static methods (neither make use of the fields of their instance) yet invoked via new Companionship().... Since the code has side effects, they shouldn't be static. As previously said though, all their dependencies are injected via a constructor. Those dependencies should also be injected into any code currently invoking new Companionship().AddCollection(pCPs);