CRUD操作类

Question

这段代码是一个大项目，所以我很抱歉没有添加所有的东西，因为这是不可能的。

[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);
    }
}

这个代码在一个实用程序项目中，它从WCF服务中调用来与Companionships交互，它使用一些业务逻辑将它们添加到数据库中，并向另一个web服务发送一条消息。

从结构上看，我认为这是可以改进的。

我这样称呼它：

 new Companionship().AddCollection(pCPs);

我更担心我与Companionship类交互的方式。

Answer 1

命名

我不会列出每一个坏名字，但你需要更仔细地考虑你选择的名字。

ID应该是Id 微软在这方面有非常具体的指导方针。。

可以在标识符中使用的两个缩写是ID和OK。在基于Pascal大小写的标识符中，它们应该显示为Id，并显示为Ok。如果作为骆驼大小写标识符中的第一个单词，它们应该分别以id和ok的形式出现。

现在再来说说你们的一些名字：

public int Add(Companionship pC) - pC是什么？请给我全名。我建议一个，但实际上我还没有弄清楚这意味着什么。

再说一遍var gda = new USDataAccess(); - gda是什么？我不能再提建议了..。

new gDataAccess() -类型应该是Pascal大小写，而不是以前缀开头。

dtEnd --不要在名称中缩写-- endDate的可读性要高得多。

cDetails --难道companyDetails不是更容易阅读吗？

老实说，你80%的名字都不太理想--这是一件很容易修正的事情！

怪异

这在我看来很奇怪：

new Company().Get(clientID)

我希望能在没有Company的情况下通过id获得一家公司

Company.GetByClientId(clientId);

注意，对于另一个类--例如CompanyRepository --这会更好。

这究竟是做什么的：

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

您不使用结果，所以我只能假设它在某个地方持续存在--这在代码中根本不明显。

解析

这类代码：

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

是脆弱的，你可能有一个字符串，但这并不意味着这是一个有效的日期。您还应该指定区域性或模式。

其他事情

你为什么要使用公共字段？做他们的财产。

为什么你要从你的方法中返回1？这是C#，您不需要返回退出代码。如果出现问题，如果不需要返回值，则抛出异常，将方法的返回类型设置为void。

Answer 2

有许多方法可以改进该代码：

1. 您有一个存储大量信息(id、日期等)的类。您有一个类对该数据执行操作。目前，他们是一个阶级，当他们应该是两个(单一责任原则)。
2. 几乎所有变量和参数的命名都是不可理解的。pC是什么？即使是Companionship也有点模糊(尽管代码意味着它在更广泛的上下文中有着独特的意义，因此可能很好)。代码应该先被人读取，然后才能被编译器读取，所以变量名应该得到改进，比如pCs变成companions或CompanionCollection之类的)。
3. 该代码与其他类(如USDataAccess )紧密耦合，直接在其内部创建这些类的实例。所有这些都应该通过接口引用并通过构造函数注入到实例中。
4. 它有一个硬编码的URL；对AD的硬编码访问等等，使得孤立的测试变得非常困难，如果不是不可能的话。应该再一次注射这些药物。
5. AddCollection()和Add()编写时就好像它们是静态方法一样(既不使用它们实例的字段)，又通过new Companionship()...调用。因为代码有副作用，所以它们不应该是静态的。但如前所述，它们的所有依赖项都是通过构造函数注入的。这些依赖项也应该注入到当前调用new Companionship().AddCollection(pCPs);的任何代码中。