Code Review Checklist (will be continuously updated)

                 Code Review Checklist (will be continuously updated)


1) Naming convention followed
2) Namespaces not left with default name (should start with <CompanyName> or <DepartmentName>)
3) Logging implemented using EntLib or log4net. Info (for tracing) and Error should be logged. Note - Also log the input params and output result esp. when you are consuming other party's service. There will be substantial evidence during a Production issue to know if the other party's webservice misbehaved or UI passed the wrong input. Logging datetime should be done on UTC.
Log exception using ex.ToString() for all details. Also check if ex.InnerException is not null, if that is the case, log that as well. InnerException can nested upto many levels. The best way is -
while (e.InnerException != null) e = e.InnerException;

 For catching Entity framework exceptions, refer - http://lazynreclined.blogspot.in/2016/02/exception-handling-while-using-entity.html

4) Separate web project created for deployment (applicable for WCF projects only)
5) Unit test cases for the component present
6) Throw exception instead of returing some kind of status codes.
7) Throw most specific exception (ex: ArgumentNullException) instead of ArgumentException or Exception
8) Catch exceptions in sequence from most specific to most general one.
9) All disposable objects like DbContext, Stream etc and unmanaged objects disposed using "Dispose pattern" (https://msdn.microsoft.com/en-us/library/fs2xkftw(v=vs.110).aspx
  To find all the classes which are not disposed correctly, enable rule CA2000 using Code Analysis in Visual Studio.). For WCF clients - refer http://lazynreclined.blogspot.in/2016/02/disposing-wcf-clients.html
10) All configurable items like URL, SMTP server, Refresh intervals etc are picked from config file.
11) WebDeploy SetParameter file and Parameters.xml file added/updated (if applicable).
12) String comparision is done using String.Compare and not "==" and is case-insensitive.
13) String concatenation is done using String.Concat or StringBuilder and not using "+".
14) object checked for null after any operation before passing it further.
15) Source file is named same as that of the type. (class name).
16) There are no errors in the browser console (in Developer tools) to rule out enable any javascript issue.
17) Wherever TimeStamp column is present, for recording any Request or Response, it should be "TimeStampUTC".
18) Whenever there is a need to transfer messages between different systems, ServiceBus should be used for fault tolerance and asynchronous behavior.
19) Any freshly build application should be load-tested (if you suspect there can be load), using a tool (JMeter).
20) Whenever connecting to database, make sure you specify the min pool, max pool (for performance reasons), connect timeout and command timeout. Timeouts should also be specified for Webservices.


21) When dataaccess is done (esp Insert/update) using EntityFramework, make sure UnitOfWork pattern is used. UnitOfWork pattern makes sure that the same context is shared across all repositories (tables) and the SaveChanges operation is atomic, if any operation requires changing more than one repository. This was not needed in old-school DAL as in such cases, a stored procedure would be used.

22) Transactions are maintained using TransactionScope class. If it is a WCF Service, [TransactionFlow] attribute is correctly used.

23) In case of queue-based solution, how are poison messages handled ? In case of data-transfer job, how are you making sure - the same record doesn't get picked up again because of an exception (due to bad-data). In such case, make sure you have a field to mark the state as "Processed-but-failed" so that it does not get picked up again.

24) Are you using unmanaged code in your windows service (say Quartz). Any exception from unmanaged code will not be caught in System.Exception and will crash/stop the service. An example is - iDB2DCFunctionErrorException from iSeries library. You can have a separate service whose only responsibility will be to monitor the first service and starts it if it stops. Do not add ANYTHING EXTRA on the second service. #SingleResponsiblityPrinciple.