CRUD operation class

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(url);

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

ws.AddDirectoryGroup(SPContext.Curre

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.

StackExchange Code Review Q#97778, answer score: 6