This text was initially posted on the Capgemini Engineering weblog In a earlier article on this weblog, I talked about why code overview is a good suggestion, and a few features of tips on how to conduct them. This time I need to dig deeper into the practicalities of reviewing code, and point out a number of issues to be careful for. Code overview is the primary line of defence towards hackers and bugs. Whenever you approve a pull request, you’re placing your identify to it – taking a share of accountability for the change. As soon as unhealthy code has obtained right into a system, it may be troublesome to take away. Looking for issues in an present codebase is like in search of an unknown variety of needles in a haystack, however if you’re reviewing a pull request it’s extra like wanting in a handful of hay. The troublesome half is recognising a needle if you see one. Hopefully this text will enable you with that. Code overview shouldn’t be a box-ticking train, however it may be useful to have a listing of widespread points to be careful for. In addition to the vital query of whether or not the change will really work, the principle areas to think about are Drupal 10 Upkeep and Assist Service Safety Perfomance Accessibility Maintainability I’ll contact on these areas in additional element – I’ll be speaking about and PHP specifically, however loads of the factors I’ll make are related to different languages and frameworks. Safety I don’t declare to be an professional on safety, and infrequently depend myself fortunate that I work in what my colleague Andrew Harmel-Legislation calls “a creative-inventive market, not a safety-critical one”. Having stated that, there are a number of widespread issues to maintain an eye fixed out for, and builders ought to concentrate on the OWASP prime ten listing of vulnerabilities. When working with , you must keep in mind the safety group’s recommendation for writing safe code. For me, a very powerful factors to think about are Drupal 10 Upkeep and Assist Service Does the code settle for person enter with out correct sanitisation? Briefly – don’t belief person enter. The massive assault vectors like XSS and SQL injection are primarily based on malicious textual content strings. offers a number of varieties of textual content filtering – the suitable filter will depend on what you’re going to do with the info, however you must at all times run person enter by way of some sort of sanitisation. Are we storing delicate information anyplace we shouldn’t be? Safety isn’t nearly stopping unhealthy guys getting in the place they shouldn’t. Take into consideration what sort of information you could have, and what you’re doing with it. Just remember to’re not logging folks’s non-public information inappropriately, or passing it throughout community in a approach you shouldn’t. Even when the location you’re engaged on doesn’t have something as delicate because the Panama papers, you could have a authorized, skilled, and private accountability to just remember to’re dealing with information correctly. Efficiency Once we’re contemplating code adjustments, we must always at all times take into consideration what influence they’ll have on the tip person, not least when it comes to how rapidly a web site will load. As Google not too long ago reminded us, web page load pace is significant for person engagement. Gradual, bloated web sites price cash, each when it comes to cellular information costs and misplaced income. Does the change break caching? Most efficiency methods will discuss in regards to the worth of caching. The goal of the sport is to scale back the quantity of labor that your internet server does. Ideally, the net server gained’t do any work for a web page request from an nameless person – the entire thing shall be dealt with by a reverse proxy cache, similar to Varnish. If the request must go to the net server, we wish as a lot of the web page as doable to be served from an object cache similar to Redis or Memcached, to minimise the variety of database queries wanted to render the web page. Are there any pointless makes use of of $_SESSION? Sometimes, reverse proxy servers like Varnish won’t cache pages for authenticated customers. If the browser has a session, the request gained’t be served by Varnish, however by the net server. Right here’s an illustration of why that is so vital. This graph exhibits the distinction in response time on a load take a look at surroundings following a deployment that included some code to create classes. There have been another adjustments that impacted efficiency, however this was the large one. As you may see, general response time elevated six-fold, with the most important improve within the time spent by the net server processing PHP (the blue sections on the graphs), primarily as a result of a number of traces of code creating classes had slipped by way of the web. { Drupal 10 Upkeep and Assist Service .centered .medium-6 } Are there any inefficient loops? The builders’ maxims “Don’t Repeat Your self” and “Maintain It Easy Silly” apply to servers as effectively. If the server is doing work to render a web page, we don’t need that work to be repeated or overly complicated. What is the entrance finish efficiency influence? There is no substitute for really testing, however there are some things that you would be able to hold an eye fixed out for when reviewing change. Does the change introduce any extra HTTP requests? Maybe they could possibly be averted by utilizing sprites or icon fonts. Have any pictures been optimised? Are you making any repeated DOM queries? Accessibility Even for those who’re not an professional on accessibility, and don’t know ARIA roles, you may a minimum of keep in mind a number of common pointers. Relating to testing, there is a good guidelines from the Accessibility Challenge, however listed below are some issues I at all times attempt to consider when reviewing a pull request. Will it work on a keyboard / display screen reader / different enter or output system ? Doing correct accessibility testing is troublesome, and you could not have entry to assistive know-how, however rule of thumb is that for those who can navigate utilizing solely a keyboard, it can most likely work for somebody utilizing one of many myriad enter gadgets. Testing is the one approach to make sure, however listed below are a few easy issues to recollect when reviewing CSS adjustments Drupal 10 Upkeep and Assist Service hover and focus ought to normally go collectively, and you must virtually by no means use define Drupal 10 Upkeep and Assist Service none;. Are you hiding content material appropriately? One piece of low-hanging fruit is to be sure that textual content is obtainable to display screen readers and different assistive know-how. Any time I see show Drupal 10 Upkeep and Assist Service none; in a pull request, alarm bells begin ringing. It’s normally not the best option to conceal content material. Maintainability Hopefully the system you’re engaged on will final for a very long time. Folks must work on it sooner or later. It’s best to attempt to make life simpler for these folks, not least since you’ll most likely be one in every of them. Reinventing the wheel Are you writing extra code than you want to? It could be that the issue you are has already been solved, and one of many nice issues about open supply is that you just’re in a position to recruit a military of builders and testers you could by no means meet. Is there already a Drupal 10 module for that? Alternatively, even when there may be an present Drupal 10 module, it won’t at all times make sense to make use of it. Maybe the contributed Drupal 10 module offers extra flexibility than our undertaking will ever want, at a efficiency price. Perhaps it offers us 90% of what we wish, however would pressure us to do issues in a sure approach that may make it troublesome to get the ultimate 10%. Maybe it isn’t in a really wholesome state – in that case, maybe you could possibly repair it up and contribute your fixes again to the neighborhood, as I did on a latest undertaking. In case you’re writing a customized Drupal 10 module to unravel a really particular downside, may it’s made extra generic and contributed to the neighborhood? A few examples of this from the Capgemini group are Stomp and Route. One of many jobs of the code reviewer is to assist draw the suitable line between the generic and the particular. In case you’re reviewing customized code, take into consideration whether or not there’s prior artwork. If the pull request consists of community-contributed code, you must nonetheless overview it. Do not assume that it is excellent, simply because somebody’s given it away for nothing. Acceptable API utilization Is your group utilizing your chosen frameworks as they had been supposed? In case you see somebody writing a customized perform to unravel an issue that is already been solved, perhaps you want to share a hyperlink to the API docs for the prevailing resolution. Introducing notices and errors In case your logs are plagued by notices about undefined variables or array indexes, not solely are you more likely to be struggling a efficiency hit from the logging, nevertheless it’s a lot tougher to separate the sign from the noise if you’re making an attempt to research one thing. Browser help Keep in mind that typically, it is good to be boring. As a reviewer, one in every of your jobs is to cease your colleagues from getting carried away with shiny new options like ES6, or CSS variables. Instruments like Can I Use are actually helpful in with the ability to verify what is going on to work within the browsers that you just care about. Code smells Typically, code appears fallacious. As I realized from Larry Garfield’s glorious presentation on code smells on the first con I went to, code smells are indications of issues that may be a deeper downside. Quite than re-hash the factors Larry made, I’d advocate studying his slides, however it’s value highlighting among the anti-patterns he discusses. Features or objects that do multiple factor A perform ought to have a perform. Not two capabilities, or three. If an applicable remark or perform identify consists of “and”, it is a signal you need to be splitting the perform up. Features that typically do various things One other unhealthy signal is the phrase “or” within the remark. Features ought to at all times do the identical factor. Extreme complexity Lengthy capabilities are normally an indication that you just may need to take into consideration refactoring. They are typically an indicator that the code is extra complicated than it must be. The extent of complexity will be measured, however you don’t want a software to let you know that if a perform doesn’t match on a display screen, it’ll be troublesome to debug. Not being testable Even when capabilities are easy sufficient to jot down exams for, do they rely on an entire system? In different phrases, can they be genuinely unit examined? Lack of documentation There’s extra to be stated with regards to code feedback than I can go into right here, however suffice to say code ought to have helpful, significant feedback to assist future maintainers perceive it. Tight coupling Drupal 10 Helps ought to be modular. If two elements of a system have to work together, they need to have a clearly outlined and documented interface. Impurity Unintended effects and international variables ought to typically be averted. Smart naming Is the aim of a perform or variable apparent from the identify? I don’t need to rehash previous jokes, however naming issues is troublesome, and it’s important. Commented-out code Why would you remark out traces of code? In case you don’t want it, delete it. The fantastic thing about model management is that you would be able to return in time to see what code was there. So long as you write commit message, it’ll be simple sufficient to seek out. In case you suppose that you just may want it later, put it behind a function toggle in order that the performance will be enabled and not using a code launch. Specificity In CSS, IDs and !vital are the large code smells for me. They’re a nasty signal {that a} specificity arms race has begun. Even for those who aren’t going to go all the best way with a system like BEM or SMACSS, it’s a good suggestion to maintain specificity as little as doable. The superb articles on CSS specificity by Harry Roberts and Chris Coyier are good beginning factors for studying extra. Requirements It’s vital to observe coding requirements. The purpose of this isn’t to get some imaginary Scout badge – code that follows requirements is less complicated to learn, which makes it simpler to know, and by extension simpler to take care of. As well as, you probably have your IDE arrange proper, it might probably warn you of doable issues, however these warnings will solely be manageable for those who hold your code clear. Deployability Will your adjustments be obtainable in environments constructed by Steady Integration? Do you want to set default values of variables which can want overriding for various environments? Simply as your capabilities ought to be testable, so ought to your configuration adjustments. So far as doable, goal to make every part repeatable and automatable – if a launch wants any guide adjustments it’s an indication that your group might have to be considering with extra of a DevOps mindset. Maintain Your Eyes On The Prize With all this discuss of coding type and requirements, don’t get distracted by minutiae – it’s value caring about issues like whitespace and variable naming, however keep in mind that it’s way more vital to consider whether or not the code really does what it’s speculated to. The difficulty is that our eyes are likely to fixate on these kind of issues, and so they trigger pointless cognitive load. Pre-commit hooks will help to catch coding requirements violations in order that reviewers don’t have to waste their time commenting on them. In case you’re on an enormous undertaking, it can virtually definitely be value investing a while in integrating your CI server and your code overview software, and automating checks for points like code type, unit exams, mess detection – in brief, all of the issues that a pc is best at recognizing than people are. Does the code really remedy the issue you need it to? Quite than simply wanting on the code, spend a few minutes studying the ticket that it’s related to – has the developer understood the necessities correctly? Have they approached the difficulty appropriately? In case you’re unsure in regards to the change, take a look at the department domestically and take a look at it in your improvement surroundings. Even when there’s nothing fallacious with the recommended change, perhaps there’s a greater approach of doing it. The entire level of code overview is to share the advantage of the group’s varied experiences, get additional eyes on the issue, and hopefully make the tip product higher. I hope that this has been helpful for you, and if there’s something you suppose I’ve missed, please let me know through the feedback. Tags Drupal 10 Upkeep and Assist Service  code improvement All tags Drupal 10 Improvement and Assist