Skip to content

Coding Standards

Jump to bottom
Arnaud Bailly edited this page Jun 11, 2021 · 45 revisions

Foreword

This file contains agreed-upon coding standards and best practices, as well as proposals for changes or new standards. Also, the concept and topics are shamelessly stolen from the Adrestia Wiki (thanks guys!).

Proposals are prefixed with [PROPOSAL] until discussed and voted on by the Hydra team. To be accepted, a practice should be voted with majority + 1, with neutral votes counting as positive votes.

Each proposal should start with a section justifying the standard with rational arguments. When it makes sense, we should also provide good and bad exmaples to make the point clearer.

Git

Commit message format

Here are seven rules for great git commit messages:

  1. Separate subject from body with a blank line
  2. Limit the subject line to 50 characters (soft limit)
  3. Capitalize the subject line
  4. Do not end the subject line with a period
  5. Use the imperative mood in the subject line and suffix with ticket number if applicable
  6. Wrap the body at 72 characters (hard limit)
  7. Use the body to explain what and why vs. how

Why

Git commit messages are our only source of why something was changed the way it was changed. So we better make the readable, concise and detailed (when required).

Haskell

Use automatic code formatting

In general, we will use a code formatting tool and enforce it's usage by CI or commit hooks. Refer to the appropriate configuration file in our repository. Any change to which tool and configuration is used shall be discussed and voted upon in the corresponding pull request. Things which are not covered by our tool of choice are also mentioned and voted on in here.

Why

  • Focus on content and logic when writing and reviewing code
  • Avoid discussions (and bikeshedding) code format (e.g. indentation) on pull requests

Use only a single blank line between top-level definitions

ℹ️ Should be enforced by code formatting tool (e.g. Fourmolu).

A source code file should not contain multiple consecutive blank lines.

Use only a single blank line between the following top-level definitions:

  • function definitions
  • data type definitions
  • class definitions
  • instance definitions

Why

  • Consistency with other Haskell code.
  • Excessive vertical space increases the amount of unnecessary scrolling required to read a module.
See Examples 
-- BAD
newtype Foo = Foo Integer
    deriving (Eq, Show)



newtype Bar = Bar Integer
    deriving (Eq, Show)
-- GOOD
newtype Foo = Foo Integer
    deriving (Eq, Show)

newtype Bar = Bar Integer
    deriving (Eq, Show)
-- BAD
instance FromCBOR Block where
    fromCBOR = Block <$> decodeBlock



newtype BlockHeader = BlockHeader
    { getBlockHeader :: Primitive.BlockHeader
    } deriving Eq
-- GOOD
instance FromCBOR Block where
    fromCBOR = Block <$> decodeBlock

newtype BlockHeader = BlockHeader
    { getBlockHeader :: Primitive.BlockHeader
    } deriving Eq

Naming record fields

  • We do use short and concise names for fields in record data types.
  • We use DuplicateRecordFields when needed to avoid conflicts.
  • We use type annotations, getField or generic-lens to disambiguate record field access.

Why

  • Use the most suitable name for record field without restrictions.
  • This allows to derive instances for pretty-printing, serialization etc. with minimal amount of boiler-plate.
  • Avoid redundancy and errors because of it
See Examples 
-- GOOD
data Foo = Foo
  { name :: Integer
  , value :: Value
  }
-- BAD
data Foo = Foo
  { fooName :: Integer
  , fooValue :: Value
  }
-- BAD
data FooBarBaz = Foo
  { _fbbName :: Integer
  , _fbbValue :: Value
  }

Avoid boolean blindness

Do not pass Bool as arguments to functions if it can be avoided. Alternatives are (also see examples):

  • Make it two functions
  • Create a newtype around Bool (We do want to avoid that as well though)
  • Create a dedicated data type

Why

  • Side-steps the type system and misses the opportunity to catch errors due to mixing multiple Bool arguments
  • Helps in documenting what's going on (we can still add documentation of course)
See Examples 
-- WORST
client :: Bool -- ^ Adding documentation is not enough
       -> IO ()
client pipelined =
  -- Naming ^ the argument is not enough
  pure ()
-- BAD
type IsPipelined = Bool

client :: IsPipelined -> IO ()
client pipelined =
  -- Naming ^ the argument is not enough
  pure ()
-- BAD
newtype IsPipelined = IsPipelined Bool

client :: IsPipelined -> IO ()
-- GOOD
clientSimple :: IO ()

clientPipelined :: IO ()
-- GOOD
data IsPipelined = Simple | Pipelined

client :: IsPipelined -> IO ()

[PROPOSAL] Prefer ticked constructors in type signatures

For user-defined kinds, we want to use ticked constructors in type signatures.

Why

  • Clearly identifies that a data-type is being promoted as kind.
See Examples 
data FireForget msg where
  StIdle :: FireForget msg
  StDone :: FireForget msg

-- BAD
fireForgetClientPeer :: Peer (FireForget msg) AsClient StIdle m a
fireForgetClientPeer = ...

-- GOOD
fireForgetClientPeer :: Peer (FireForget msg) 'AsClient 'StIdle m a
fireForgetClientPeer = ...

[PROPOSAL] Use camel-case for names, including acronyms

Use camel-case for all identifiers, whether functions or types, including acronyms.

Why

  • Not-quite-camel-case identifiers introduce irregularity in names which might hamper interoperability (eg. generating snake-case identifiers from camel-case)
See Examples 
-- BAD
data UTXO = ...

data UTxO = ...

-- GOOD
data Utxo = ...

[DRAFT] Error handling

Distinguish between errors which are common and errors which are exceptional, e.g. validating something fails "as likely" as it will not, whereas sending a message over the network is expected to work usually.

Here are some condensed rules we try to follow:

Total functions

  • Try to avoid errors in the first place "by design", e.g. using smaller type like Natural or NonEmpty lists (Source)
  • Avoid error and it's derivates at all cost! Add a huge comment if you still need to use it.

All (pure) functions must be total so there's no use for error or panic except as a placeholder or Test Double to remind one there's more test to write. The only reason why one would use panic in production code is to satisfy the completeness checker of the compiler even though we know some case is impossible. In this case, it's then better to use Data.Void.absurd.

bar :: IO a
bar = threadDelay 1_000_000 >> bar

foo :: Int -> IO Int
foo x = pure (2 * x)

run :: Int -> IO ()
run x = do
  result <- race bar (foo x)
  case result of
    Left void_ -> absurd void_ -- there is no way `bar` can complete before `foo`
    Right a -> print a

Domain Errors

  • Use Either e a for expected errors (or even Maybe if there is only one error case)
  • Use m (Either e a) for the same reason in monadic code
  • We want to avoid ExceptT e IO and thus also in MonadIO m situations:

ExceptT is bad because:

  • It's inefficient
  • It makes code more complicated
  • It does add any form of safety as the undelrying IOs can still throw exceptions anyhow

Also, make sure e is a proper error type with relevant information, possibly even an instance Exception MyError. And have ToJSON and FromJSON instances to be able to shove the error's information in a properly formatted log trace.

And quoting Edward Yang:

canonicalizing errors that the libraries you are interoperating [with are exposing] is a good thing: it makes you think about what information you care about and how you want to present it to the user. You can always create a MyParsecError constructor which takes the parsec error verbatim, but for a really good user experience you should be considering each case individually.

Exceptions

  • Exceptional situations may be reported using MonadThrow m and Exception instances
  • Add documentation about what additional exceptions may be thrown (and when) by a function.
  • Use safe-exceptions to avoid confusion of synchronous and asynchronous exceptions.
  • Use bracket or ResourceT for resource management in presence of exceptions.
  • Make sure you do use throwIO and not throw if you are in the IO monad, since the former guarantees ordering; the latter, not necessarily.

Could we make use of checked exceptions package here? Seems like not very active but still maintained, yet not much discussed and advocated in the community.

See Examples 
-- BAD
takeN :: Integer -> [a] -> [a]

-- BAD
takeN :: Integer -> [a] -> Maybe [a]

-- GOOD
takeN :: Natural -> [a] -> [a]

-- GOOD
complexAlgorithm :: Input -> Either ValidationError Output

-- GOOD
complexAlgorithm :: MonadError ValidationError m => Input -> m Output

-- BAD
complexAlgorithm :: (MonadError ValidationError m, MonadIO m) => Input -> m Output

-- BAD
myFunction :: FilePath -> ExceptT MyException IO Handle

-- BAD
myFunction :: MonadIO m => FilePath -> ExceptT MyException m Handle

-- GOOD
-- | Does throw 'MyException', when ... (and any other type of 'Exception' really)
myFunction :: (MonadThrow m, MonadIO m) => String -> m Int

-- ???
-- Using something like checked exceptions
myFunction :: (Throws MyException m, Throws OtherException m, MonadThrow m, MonadIO m) => String -> m Int

References

[PROPOSAL] Constraints grouping

When a type declaration includes more than one constraints, use tuple notation instead of double-arrow notation.

Parenthesis are not needed when there is a single constraint.

See Examples 
-- BAD
processEffect ::
  MonadAsync m =>
  MonadTimer m =>
  MonadThrow m =>
  HydraNode tx m ->
  Tracer m (HydraNodeLog tx) ->
  Effect tx ->                                                                                                                 Generate signature comments
  m ()

-- GOOD
processEffect ::
  (MonadAsync m, MonadTimer m, MonadThrow m) =>
  HydraNode tx m ->
  Tracer m (HydraNodeLog tx) ->
  Effect tx ->                                                                                                                 Generate signature comments
  m ()

-- GOOD
processEffect ::                                                                                                               Generate signature comments
  ( MonadAsync m
  , MonadTimer m
  , MonadThrow m
  ) =>
  HydraNode tx m ->
  Tracer m (HydraNodeLog tx) ->
  Effect tx ->
  m ()
processEffect HydraNode{hn, oc, sendResponse, eq} tracer e = do

-- GOOD
createEventQueue :: MonadSTM m => m (EventQueue m e)                                                                               Unfold createEventQueue

[PROPOSAL] Use Red Bin to track and address "defects"

Use a Red Bin to quickly identify, analyse, track and resolve defects in the process.

  • When a defect is spotted, place a sticky in the Red bin area of our Miro board. A "defect" is anything that goes in our way: Inconsistencies in formatting, long build times, flacky tests, unclear names, ill understood code or tool...
  • If there's already a similar issue, add a 👍 emoji to the sticky
  • Work on the defect as soon as possible
  • optional: Link to an entry in Logbook explaining the issue and if possible analysing it: Why it matters, what possible solutions there are

Why

Identifying and resolving defects quickly and relentlessly is a key practice to improve quality of both the process and the product

Clone this wiki locally