What are some hidden red flags to look for when doing code review?

When reviewing another developers code, what are some red flags I should look for?

Answers requested for

• Using Httpclient wrong
• SiteDefinition.current.StartPage in a scheduled job.

Additional Posts

A few specific things:

• If using HttpClient, do not Dispose of and recreate an instance for each request. Instead, create on instance and cache that instance for the entire application. See here: https://aspnetmonsters.com/2016/08/2016-08-27-httpclientwrong/ .
• Inside a Scheduled Job, you can't rely on things that rely on HttpContext, because there is no context (Scheduled Jobs run in a separate thread generally). So things like SiteDefinition.Current might not return what you expect (especially if none of the sites in Episerver are set up as the * fallback site). For site definitions, use ISiteDefinitionRepository instead.
• If you're using FindPagesByCritera in any place that gets hit frequently (such as a normal Page Controller), you almost certainly should be caching. FindPagesByCriteria is slow and doesn't have built-in caching like IContentLoader methods do.
• If you're using ContentLocator, you might need caching. Many of those methods either use FindPagesByCriteria or load a significant amount of content to search. Consider using an external index such as Find, Solr, or Content Index.
• Transient data should not be put on IContent objects. If you need to store info for a page view, put it in the View Model class, not the content object. Content objects share a single instance across threads so any transient data you put on the object gets shared across all references to that object. This is not the case for View Model classes, where you can and should store transient data.
• Never use CreateWritableClone() unless you're planning to save changes to some content. If you find yourself needing to call this method for any other reason, you're doing something wrong.
• DDS is great for storing small amounts of data. It quickly becomes slow and cumbersome for large datasets.
• Don't get SiteDefinition via dependency injection. Use SiteDefinition.Current if you have a request, or ISiteDefinitionRepository if you don't. Maybe it's been fixed, but in our experience, the injected SiteDefinition would always return the fallback site, not necessarily the one the user is currently on.
• If you're putting something in a query string, use some kind of builder or make sure to URL encode the value. /search?q=@search is bad. search?q=@Uri.EscapeDataString(search) is better.
• I always look at @Html.Raw extra closely - serious potential for XSS vulnerabilities. Use @Html.PropertyFor or @Html.XhtmlString for XHtmlString values (otherwise visitor groups and embedded blocks don't render correctly).
• Forms that have an effect (such as signing up for an email list, or changing data somewhere) must have a validated antiforgery token and be a POST. Forms that don't have an effect (such as search forms) don't need a token and should be a GET.
• If you're using GetChildren<T> for navigation, you should be filtering the results for access level and publish dates.

Some general things:

• Avoid putting logic or "work" in constructors. Constructors should be quick and should basically never throw exceptions. If you're object requires work to be created, if possible, consider putting that work in a static method and making the constructor private. Something like MyObject.CreateInstance(...args).
• If you catch an exception, you need to log it. Otherwise, you might as well write catch(Exception) { /* Any problems here will go unreported for a long time, and when we finally hear about it we'll have no useful information! */ } In general, do not catch exceptions unless you can gracefully handle the failure or if it's truly an exception you don't care about (such as a Twitter feed failing to load). Globally, uncaught exceptions should be handled and sent to Sentry.
• Getters and Setters should be lightweight and have no external dependencies. Getters should have no side effects. If your getter or setter is hitting a database, it should be a method. If your getter changes a value somewhere else, it's probably wrong. In general, Properties are for fast, simple logic. I should be able to repeatedly call a property without worrying about it being slow or affecting other things.
• Don't repeat yourself (within reason). If you've got a long LINQ query for example, cache the result of that query in a variable instead of copy/pasting it multiple times. Assign chunks of logic to variables or functions and give it a good name. Much easier to read if (userIsAllowedToEdit) in 3 places than if (user.IsActive && (!user.CanExpire || !user.IsExpired) && (user.IsInRole("Admin") || (user.IsInRole("Editor") && user.Name == page.Owner)) repeated the same number of times.