Contributing a (good) pull request

Structuring a pull request

A good pull request tells a story. Each commit is a chapter in that story. Like stories, pull requests should be coherent and clearly contribute to an overarching narrative.

• Sometimes pull requests are just one small commit
• It's OK to include an opportunistic cleanup if you're changing other code in the vicinity

Avoid putting unrelated ideas into a single pull request. I'll ask you to split them out if you do. There have been situations where people have submitted unrelated code changes in a single PR where one change was OK, and the other was not.

Coding standards and naming schemes

In general:

• Follow Microsoft's Framework Design Guidelines, in particular the naming conventions. (One exception is the naming of private member fields, which I prefer: private static readonly Foo _someFoo.)
• Prefer var in all cases, unless you need to declare something more general than the return type provided by the method supplying the value. Sometimes methods change return types. Using var reduces churn in code files that consume return values from those changed methods.
• Braces: Use them, even for one-liners.
• Comments: Capture the "what" and the "how" as variable and method names if you can. Comments should be used sparingly, because they have a tendency to lie. They should be reserved for explaining the "why", when it's not obvious.

Automated tooling

• Visual Studio settings (vssettings)
• ReSharper settings (DotSettings)

Examples

Ternaries

Avoid negations. Put each branch on a new line unless it's part of, e.g., a LINQ query.

Good:

return foo == null
    ? fooAlternative
    : foo;

Bad:

return foo != null
    ? foo
    : fooAlternative;

LINQ

I prefer fluent syntax, with each idea on a single line:

var occurrences = firstEvent.GetOccurrences(startSearch, endSearch)
    .Select(o => o.Period as Period)
    .OrderBy(p => p.StartTime)
    .ToList();

There's some query syntax in ical.net, too, and it generally follows the same structure.

For some longer queries, I split the query lambda chain out into its own variable, and then materialize it's evaluation. This is sometimes useful to make clear the deferred execution nature of LINQ. Here's a silly example:

var orderedOccurrenceQuery = firstEvent.GetOccurrences(startSearch, endSearch)
    .Select(o => o.Period as Period)
    .OrderBy(p => p.StartTime);

var orderedOccurrenceResult = orderedOccurrenceQuery.ToList();

Loops

Write LINQ when you can. Write loops when you can't avoid it. "It's faster!" isn't a good reason to write a loop instead of a LINQ query.

Loops that don't have side effects are a code smell.

Avoid nesting conditionals

Good:

protected HashSet<IPeriod> EvaluateExRule(IDateTime referenceDate, DateTime periodStart, DateTime periodEnd)
{
    if (Recurrable.ExceptionRules == null || !Recurrable.ExceptionRules.Any())
    {
        return new HashSet<IPeriod>();
    }

    var evaluator = Recurrable.ExceptionRules.First().GetService(typeof(IEvaluator)) as IEvaluator;
    if (evaluator == null)
    {
        return new HashSet<IPeriod>();
    }

    var exRuleEvaluatorQuery = Recurrable.ExceptionRules.SelectMany(exRule => evaluator.Evaluate(referenceDate, periodStart, periodEnd, false));
    var exRuleExclusions = new HashSet<IPeriod>(exRuleEvaluatorQuery);
    return exRuleExclusions;
}

Bad:

protected virtual void EvaluateExRule(IDateTime referenceDate, DateTime periodStart, DateTime periodEnd)
{
    if (Recurrable.ExceptionRules != null)
    {
        foreach (IRecurrencePattern exrule in Recurrable.ExceptionRules)
        {
            IEvaluator evaluator = exrule.GetService(typeof(IEvaluator)) as IEvaluator;
            if (evaluator != null)
            {
                var periods = evaluator.Evaluate(referenceDate, periodStart, periodEnd, false);
                foreach (var p in periods)
                {
                    Periods.Remove(p);
                }
            }
        }
    }
}