Skip to content

Instantly share code, notes, and snippets.

@benj2240

benj2240/StopIt.md

Last active September 14, 2022 22:07
Show Gist options

  • Select an option

    Learn more about clone URLs
  • Save benj2240/75914d4e4eabb050728e3d075a25e5e9 to your computer and use it in GitHub Desktop.

Select an option

Learn more about clone URLs
Save benj2240/75914d4e4eabb050728e3d075a25e5e9 to your computer and use it in GitHub Desktop.
Download ZIP
Raw
StopIt.md

Stop writing code that way.

Stop putting comment headers in code files.

  • They don't help.
    • I already know what the file is named. I'm looking at it.
    • I already know where the file is stored. I'm looking at it.
    • If I need to know who wrote it originally, I can look that up.
    • If I need to know who has changed it, I can look that up.
  • They can hurt.
    • If I copy a repo from TFS to Git, I would have to edit every header
    • If I move classes to a subfolder, I would have to edit every header
    • If I rename a class, I want to rename the file, and I would have to edit the header
    • If I copy a class, I have to strip the header
    • If I edit a file, I have to edit the header
    • If I forget or miss any of those things, the header comment is now WORSE than useless; it is a lie.

Stop putting rows of comment characters above class members.

  • They don't help.
    • I can tell this is a top-level class member, by of the indentation.

Stop putting useless XML comments on class members.

  • They don't help.
    • I learn NOTHING from /// <summary></summary>
    • I can tell when a function is a constructor.
    • I can tell when a constructor accepts a dependency parameter.
    • I can tell when a function returns boolean.
    • I can tell a function's parameters' names.
    • I can tell a function's parameters' types.
    • I can tell a function's return type.
    • I can tell a property's name.
    • I know public void Dispose() supports IDisposable.
    • I know public void Dispose(bool Disposing) is part of the IDisposable pattern.
    • I know IPFLogger requires a public ILog Log { get; }.
  • They can hurt.
    • A class that could have fit on one screen may be inflated by comments to require scrolling
      • Fitting on one screen IS a desirable property. It MATTERS.
      • Obviously not every class can. Those that can, should.

Stop using regions as comments

  • Regions are regions and comments are comments.
  • The only "advantage" of a region is that it can be collapsed.
  • A function that needs parts of itself collapsed to be readable is too large.
  • A class that needs parts of itself collapsed to be readable is too large.
  • One possible exception: Regions to hide noisy interface implementation code.
  • I can tell whether a member is private or public.
  • I can tell whether a member is a property, constructor, or method.

Stop publicly exposing class members which are only used within the class.

  • It makes refactoring harder:
    • If I have a public string[] Stuff { get; set; }
    • And I want to make it a public List<string> Stuff { get; set; }
    • And my class is distributed via T:\ or Nuget
    • Before making my change I MUST search ALL CODE EVERYWHERE to ensure nothing will break
  • The same applies to public const string Name { get; }
  • The same applies to public ILog Log

Stop implementing interfaces the class isn't used as

  • If I have interface IBar { public void Bar(); },
  • And my class implements it: class Foo : IBar { public void Bar() { ... } },
  • But I only call Bar() from within my class,
  • And no one ever receives an instance of my class, knowing only that it implements IBar,
  • Then all I have done is waste my time, and make refactoring the class harder.
  • See: Stop publicly exposing members which are only used within the class.
  • YES, I am talking about IPFLoggable. Also IPFWebService, and similar.

Stop overriding Equals() and GetHashCode()

  • Overriding Equals() is invalid without overriding GetHashCode().
  • Correcly implementing GetHashCode() is difficult.
  • Our GetHashCode() implementations always rely on mutable properties, which is invalid.
  • You can get both, and often better performance, by using a struct instead.

Stop implementing Clone()

  • It doesn't help.
  • It introduces noise to the code.
  • It adds a maintenance burden.

Stop implementing static TestData

  • As a rule: Code used only for testing does not belong is production classes.
  • You can get equivalent behavior with a helper method in the test project.
  • It adds a maintenance burden.

Stop implementing ToString()

  • If I want to log some properties of a class, I will log them myself.
  • It adds a maintenance burden.

Stop implementing operator overrides

  • This is never necessary.
    • One exception would be a custom "number-ish" type...
      • DO NOT implement a custom "number-ish" type in business application code.
  • This is confusing.
  • This is even harder to maintain, as usage is nearly impossible to find.

Stop using events where overrides will do

  • Threading is harder to control and harder to reason about

Stop using getters to lazily set properties

  • Setting at instantiation is easier to write
  • Setting at instantiation is easier to read
  • Setting at instantiation results in the same performance 90% of the time
    • In fact, a initialization delay is more predictable than an access delay
    • For the other 10%, try System.Lazy

Stop using String.Format

  • Interpolation is easier to read and easier to write
    • String.Format("{3} readers {1} from {6} to {4}", "objective truth", "scan", "Hebrew", "English", "right", "fact", "left");
    • $"{language} readers scan from {startingEdge} to {trailingEdge}";

Stop using strings to refer to argument names

  • nameof(myarg) is compiler-assisted and easier to refactor

Stop testing myString.Trim().Length == 0

  • string.IsNullOrWhiteSpace(myString) is easier to write
  • string.IsNullOrWhiteSpace(myString) is easier to read
  • string.IsNullOrWhiteSpace(myString) is more performant

Stop using needless temporary variables.

  • int x = 1; return x; should be return 1; instead

Stop shuffling using statements.

  • You know what everyone knows? The alphabet.
  • You know what not everyone knows? The special way you like to sort usings.
  • Look at this. This is a real sample. Is this what you want?
    using System;
    using System.Web;
    using System.Collections;
    using System.Configuration;
    using System.Web.Services;
    using System.Web.Services.Protocols;

    using MyCompany.ReferenceData.NetStandard.Messages;
    using MyCompany.ReferenceData.Manager;
    using MyCompany.Foundation.NetStandard.Base;
    using MyCompany.Foundation.NetStandard.Data.CodeDecode;
    using MyCompany.Foundation.NetFramework.WebSvc;
    using MyCompany.Foundation.NetStandard.PFLog4Net;
    using Microsoft.Extensions.Configuration;
    using Microsoft.Extensions.FileProviders;
    using System.IO;
    using Microsoft.Extensions.Caching.Memory;
    using System.Collections.Generic;
    using System.Text;

Stop typing 2 spaces after periods.

Stop declaring variables with explicit types ...

Stop trying to write reusable code

  • Library code must be easier to use than the alternative
  • Library code must provide equivalent safety compared to the alternative ...

Stop trying to write configurable code ...

Stop trying to control serialization where the precise format doesn't matter ...

Stop adding public setters to properties that are read-only in practice ...

Stop writing "Test Stub" classes ...esp not in production projects ...

Example message class:

  • original: 65 lines, 2226 chars
  • delete empty comments -> 43 lines, 932 chars
  • use autoproperties -> 19 lines, 560 chars
  • simplify xml decorations -> 20 lines, 517 chars
  • remove xml decorations, fix bug -> 16 lines, 429 chars
  • arguably clearer -> 14 lines, 313 chars
  • That means the original was 80% cruft. EIGHTY PERCENT.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment