Re: [Monotone-devel] Code refactoring

[graydon hoare]

Kevin Smith <address@hidden> writes:

> Last night, I started actually writing some monotone code. 

good to hear. got a depot I can subscribe to yet?

> Working just in the commands.cc module, I found some aspects of the
> existing code to be difficult to work with.

yeah, it's a personal style, but I've been pretty happy so far. what
aspect of it bothered you?

> How open are you (Graydon) to having me submit some refactoring
> patches to this module? 

oh, quite. so long as it seems to make things better, not just
refactor for the sake of abstraction. quality is a balance between
many forces (readability, simplicity, ease of editing, etc.) and
sometimes a few duplicated stanzas are better than extra abstractions.

it's all pretty subjective. post some concrete cases and if I actually
object to something we can talk it out. I suspect it'll be fine. I can
give you some simple style rules in advance, if you like:

  - avoid pointers at all cost
    - if you must use them, use a shared_ptr<> or cleanup_ptr<>

  - if there's a validity or encoding invariant on a string, express a
    check function for it in vocab_terms.hh and vocab.cc.

  - use simple libstdc++ objects (set, map, vector) whenever possible,
    don't wrap things in classes needlessly.
    - if a libstdc++ object gets complex, typedef it.

  - for all but the simplest datatypes (small strings, integers) pass
    parameters and return slots by reference, to functions. indicate
    return parameters by lack of const on the reference.

  - use I() invariants, N() usage needs, W() warnings, P() progress 
    indication and L() logging, liberally.

  - prefer an I() or N() shutdown over an ambiguous or unsafe
    operation. it's even ok to double-check things which "should be
    impossible". if it costs noticable performance, we can remove such
    a check later.

  - if you're a unit-test junkie, by all means put tests in the
    #ifdef BUILD_UNIT_TESTS guarded blocks at the bottom of files, or
    add such blocks to files which don't yet have any.

> [1] Refactoring will often result in more run-time method invocations
> and object creations, which theoretically could slow the app down. As
> long as I confine myself to commands.cc for now, this should have no
> real effect on performance, which I know is a concern for monotone.

eh, where it needs to be done it needs to be done. I'm not going to
make up some performance excuse about something I don't like. I'll
just say I don't like it :)

anyways, performance is best addressed by measurement and hard
numbers. it's usually hard to guess, just looking at a patch, what's
going to be a cost center.

-graydon