DOOM团队代码复审报告
DOOM团队代码复审报告
1 代码规范性
1.1 变量命名规范性
在所有的类中，所有的变量都按照“驼峰大小写”的命名规范，且变量名称基本为变量意义的英文缩写，同时，重要的变量附有注释，方便理解。
如下面的截图：
1.2 函数命名规范性
Doom团队的代码，类的所有方法，采用的也是首字母为小写的驼峰大小写的规则，但根据C#的编码规范，公有方法，最好首字母大写，私有成员，首字母小写。例如如下的代码：
这是一个被‘public static’修饰的方法，根据C#编码规范，最好将其命名为DeleteLog
1.3 sql语句书写规范性
所用的sql语句都采用了“关键词大写，其他量小写”的sql语句书写规范，合乎标准。
2 代码可读性
在Doom组提交给我们的代码片段里，AntiAbuse类的所有公有方法都做了详细的注释。而且对函数签名的注释采用VS的注释模板，这样可以被Intellisense识别，方便进行函数调用时自动提示各个形参的意义。
同时，在函数体内，有对于方法意义的足够注释
美中不足的是，在提交给我们的所有代码中，只对有AntiAbuse的类做的注释比较详尽，而其他类的注释较少。
3 具体代码部分
3.1 错误处理
在AntiAbuse类里，所有的数据库访问都没有进行相应的错误处理，如下图所示的这一方法。
这里使用了Using语句包围Connection，是一种很好的习惯，但是，对于connection.Open()方法可能抛出的异常（数据库连接失败等），并没有进行相应的处理。SqlCommand cmd的相应方法也是这样。
3.2 代码书写的统一性
在复审过程中，我们发现了一个问题，在数据库的操作逻辑上，有的代码使用的using方法（如上图），有的则是调用的DbHelper类里的相关方法。
这样，对数据库的操作的编码并不统一，在进行修改的时候可能会造成混乱，建议统一采用一种格式。最好能够统一使用DbHelper类提供的封装好的方法对数据库进行操作。
3.3 重复代码
在复审过程中，我们发现了许多重复的代码，比如，在每一个对数据库进行操作的方法上，都硬编码了connectionString。
这样做，一旦connectionString需要进行修改，则需要动到源代码，更何况connectionString在代码中出现了许多次，改起来会十分的麻烦，在这里，还是建议将connectionString从代码中剥离出来，保存在xml文件中，并通过特定的类，如DbHelper，在runtime将其获取，这样做，使得程序与自身所在的运行环境（不同的环境需要不同的数据库连接字符串）解耦度更高。
4 效能问题（关于部分代码的算法改进）
在AntiAbuse类中，有一个关键的函数isSensitiveWords(string content)，其作用是判断content里是否包含系统内设置的敏感词。为了便于分析，在此贴上其代码。
/// <summary>
/// 判断一个回答或者评论是否为敏感词
/// </summary>
/// <param name="content">回答内容或评论内容</param>
/// <returns>是敏感词就返回true，否则返回false</returns>
protected static Boolean isSensitiveWords(string content) {
/*
*按数据库里的敏感词表中的敏感词对content进行分析
*设计一个算法判断content是否为敏感的
*如果是敏感的，就返回true，否则返回false
*/
Boolean isSensitiveWords = false; //是否为敏感词
/*从数据库中读出所有的敏感词到一个hash表里面*/
SqlConnection conn = DbHelper.getConnection();
HashSet<String> senWordsSet = new HashSet<string>();
SqlDataReader reader = DbHelper.ExecuteReader(
"SELECT * FROM [SensitiveWords]"
, conn
);
while (reader.Read()) {
senWordsSet.Add(reader.GetString(1));
}
/*判断content是否为敏感的算法实现*/
int contentLen=content.Length; //content的长度
int start = 0; //子字符串的开始位置
int step = 10; //步长
if (step > contentLen) { //如果步长大于content的长度，调整步长
step = contentLen;
}
while (start < contentLen) {
string word;
if ((step = (contentLen - start)) >= 10) { //设置步长step
step = 10;
}
word = content.Substring(start, step);
while (step > 0) {
if (senWordsSet.Contains(word)) { //如果word是敏感词，那么说明content是敏感的
isSensitiveWords = true;
}
step--;
word = content.Substring(start, step);
}
if (step == 0)
{
start++;
}
else {
start += step;
}
}
return isSensitiveWords;
}
可以预见，在生产环境下，该方法的调用次数非常高（每当产生一个User Generated Content就调用一次），然而，在该方法的逻辑内，可以看到，每调用一次该方法，就要从数据库里读取一次senWordsSet（敏感词列表），而在生产环境下，该列表的变动可能比较小，因此可以将senWordsSet变为AntiAbuse类里的一个静态数据成员，在类初始化时，也将其初始化。但考虑到这里的isSensitiveWords(string content)是静态型的，需要在使用senWordList之前判断其是否为null，如果是则加载，这样就避免了不必要的数据库IO开销。
另外，再来看它的关键逻辑：找敏感词，其大致思路是——把content分成n段，每一段使用string的isContains方法检测，由于不清楚isContains（）的具体算法，我对该方法的效率还持有怀疑。如果不放心其效率，可以自己编码实现KSP算法。另外，考虑到很多垃圾评论都非常短，而且严重同质化，如“好”，“顶”，“楼主好人”等，我们可以再按照content的长度分类，如果content较短，就让它跟小于自己长度的敏感词比较，提高效率。
另外，在这里，不知敏感词是否有英文，如果有，则应该先进行大小写转换，方便比较。
另外，可以参考博文
5. 设计模式
该小组的代码没有采用任何设计模式，而且大量的方法都是static的，static有static的好处，特别是对于只专注于业务逻辑的代码，但建议采用某些设计模式，如简单工厂模式，比如上面的senWordsSet，可以放在工厂初始化的业务逻辑里，一次初始化即可。
综合评分，经过权衡各方面因素，我们为DOOM提交的代码打分为：92
