真枪实弹谈写代码

  程序员最头疼的是啥？Bug。那如何写代码才能降低Bug率呢，前辈们已经总结出了各种方法，比较出名的有：设计模式六大原则、Rob Pike六项原则、Unix哲学17条原则、KISS、DRY、Python之禅、宽进严出等等等等，还有各个公司自己的技术栈和编码规则。俗话说的好，道理懂了这么多，为什么还是会写出Bug？孔子曰：“学而不思则惘，思而不学则殆。”道理虽懂，还需实践。

  每项原则具体代表什么例子，请自行搜索。

  插播一条，小生觉得，中国人写代码，应该是不会出问题的才对。古人云：“各人自扫门前雪，莫管他人瓦上霜。”每个人，每个模块，每行代码负责好自己的事情，整体是个和谐的，划分清晰明了，不会错乱的系统。呵呵，开个玩笑。

  下面直接进入例子。

例子1：（实验楼楼赛第15期第1题）

题意说明：算出http请求body的md5值，加到header里(X-Md5)，如果没有body，则不加X-Md5。

题意很简单，就是计算http.body的md5，先看实验楼参考答案：

func (t *Transport) RoundTrip(req *http.Request) (*http.Response, error) {

    // do something
    if req.Body == nil {
        return t.RoundTripper.RoundTrip(req)
    }

    b, err := ioutil.ReadAll(req.Body)
    req.Body.Close()
    if err != nil {
        return nil, err
    }

    if len(b) ==0 {
       return t.RoundTripper.RoundTrip(req)
    }

    m := md5.Sum(b)
    req.Header.Set("X-Md5", hex.EncodeToString(m[:]))

    // 由于 ioutil.ReadAll 方法会读取到 EOF，所以需要重置 Body
    req.Body = ioutil.NopCloser(bytes.NewBuffer(b))

    return t.RoundTripper.RoundTrip(req)
}

  代码最后的结果肯定是对的，但这样写真的好吗？有没有感觉作者写的太着急了？从md5.Sum(b)分割，上面一直在操作req.Body，结果req.Body都还没重置呢，就急不可奈的要求md5了，这其实属于“先求结果，慢慢补锅”类型的代码，不提倡，小生更提倡的是，结果固然重要，但不要着急，先把屁股擦干净了，不要隔着长长的一大段业务逻辑填前面的坑。

接下来我们仔细看，先看3个if判断：

1. 第1个(第4行)判断req.Body是空，属正常情况，return;
2. 第2个(第10行)判断读取req.Body有没有出错，属异常情况，return;
3. 第3个(第14行)判断http.Body是空，属正常情况，return。

  这里的“正常情况”是指，一个http请求，没有body是很正常的，比如GET请求。

  这3个return处，所属情况是不一样的，第1、3处属同一类情况，都是没有请求体http.body，return是一样的。但第2处属异常情况，再加上最后第4个return，我们看到该函数的所有出口处分别为：正常、异常、正常、正常。是不是感觉很奇怪？我们知道，对于一套逻辑，比较好的一种处理流程应该是：异常、异常、异常……正常。所以，小生改写如下：

func (t *Transport) RoundTrip(req *http.Request) (*http.Response, error) {
    if req.Body == nil {
        req.Body = http.NoBody
    }
    buf := &bytes.Buffer{}
    hs := md5.New()
    n, err := io.Copy(hs, io.TeeReader(req.Body, buf))
    req.Body.Close()
    if err != nil {
        return nil, err
    }
    req.Body = ioutil.NopCloser(buf)

    if n > 0 {
        req.Header.Add("X-Md5", hex.EncodeToString(hs.Sum(nil)))
    }
    return t.RoundTripper.RoundTrip(req)
}

  判断条件if数量并没有减少，处理流程也一样，但return只剩两个了，按顺序为：异常、正常。

  分析一下，第3行，将req.Body为空的情况和len(req.Body)==0的情况合并了；

  第7行，该函数的主要目的是求md5，所以将主要目的md5写在了io.Copy(dst, src)中的dst处，而原body不能丢，便将buf放在src处作为原req.Body的备份；

  第12行，按逻辑对称的方式，先把上面req.Body的问题扫尾解决完；

  最后留下真正要做的逻辑放在最后4行。

  小生的写法不一定是最好，但基本做到了逻辑的通用化处理。须知，if判断越多，人理解起来越麻烦，需要记忆辅助的理解也会越多。通用化处理的好处是：第1处if将http.body为空的情况合并，在读下面代码的时候，不需要记忆目前http.body的状态，可以随时拿来就用；io.Copy读http.body过程出错返回是必不可少了，不必多说；最后只留下真正的逻辑求md5放在最后，符合了从异常到正常流程的处理。

---

例子2：

共用redis添加前缀，用于区分不同应用。

class RedisProxy:
    _cmds = ('get', 'set', 'incr')
    def __init__(self, prefix, client):
        self._prefix = prefix
        self._client = client
    def __getattr__(self, cmd):
        def inner(*args):
            if cmd in self._cmds:
                if len(args) > 0:
                    ls = list(args)
                    ls[0] = self._prefix + ls[0]
                    args = tuple(ls)
            return self._client.__getattr__(cmd)(*args)
        return inner

单例模式

foo = RedisProxy('foo_', RedisClient())

  这样写对吗？是对的，调用的时候，如foo.set('key', 'value')，自动转成set foo_key value，很方便，很简单，业务逻辑不用分心关注前缀问题。

  但这种封装方式是不对的，首先，RedisProxy它的主要作用是什么？只是加个前缀，那它本身承载了多少东西？一个prefix必须知道的，client是必须的吗？cmds是必须的吗？

  那小生会修改如下：

_prefix = 'foo_'

def format(key):
    return _prefix + key

def trim(key):
    return key[len(_prefix):] if key[:len(_prefix)] == _prefix else key

  是的，只负责拼接前缀，其余的概不负责，client不维护，哪些命令要加前缀也不管，由业务自行决定。想加的调用format，不想加的不调用。这样做，一是方便测试（呃，貌似也不用测试了），二是不需要知道redis有哪些命令，也不需要知道哪些命令要加前缀，三是不需要维护client状态。所有的所有，由业务自己决定。如果业务方出错了，只影响单一的业务，如果封装的多出错了，那基本上整个应用都会有问题。

  现在用起来可能麻烦一点，比如：client.set(format(‘key’), ‘value’)。但一般是可以通过封装一个redis key解析专用模块的，该专用模块用于解析redis命令，找出命令中的key、返回值中的key等信息。

  一般应用代码可以分为几类：底层、三方库、业务逻辑、存储逻辑、功能模块（分为专用和通用）等，底层和三方库一般不会动，业务逻辑和存储逻辑一般会解耦，功能模块则处于一个比较尴尬的位置。专用功能模块分为业务专用模块和组件专用模块，业务专用模块和业务挂钩，一般通用性较差，主要适用于当前应用；组件专用模块和专用组件挂钩，对不同组件间不通用，但对不同应用间是通用的。通用功能模块则属于多应用可通用的模块，分为功能通用和工具能用。

  将需要码农开发的模块（这里不包括底层和三方库）进行分级，按顺序依次为：1.业务逻辑、2.存储逻辑、3.业务专用、4.组件专用、5.功能通用、6.工具通用。不同模块的组合，生成序号最小者模块，比如：存储逻辑+组件专用->存储逻辑，业务专用+能用功能->业务专用。提高通用性的方法一般是：对接口编程。写代码时分析清自己所在级别，目标级别，一级一级逼近，一般写出来的代码都不会太差。

  比如上面例子，如果想做一个通用的RedisProxy，它需要是三个不同功能的合集：redis client管理模块、redis key解析模块、和前缀添加删除模块。

伪代码如下：

class RedisProxy:
    def __init__(self, client, cmd_parser, key_formater):
        self._client = client
        self._cmd_parser = cmd_parser
        self._key_formater = key_formater
    def _format_key(typ, val):
        if typ == REDIS_KEY:
            return self._key_formater.format(val)
        return val
    def _trim_key(typ, val):
        if typ == REDIS_KEY:
            return self._key_formater.trim(val)
        return val

    def __getattr__(self, cmd):
        def inner(*args):
            if self._client.state != CONNECT:
                self._client.reconnect()
            args = self._cmd_parser.command(cmd, args, self._format_key)
            resp = self._client.__getattr__(cmd)(*args)
            return self._cmd_parser.response(cmd, resp, self._trim_key)
        return inner

  这样，在用的时候，可以将proxy.set('key', 'val')翻译成set key val，再改为set prefix_key val到redis执行；将proxy.scan(0)得到的结果(0,('prefix_key'))改为(0, (‘key’))。这样进去出来的结果是匹配的，就像数据结构里的栈，pop(push(x)) == x。再看RedisProxy，它的组成模块中，client和cmd_parser属于组件专用模块，key_formater属于业务专用模块（因为每个应用对键的格式化是不同的），由三个模块组合成了一个最终的业务专用模块。

---

例子3：

解析形如"a:123,b:456"，得到a=123和b=456。

不要把这个问题想复杂，只是个简单的字符串切割问题，你会怎么切？

s = 'a=123,b=456'
ls = s.split(',b=')
a = int(ls[0][2:])
b = int(ls[1])
print(a, b)

  这样切对不对呢？从结果上来说是对的，但程序是“侵权”的，split的任务是按,切成n份，下面每份要干什么，怎么干，都不是split该关心的事。意思是，每行代码，搞清楚它要做的事，不要越权，“莫管他人瓦上霜”。上面代码，split和下面两行代码之间存在着强耦和，另外一个问题是，它不是按规则切的，切割出来的结果存在着不对称性，以对称为美的社会，这样的代码也是不美观的。修改如下：

s = 'a=123,b=456'
ls = s.split(',')

d = {}
for t in ls:
    kv = t.split('=')
    d[kv[0]] = kv[1]

a = int(d['a'])
b = int(d['b'])
print(a, b)

  这样切，每步操作都是在“各扫门前雪”，不会和上下文产生强耦合，而且操作都是对称的。

---

例子4：

在Golang中，每个http api返回的数据，在结构上都是大体一样的，所以基本上每个项目的入口处都要封装自己的消息返回格式，比如：

type Response struct {
        Code int         `json:"code"`
        Msg  string      `json:"msg"`
        Data interface{} `json:"data"`
}

type JsonRender struct {
        RespWriter http.ResponseWriter
}

func (this *JsonRender) RenderErr(code int) {
        json.NewEncoder(this.RespWriter).Encode(&Response{
                Code: code,
                Msg:  errcode.Text(code),
                Data: "",
        })
}

func (this *JsonRender) RenderOK(data interface{}) {
        json.NewEncoder(this.RespWriter).Encode(&Response{
                Code: 0,
                Msg:  "",
                Data: data,
        })
}

  这样封装，固然是达到了目的，但这种封装方式对吗？

  首先，我们要先问这样一个问题？我们封装返回值的目的是什么？很明显，不用外界关心以何种编码方式返回数据。所以第一点，Response不应该导出，应该改为type response struct{...}。

  第二，我们再看JsonRender的作用，只是封装了一个http.ResponseWriter，而且也是导出字段；两个方法很简短，直接往http.ResponseWriter里面写json数据；这里不禁要问了：这个封装有什么存在的意义？是方便使用了，还是屏蔽细节了？好像两者都没有。所以这个封装毫无意义可言。

  最基本的修改如下：

type response struct {
        Code int         `json:"code"`
        Msg  string      `json:"msg"`
        Data interface{} `json:"data,omitempty"`
}

func render(w http.ResponseWriter, code int, msg string, data interface{}) {
        w.Header().Set("Content-Type", "application/json; charset=utf8")
        json.NewEncoder(this.RespWriter).Encode(&Response{
                Code: code,
                Msg:  msg,
                Data: data,
        })
}

func RenderOK(w http.ResponseWriter, data interface{}) {
        render(w, 0, "", data)
}

func RenderErr(w http.ResponseWriter, code int) {
        render(w, code, errcode.Text(code), nil)
}

  只暴露RenderOK和RenderErr，里面具体内容不外泄。

---

例子5：

变量作用域和通道关闭操作

func handleConn(conn net.Conn) {
        const QueueSize = 10

        mq := make(chan string, QueueSize)
        defer close(mq)

        go func() {
                for {
                        select {
                        case msg, ok := <-mq:
                                if !ok {
                                        log.Printf("debug: msg queue closed")
                                        return
                                }
                                _ = msg
                                // some logic to process msg
                        }
                }
        }()

        var pkt []byte
        var err error
        for {
                pkt, err = read(conn)
                if err != nil {
                        return
                }

                go func() {
                        msg := parsePacket(pkt)
                        mq <- msg
                }()
        }
}

  上面代码有两个BUG，变量作用域这个BUG比较容易发现，pkt和err定义在了for循环外面，那么就要求，for循环里面不能有goroutine闭包，其中err没出现在闭包里，但pkt出现在了闭包中，并且是go func中，那这里肯定是个BUG了。

  close(chan)这个BUG好多人觉得不容易发现，但其实这个BUG应该是一眼就看出来的，为什么？因为mq的定义和defer close(mq)离得非常近，看到close操作就该找是谁在写mq。正常逻辑是：哪个协程在写chan，就该由哪个协程close(chan)。由此，我们看到close，马上找谁在写，很明显在函数最后面的go func中，这就扎眼了，写chan的和close(chan)没在同一协程里，这肯定是要出BUG的。

  修正这两个BUG也非常容易：

func handleConn(conn net.Conn) {
        const QueueSize = 10

        mq := make(chan string, QueueSize)
        wg := &sync.WaitGroup{}
        defer func() {
                wg.Wait()
                close(mq)
        }()

        go func() {
                for {
                        select {
                        case msg, ok := <-mq:
                                if !ok {
                                        log.Printf("debug: msg queue closed")
                                        return
                                }
                                _ = msg
                                // some logic to process msg
                        }
                }
        }()

        for {
                pkt, err := read(conn)
                if err != nil {
                        return
                }

                wg.Add(1)
                go func() {
                        defer wg.Done()
                        msg := parsePacket(pkt)
                        mq <- msg
                }()
        }
}

  将pkt和err放到for循环里面去定义；写mq的有多个协程，那就等所有写协程都退出再close(chan)。

  那有人可能会问，即使加了sync.WaitGroup，写chan协程和关闭chan协程仍然不是同一个啊，那是不是还有BUG？不会，用sync.WaitGroup后，可以理解为将所有的写操作都转移到了当前协程中，这样就相当于写和关闭在同一协程中了。

---

例子6：

关闭http.Response.Body

resp, err := http.Get("http://example.com")
if err != nil {
        // handle err
}
body, err := ioutil.ReadAll(resp.Body)
if err != nil {
        // handle err
}
// use body
_ = body

  对于经常写go程序的人来说，上面这段代码BUG非常好找，少了一句defer resp.Body.Close()，但在这里，我不是要说这个BUG，而是面对这段有问题的代码时，该怎么尽快找到BUG并解决它。

  看看以下三篇文章是怎么发现并解决这个问题的吧：
  看看这篇拯救发际线的干货吧–警惕 Go 编程陷阱
  Go http request 引起的 goroutine 泄漏
  Go Http包解析：为什么需要response.Body.Close()

  这种分析问题的方式，对吗？完全不对！写代码前，一定要看清楚http.Response.Body的类型，net/http/response.go里面写着：

type Response struct {
        // ...
        Body io.ReadCloser
        // ...
}

  Body的类型是io.ReadCloser，ReadCloser的用法很简单：Read完Close，这么简单的规则，根本范不着读源代码。即便说看到类型，不确定这样用对不对，再看注释也就明白了。注释里写道：It is the caller's responsibility to close Body.这样就很明显了，读完关闭。

  所以，写代码一定要遵循规则，出了问题也不一定非要分析源代码才能解决。

---

囿于例子不太好找有代表性的，故而以后有发现随时补充。