affiliate_link
Hiển thị các bài đăng có nhãn code reviews. Hiển thị tất cả bài đăng
Hiển thị các bài đăng có nhãn code reviews. Hiển thị tất cả bài đăng

Thứ Tư, 22 tháng 5, 2013

7 Agile Best Practices that You Don’t Need to Follow

There are many good ideas and practices in Agile development, ideas and practices that definitely work: breaking projects into Small Releases to manage risk and accelerate feedback; time-boxing to limit WIP and keep everyone focused; relying only on working software as the measure of progress; simple estimating and using velocity to forecast team performance; working closely and constantly with the customer; and Continuous Integration – and Continuous Delivery – to ensure that code is always working and stable.

But there are other commonly accepted ideas and best practices that aren’t important: if you don’t follow them, nothing bad will happen to you and your project will still succeed. And there are a couple that you are better off not following at all.

Test-Driven Development

Teams that need to move quickly need to depend on a fast, efficient testing safety net. With Test First Development or Test-Driven Development (TDD), there’s no excuse for not writing tests – after all, you have to write a failing test before you write the code. So you end up with a good set of working automated tests that ensure a high level of coverage and regression protection.

TDD is not only a way of ensuring that developers test their code. It is also advocated as a design technique that leads to better quality code and a simpler, cleaner design.

A study of teams at Microsoft and IBM (Realizing Quality Improvement through Test Driven Development, Microsoft Research, 2008) found that while TDD increased upfront development costs between 15-35% (TDD demands developers change the way that they think and work, which slows developers down, at least at first), it reduced defect density by 40% (IBM) or as much as 60-90% (Microsoft) over teams that did not follow disciplined unit testing.

But in Making Software Chapter 12 “How Effective is Test-Driven Development” researchers led by Burak Turhan found that while TDD improves external quality (measured by one or more of test cases passed, number of defects, defect density, defects per test, effort required to fix defects, change density, % of preventative changes) and can improve the quality of the tests (fewer mistakes in the tests, tests that are easier to maintain), TDD does not consistently improve the quality of the design. TDD seems to reduce code complexity and improve reuse, however it also negatively impacts coupling and cohesion. And while method and class-level complexity is better in code developed using TDD, project/package level complexity is worse.

People who like TDD like it a lot, so if you like it, do it. And even if you are not TDD-infected, there are times when working test first is natural – when you have to solve a specific problem in a specific way, or if you’re fixing a bug where the failing test case is already written up for you. But the important thing is that you write a good set of tests and keep them up to date and run them frequently – it doesn't matter if you write them before, or after, you write the code.

Pair Programming

According to the VersionOne State of Agile Development Survey 2012, almost 1/3 of teams follow pair programming – a surprisingly high number, given how disciplined pair programming is, and how few teams follow XP (2%) or Scrum/XP Hybrid (11%) methods where pair programming would be prescribed.

There are good reasons for pairing: information sharing and improving code quality through continuous, informal code reviews as developers work together. And there are natural times to pair developers, or sometimes developers and testers, together: when you’re working through a hard design problem; or on code that you’ve never seen before and somebody who has worked on it is available to help; or when you’re over your head in troubleshooting a high-pressure problem; or testing a difficult part of the system; or when a new person joins the team and needs to learn about the code and coding practices.

Some (extroverted) people enjoy pairing up, the energy it creates and the opportunities it provides to get to know others on the team. But forcing people who prefer working on their own or who don’t like each other to work closely together is definitely not a good idea. There are real social costs in pairing: you have to be careful to pair people up by skill, experience, style, personality type and work ethic. And sustained pair programming can be exhausting, especially over the long term – one study (Vanhanen and Lassenius 2007) found that people only pair between 1.5 and 4 hours a day on average, because it’s too intense to do all day long.

In Pair Programming Considered Harmful? Jon Evans says that pairing can have also negative effects on creativity:

Research strongly suggests that people are more creative when they enjoy privacy and freedom from interruption … What distinguished programmers at the top-performing companies wasn’t greater experience or better pay. It was how much privacy, personal workspace and freedom from interruption they enjoyed,” says a New York Times article castigating “the new groupthink”.

And in “Still Questioning Extreme Programming” Pete McBreen points out some other disadvantages and weaknesses of pair programming:

  • Exploration of ideas is not encouraged, pairing makes a developer focus on writing the code, so unless there is time in the day for solo exploration the team gets a very superficial level of understanding of the code.
  • Developers can come to rely too much on the unit tests, assuming that if the tests pass then the code is OK. (This follows on from the lack of exploration.)
  • Corner cases and edge cases are not investigated in detail, especially if they are hard to write tests for.
  • Code that requires detail thinking about the design is hard to do when pairing unless one partner completely dominates the session. With the usual tradeoff between partners, it is hard to build technically complex designs unless they have been already been worked out in a solo session.
  • Personal styles matter when pairing, and not all pairings are as productive as others.
  • Pairs with different typing skills and proficiencies often result in the better typist doing all of the coding with the other partner being purely passive.
And of course pairing in distributed teams doesn't work well if at all (depending on distance, differences in time zones, culture, working styles, language), although some people still try.

While pairing does improve code quality over solo programming, you can get the same improvements in code quality, and at least some of the information sharing advantages, through code reviews, at less cost. Code reviews – especially lightweight, offline reviews – are easier to schedule, less expensive and less intrusive than pairing. And as Jason Cohen points out even if developers are pair programming, you may still need to do code reviews, because pair programming is really about joint problem solving, and doesn’t cover all of the issues that a code review would.

Back to Jon Evans for the final word on pair programming:

The true answer is that there is no one answer; that what works best is a dynamic combination of solitary, pair, and group work, depending on the context, using your best judgement. Paired programming definitely has its place. (Betteridge’s Law strikes again!) In some cases that place may even be “much of most days.” But insisting on 100 percent pairing is mindless dogma, and like all mindless dogma, ultimately counterproductive.

Emergent Design and Metaphor

Incremental development works, and trying to keep design simple makes good sense, but attempting to define an architecture on the fly is foolish and impractical. There’s a reason that almost nobody actually follows Emergent Design: it doesn't work.

Relying on a high-level metaphor (the system is an "assembly line" or a "bill of materials" or a "hive of bees") shared by the team as some kind of substitute for architecture is even more ridiculous. Research from Carnegie Mellon University found that

… natural language metaphors are relatively useless for either fostering communication among technical and non-technical project members or in developing architecture.
Almost no one understands what a system metaphor is any ways, or how it is to be used, or how to choose a meaningful metaphor or how to change it if you got it wrong (and how you would know if you got it wrong), including one of the people who helped come up with the idea:
Okay I might as well say it publicly - I still haven't got the hang of this metaphor thing. I saw it work, and work well on the C3 project, but it doesn't mean I have any idea how to do it, let alone how to explain how to do it.
Martin Fowler, Is Design Dead?

Agile development methods have improved development success and shown better ways to approach many different software development problems – but not architecture and design.

Daily Standups

When you have a new team and everyone needs to get to know each other and more time to understand what the project is about; or when the team is working under emergency conditions trying to fix something or finish something under extreme pressure, then getting everyone together in regular meetings, maybe even more than once a day, is necessary and valuable. But whether everyone stands up or sits down and what they end up talking about in a meeting should be up to you.

If your team has been working well together for a while and everyone knows each other and knows what they are working on, and if developers update cards on a task board or a Kanban board or the status in an electronic system as they get things done, and if they are grown up enough to ask for help when they need it, then you don’t need to make them all stand up in a room every morning.

Collective Code Ownership

Letting everyone work on all of the code isn't always practical (because not everyone on the team has the requisite knowledge or experience to work on every problem) and collective code ownership can have negative effects on code quality.

Share code where it makes sense to do so, but realize that not everybody can – or should – work on every part of the system.

Writing All Requirements as Stories

The idea that every requirement specification can be written as User Stories in 1 or 2 lines on cards, that requirements should be too short on purpose (so that the developer has to talk to someone to explain what’s really needed) and insisting that they should all be in the same template form

“As a type of user I want some goal so that some reason…”
is silly and unnecessary. This is the same kind of simple minded orthodoxy that led everyone to try to capture all requirements in UML Use Case format with stick men and bubbles 15 years ago.

There are many different ways to effectively express requirements. Sometimes requirements need to be specified in detail (when you have to meet regulatory compliance or comply with a standard or integrate with an existing system or implement a specific algorithm or…). Sometimes it’s better to work from a test case or a detailed use case scenario or a wire frame or some other kind of model, because somebody who knows what’s going on has already worked out the details for you. So pick the format and level of detail that works best and get to work.

Relying on a Product Owner

Relying on one person as the Product Owner, as the single solitary voice of the customer and the “one throat to choke” when the project fails, doesn't scale, doesn't last, and puts the team and the project and eventually the business at risk. It’s a naïve, dangerous approach to designing a product and to managing a development project, and it causes more problems than it solves.

Many teams have realized this and are trying to work around the Product Owner idea because they have to. To succeed, a team needs real and sustained customer engagement at multiple levels, and they should take responsibility themselves for making sure that they get what they need, rather than relying on one person to do it all.

Thứ Năm, 7 tháng 3, 2013

Peer reviews for security are a waste of time?

At this year’s RSA conference, one of the panel’s questioned whether software security is a waste of time. A panellist, John Viega, said a few things that I agreed with, and a lot that I didn't. Especially that

“peer reviews for security are a waste of time.”

This statement is wrong on every level.

Everyone should know by now that code reviews find real bugs – even informal, lightweight code reviews.

“Reviews catch more than half of a product’s defects regardless of the domain, level of maturity of the organization, or lifecycle phase during which they were applied”. What We Have Learned About Fighting Defects

Software security vulnerabilities are like other software bugs – you find them through testing or through reviews. If peer code reviews are a good way to find bugs, why would they be a waste of time for finding security bugs?

There are only a few developers anywhere who write security plumbing: authentication and session management, access control, password management, crypto and secrets handling. Or other kinds of plumbing like the data access layer and data encoding and validators that also have security consequences. All of this is the kind of kind of stuff that should be handled in frameworks anyway – if you’re writing this kind of code, you better have a good reason for doing it and you better know what you are doing. It’s obviously tricky and high-risk high-wire work, so unless you’re a team of one, your code and decisions need to be carefully reviewed by somebody else who is at least as smart as you are. If you don’t have anyone on the team who can review your work, then what the hell are you doing trying to write it in the first place?

Everybody else has to be responsible for writing good, defensive application code. Their responsibilities are:

  • Make sure their code works – that the logic is correct
  • Use the framework properly
  • Check input data and return values
  • Handle errors and exceptions correctly
  • Use safe routines/APIs/libraries
  • Be careful with threading and locking and synchronization

A good developer can review this code for security and privacy requirements: making sure that you are masking or encrypting or – even better – not storing PII and secrets, auditing, properly following access control rules. And they can review the logic and workflow, look for race conditions, check data validation, make sure that error handling and exception handling is done right and that you are using frameworks and libraries carefully.

This is what code reviews are for. To look for and find coding problems. If you find these problems – and code reviews are one of the most effective ways of doing this – your code will be safer and more secure. So I don’t get it. Why are peer reviews for security a waste of time?

Thứ Tư, 20 tháng 2, 2013

A Bug is a Terrible Thing to Waste

Some development teams, especially Agile teams, don’t bother tracking bugs. Instead of using a bug tracking system, when testers find a bug, they talk to the developer and get it fixed, or they write a failing test that needs to be fixed and add it to the Continuous Integration test suite, or if they have to, they write up a bug story on a card and post it on the wall so the team knows about it and somebody will commit to fixing it.

Other teams live by their bug tracking systems, using tools like Jira or Bugzilla or FogBugz to record bugs as well as changes and other work. There are arguments to be made for both of these approaches.

Arguments for tracking bugs – and for not tracking bugs

In Agile Testing: A Practical Guide for Testers and Agile Teams, Lisa Crispin and Janet Gregory examine the pros and cons of using a defect tracking system.

Using a system to track bugs can be the only effective way to manage problems for teams who can’t meet face-to-face - for example, distributed teams spread across different time zones. It can also be useful for teams who have inherited open bugs in a legacy system; and a necessary evil for teams who are forced to track bugs for compliance reasons. The information in a bug database is a potential knowledge base for testers and developers joining the team – they can review bugs that were found before in the area of the code that they are working on to understand what problems they should look out for. And bug data can be used to collect metrics and create trends on bugs – if you think that bug metrics are useful.

But the Lean/Agile view is that using a defect tracking system mostly gets in the way and slows people down. The team should stay focused on finding bugs, fixing them, and then forget about them. Bugs are waste, and everything about them is waste – dead information, and dead time that is better spent delivering value. Worse, using a defect tracking system prevents testers and developers from talking with each other, and encourages testers to take a “Quality Police” mindset. Without a tool, people have to talk to each other, and have to learn to play nicely together.

This is a short-term, tactical point of view, focused on what is needed to get the software out the door and working. It’s project-thinking, not product thinking.

Bugs over the Long Term

But if you’re working on a system over a long time like we are, if you're managing a product or running a service, you know that it’s not that simple. You can’t just look at what’s in front of you, and where you want to be in a year or more. You also have to look back, at the work that was done before, at problems that happened before, at decisions that were made before, to understand why you are where you are today and where you may be heading in the future.

Because some problems never go away. And other problems will come back unless you do something to stop it. And you’ll find out that other problems which you thought you had licked never really went away. The information from old bugs, what happened and what somebody did to fix them (or why they couldn't fix them), which workarounds worked (and which didn't) can help you understand and deal with the problems that you are seeing today, and help you to keep improving the system and how you build it and keep it running.

Because you should understand the history of changes and fixes to the code if you’re going to change it. If you like the way the code is today, you might want to know how and why it got this way. If you don’t like it, you’ll want to know how and why it got this way – it’s arrogant to assume that you won’t make the same mistakes or be forced into the same kinds of situations. Revision control will tell you what was changed and when and who did it, the bug tracking system will tell you why.

Because you need to know where you have instability and risk in the system. You need to identify defect-dense code or error-prone code – code that contains too many bugs, code that is costing you too much to maintain and causing too many problems, code that is too expensive to keep running the way that is today. Code that you should rewrite ASAP to improve stability and reduce your ongoing costs. But you can’t identify this code without knowing the history of problems in the system.

Because you may need to prove to auditors or regulators and customers and investors that you are doing a responsible job of testing and finding bugs and fixing them and getting the fixes out.

And because you want to know how effective the team is in finding, fixing and preventing bugs. Are you seeing fewer bugs today? Or more bugs? Are you seeing the same kinds of bugs – are you making the same mistakes? Or different mistakes?

Do you need to track every Bug?

As long as bugs are found early enough, there’s little value in tracking them. It’s when bugs escape that they need to be tracked: bugs that the developer didn't find right away on their own, or in pairing, or through the standard automated checks and tests that are run in Continuous Integration.

We don’t log

  • defects found in unit tests or other automated tests – unless for some reason the problem can’t or won’t be fixed right away;
  • problems found in peer reviews – unless something in the review is considered significant and can’t get addressed immediately. Or a problem is found in a late review, after testing has already started, and the code will need to be retested. Or the reviewer finds something wrong in code that wasn't changed, an old bug – it’s still a problem that needs to be looked at, but we may not be prepared to deal with it right now. All problems found in external reviews, like a security review or an audit, are logged;
  • static analysis findings – most of the problems caught by these tools are simple coding mistakes that can be seen and fixed right away, and there’s also usually a fair amount of noise (false positives) that has to be filtered out. We run static analysis checks and review them daily, and only log findings if we agree that the finding is real but the developer isn't prepared to fix it immediately (which almost never happens, unless we’re running a new tool against an existing code base for the first time). Many static analysis tools have their own systems for tracking static analysis findings any ways, so we can always go back and review outstanding issues later;
  • bugs found when developers and testers decide to pair together to test changes early in development, when they are mostly exploring how something should work – we don’t usually log these bugs unless they can’t be / won’t be fixed (can’t be reproduced later for example).

A Bug is a Terrible thing to Waste

We log all other bugs regardless of whether they are found in production, in internal testing, partner testing, User Acceptance Testing, or external testing (such as a pen test). Because most of the time, when software is handed to a tester, it’s supposed to be working. If the tester finds bugs, especially serious ones, then this is important information to the tester, to the developer, and to the rest of the team. It can highlight risks. It can show where more testing and reviews need to be done. It can highlight deeper problems in the design, a lack of understanding that could cause other problems.

If you believe that testing provides important information not just about the state of your software, but also on how you are designing and building it – then everyone needs to be able to see this information, and understand it over time. Some problems can’t be seen or fully understood right away, or in 1-week or 2-week sprint-sized chunks. It can take a while before you recognize that you have a serious weakness in the design or that something is broken in your approach to development or in your culture. You’ll need to experience a few problems before you start to find relationships between them and before you can look for their root cause. You’ll need data from the past in order to solve problems in the future.

Tracking bugs isn't a waste if you learn from bugs. Throwing the information on bugs away is the real waste.

Thứ Tư, 6 tháng 2, 2013

Code and Code Reviews: What’s in a Name?

In a code review a developer needs to look at the code from two different perspectives:

  1. Correctness. Is the code logically correct, does it do what it is supposed to do? Will it hold up in the real world? Is it safe? Does it handle errors and exceptions? Does it check for bad input parameters and return values? Is it secure? And where performance is important, is it efficient?
  2. Maintainability. Can I understand this code well enough to maintain it, could I change it safely myself? Is it readable and consistent? Is the logic too complex, are the pieces too big? Is it unit tested and if not can it be unit tested? This is where a reviewer looks out for too much copying and pasting, whether hand-rolled code could be done using standard libraries or language features instead, adherence to style guidelines and standards.

It’s obvious that using good names for classes and methods and variables is important in making code understandable (if you can’t understand it, you can’t tell if it is doing what it is supposed to do) and easier to maintain. Books like Clean Code and Code Complete have entire chapters on proper naming. But even good, experienced developers have a hard time coming up with the right abstractions and good, meaningful, intention-revealing names for them. It’s especially hard if they’re working on code that they don’t know that well.

Bad names cause reviewers to stumble, or to make the wrong assumptions about what the code is doing. There are lame names – lazy, ambiguous, generic names that don’t help the reader understand what’s happening in the code. Then there are names which are misleading or just plain wrong – names which used to be right, but aren't now because the logic changed but the name didn't. You’re reading through code, it calls postPayment, but postPayment doesn't just post a payment, it does a lot more now – or worse, it doesn't post a payment at all any more.

Focusing on naming becomes more important over time as more code gets changed more often. The design changes, responsibilities change, often incrementally and subtly. Code gets added, then later other code gets deleted or moved. The programmer making the latest change just wants to focus on what they’re doing, fix the code and get out, and doesn't look at the bigger picture, doesn't notice that the meaning of the code has changed or become less clear because of what they have just done.

Reviewers need to be on the watch for these kinds of problems.

But naming isn't just about making it easier for somebody else to read the code and maintain it – or even making it easier for you to read it and change it yourself when you come back to it later. As a colleague of mine has pointed out, if someone can’t come up with a good name for a method or class, it’s a sign that they are having problems understanding what they were working on. So in reviewing code, a bad name is more than just a distraction or an irritation – it’s a warning sign that there could be more problems in the code, that the developer might not have understood the design well enough to make the change correctly, or that they weren't paying close enough attention to what they were working on, and they may have made other mistakes that you – the reviewer – need to be on the lookout for.

Focusing on naming sometimes seems fussy and nit picky, another battle in the endless “style wars” which end in hand-to-hand fighting over bracket placement and indentation. But good naming is much more important than aesthetics or standardization. It’s about making sure that code is working correctly, and that it will stay that way.

Thứ Năm, 15 tháng 12, 2011

2011: The State of Software Security and Quality

It’s the end of the year. Time to look back on what you’ve done, what you’ve learned, your successes and mistakes, and what you learned from them. I also like to look at the big picture: not just my team and the projects that I manage, or even the company that I work for, but software development in general. How are we doing as an industry, are we getting better, where are we falling behind, what are the main drivers for developers and development managers?

Besides the usual analysis from InformationWeek, and from Forrester and Gartner (if you can afford them), there’s some interesting data available from software quality and software security vendors.

CAST Software
, a vendor of code structural analysis tools, publishes an annual Report on Application Software Health based on an analysis of 745 applications from 160 companies (365 million lines of code) using CAST's static analysis platform. A 25-page executive summary of the report is available free with registration – registration also provides you access to some interesting research from Gartner and Forrester Research, including a pretty good paper from Forrester Research on application development metrics.

The code that CAST analyzed is in different languages, approximately half of it in Java. Their findings are kinda interesting:
  • They found no strong correlation between the size of the system and structural quality except for COBOL apps where bigger apps have more structural quality problems.
  • COBOL apps also tended to have more high-complexity code modules.
  • I thought that the COBOL findings were interesting, although I haven’t worked with COBOL code in a long long time. Maybe too interesting – CAST decided that the findings for COBOL were so far outside of the norm that “consequently we do not believe that COBOL applications should be directly benchmarked against other technologies”.
  • For 204 applications, information was included on what kind of application development method was followed. Applications developed with Agile methods and Waterfall methods had similar profiles for business risk factors (robustness, performance, security) but Agile methods were not as effective when it comes to cost factors (transferability, changeability), which would seem counter-intuitive, given that Agile methods are intended to reduce the cost of change.
  • The more releases per year, the less robust, less secure and less changeable the code was. Even the people at CAST don’t believe this finding.
CAST also attempts to calculate an average technical debt cost for applications, using the following formula:
(10% of low severity findings + 25% of medium severity findings + 50% of high severity findings) * # of hours to fix a problem * cost per hour for development time
The idea is that not all findings from the static analysis tool need to be fixed (maybe 10% of low severity findings and 25% of medium severity findings), but at least half of the high severity issues found need to be fixed. They assume that on average each of these fixes can be made in 1 hour, and estimate that the average development cost for doing this work is $75 per hour. This results in an average technical debt cost of $3.61 per LOC. For Java apps, the cost is much higher, at $5.42 per LOC. So your average 100 KLOC Java system carries about $500,000 worth of technical debt....

It’s difficult to say how real or meaningful these findings are. The analysis depends a lot on the extensiveness of CAST’s static analysis checkers (with different checking done for different languages, making it difficult to compare findings across languages) and the limited size of their customer base. As their tools improve and their customer base grows, this analysis will become more interesting, more accurate and more useful.

The same goes for the State of Software Security Report from Veracode, a company that supplies static and dynamic software security testing services. Like the CAST study, this report attempts to draw wide-ranging conclusions from a limited data set – in this case, the analysis of almost 10,000 “application builds” over the last 18 months (which is a lot less than 10,000 applications, as the same application may be analyzed at least twice if not more in this time window). The analysis focused on web apps (75% of the applications reviewed were web apps). Approximately half of the code was in Java, one quarter in .NET, the rest in C/C++, PHP, etc.

Their key findings:
  • 8 out of 10 apps fail to pass Veracode’s security tests on the first pass - the app contains at least 1 high-risk vulnerability.
  • For web apps, the top vulnerability is still XSS. More than half of the vulnerabilities found are XSS, and 68% of web apps are vulnerable to XSS attacks.
  • 32% of web apps were vulnerable to SQL Injection, even though only 5% of all vulnerabilities found were SQL Injection issues.
  • For other apps, the most common problems were in error handling (19% of all issues found) and cryptographic mistakes (more than 46% of these apps had issues with crypto).
Another interesting analysis is available from SmartBear Software, a vendor of software quality tools, which recently sponsored a webinar by Capers Jones on The State of Software Quality in 2011. Capers Jones draws from a much bigger database of analysis over a much longer period of time. He provides a whirlwind tour of facts and findings in this webinar, summarizing and updating some of the material that you can find in his books.

SmartBear provides some other useful free resources, including a good white paper on best practices for code review. This is further explored in the chapter on Modern Code Reviews by SmartBear’s Jason Cohen in the book Making Software: What Really Works, and Why we Believe it, a good collection of essays on software development practices and the state of research in software engineering.

All of these reports are free – you need to sign up, but you will get minimal hassle – at least I haven’t been hassled much so far. I’m not a customer of any of these companies (we use competing or alternative solutions that we are happy with for now) but I am impressed with the work that these companies do to make this information available to the community.

There were no real surprises from this analysis. We already know what it takes to build good software, we just have to make sure that we actually do it.