Code Review .net
Maria Mauri
I think with git it would be far easier. I saw that there is Git Publisher in post-build actions and I could push changes to branch - "review-branch" or something like that and send notification to reviewers. They will pull repository and everything would be great. hmm also maybe instead of pushing to review branch, Jenkins should create pull request to that branch. But as I said it would be scenario for git. I wrote it here, because for me git is better option here, but maybe not possible. I just want to know if I'm not missing something if we are talking about git.
Review:What is the best approach?- (GIT) Jenkins creates pull request -- if possible- (GIT) Jenkins pushes to second review-branch and sends notification on email or creates some tickets on some issue tracker (Redmine/Jira)- (SVN) Jenkins commits to review repository and sends email or creates tickets.- or maybe it's better to integrate everything with ReviewBoard/Gerrit or something similar. Right now I've never used such software and I read that Gerrit is a bit troublesome.
That means you can do a code review on committed code, and fix any issues in a new commit. In fact, this is actually good to do with Subversion because Subversion gives you a nice revision number that contains all the code changes. Everyone is on the same page this way. ("We're reviewing revision 23,340"). Most code reviews are looking for things like forgetting to check for a null pointer, etc. If the code is really bad, Subversion makes it easy to completely reverse the change.
Jenkins will immediately tell you if a build is bad, and our policy is that a developer has 10 minutes to fix the problem, or back out their change. (90% of the time, they back out their change). We have our system setup, so both the developer and Jenkins use the same build mechanism. This allows a developer to test the build and run the unit tests before committing their changes.
This is why most version control systems don't have shelving, and you don't see three sets of repositories and most places. This isn't your apartment. Your mom isn't going to come by and see your code history. It doesn't have to be nice and tidy. Let it reflect what's going on.
The Promoted Builds Simple Plugin is ...well... simple. You specify all of the various promotion levels in your Jenkins configuration page, and that's what all the builds will use. Only a single promotion level per individual build is allowed, and it's all manually done.
The standard Promoted Build Plugin is a bit more work, but it's also way more powerful. It allows each Jenkins job to have its own build promotion scheme. Promotions can be both manual and automatic. When a build gets promoted, it can spawn other actions such as sending out email, deploying to a server, etc. Plus, you can show multiple promotions. Imagine the following promotion levels:
If using SVN, check out the Jenkins SVN Publisher Plugin. For GIT, it looks like it might be possible with the Git Plugin, but there is this little warning: "Installing the plugin itself works like a charm but configuring the system to work properly under Windows can be a be tricky. Lets see the problems you may run into."
At any rate, our Business Intelligence developers don't really do the same type of coding/development that we do for websites, windows apps, etc. Our templates/checklists don't really apply to them. Since I've been the one trying to evangelize and get everyone do start doing peer reviews, they asked me for advice on what types of things they should be looking for.
I could probably guide them and help them to develop their own checklists by asking questions about what can go wrong, what is important to them, etc., but I thought I'd ask and see if anyone is aware of established guidelines for doing "code reviews" for BI developers.
Code clarity -Naming conventions are there in BI ETL development too for different objects used in the code.*Security -Not applicable at a code review point of view in BI applications. Security focus is more in the design phase (which users need to see which sub-section of the data) and inthe administration side (reporting tool user log-in configuration) etc.
Functionality checks-I think this comes under the unit testing phase or other testing phases - less to do here at the code review time, except for checking if there are unit test plans and test results available as part of the review.
Make suggestions, not commands. Dictating instead of discussing will limit your communication opportunities considerably, so choosing requests over commands is much more preferable for the success of your review. Consider two phrases to feel the difference (in the second case, a much more productive approach is applied as it creates the channel of open communication):
'I would have done it differently.'
'Could we find another way to do it?'
The best way to make holistic changes is to try to move all the comments into the code, thus making them visible within the code.
When reviewers start making their comments and identifying issues, developers that are responsible for the branch can start either working to fix them or question the necessity for the change. In the latter case, the reviewer must be able to clearly articulate the reason for the change. 'Apply action filters methods to make them accessible only for authenticated users' is a good example of the change and explanation for its necessity.
Code review is a critical part of building a high-quality software developers' workflow. By examining source code systematically, a reviewer learns how to introduce changes efficiently. Thus, it facilitates communication among team members (sharpens up the ability to convey the ideas and teaches teamwork), and elevates the team to the next professional level due to a great number of skills that developers acquire when participating in the review.
We use cookies to provide you with a better experience on the Devart website. You can read more about our use of cookies in our Cookies Policy.
Click OK to continue browsing the Devart site. Be aware you can disable cookies at any time.
A client recently asked me to perform a code review on C# code. As an independent contractor I was wondering what are the automated solutions out there where I would not have to break the bank to carry out similar assignments.
So far I have come across the O2 Platform but the documentation is all over the place. Microsoft's FxCop is another tool I found but their reports are horrible and I'd like something that would give me a report which I could use to further carry out a manual inspection without having to pull out my hair trying to read output.
One option you could look at is the on-demand style scanning that several of the tool vendors do, which is likely to be cheaper if you're doing a smaller number of scans and also is something you might be able to factor into the cost of your assessments.
When performing a security code review, finding issues like Cross Site Scripting, SQL injection, Poor Input Validation, and others quickly can be a huge time saver. This also helps to make sure that the majority of the code has been reviewed for low hanging fruit, and then later a more in-depth review can be performed of the major areas of concern.
If you've been reading this blog for a while, you may have noticed that I'm a big fan of regular expressions. Some of these issues can be discovered by building a good regular expression. For this purpose I wrote a very basic static analysis tool I've lovingly named YASAT (Yet Another Static Analysis Tool). It uses a list of regular expression rules to scan a source tree and produce a report that a code reviewer could verify. Its purpose is to give you a sense of hot spots and produce many false positives, without too many false negatives so you can use it to start your code review off on the right foot. If you're interested in the tool go check it out on github.
One small caveat: this is not intended to be an exhaustive list of all potential security issues in ASP.NET. There is no replacement for a "brain on, eyes open" line-by-line code review. This is simply intended to give you some good starting points in a new code base quickly.
Look for any Label, Literal, Checkbox, LinkButton, RadioButton or any other control that has a ".Text" property. If the value that is assigned to the .Text is not properly encoded there is a possibility for XSS.
GridViews, DataLists, and Repeaters can be set to either encode by default or not. If you see one of these being used verify that it's being used properly. You set the data on these by assigning the DataSource property to some kind of structured data (usually a DataTable). Make sure the values in the DataSource are properly encoded or that the control is doing this automatically.
.NET can do automatic malicious character detection using the ValidateRequest=true setting. True is the default for this, so if you don't see it it's set properly. You must set this to false if you're going to accept any character that .NET thinks could be dangerous (like < or '), so it's common to see it turned off. This can live either at the top of an aspx file (between the tags) or in the web.config file in the section.
.NET has a bunch of validators (CompareValidator, CustomValidator, RegularExpressionValidator); they all work on the entire string (even if your regular expression lacks the ^$ stuff), however these only check client side by default. (Note: I wrote a blog entry on creating good regular expressions for input validation earlier.) You can check those same validators on the server by checking the Page.IsValid property, but this isn't done by default, so they're probably vulnerable if if you don't see validation and you don't see something like the following:
The common ways to execute SQL use the SqlConnection and SqlCommand classes. SqlCommand has ExecuteNonQuery and ExecuteQuery methods. NonQuery returns the number of rows that were affected while ExecuteQuery returns a DataReader used to read the SQL data stream. You'll be able to recognize all kinds of SQL injection possibilities (format strings, concatenations, etc.) when you look at the ExecuteNonQuery and ExecuteQuery methods. If they're using parameterized queries, they're probably fine.
4a15465005