Recently I was asked some details about the ' Golang ' good code and the bad code, and I found the problem to be interesting and interesting enough to write an article on the issue. To illustrate my point, I gave some examples of my code in the air traffic control area of air traffic Management (' ATM '), which is hosted on [Github] (https://github.com/teivah/ Golang-good-code-bad-code). # # Background First, briefly describe the background of the project. The ' Eurocontrol ' (European air Management Organization) is an organization that manages air traffic in European countries. The common data exchange network between ' Eurocontrol ' and ' Air Navigation Service Provider ' (' ANSP ') is called ' AFTN '. This network is used primarily to exchange information in two formats: ' Adexp ' and ' ICAO ' (ICAO) format information. Each kind of information has its own syntax format, but semantically, more or less they are the same. In this context, performance must be a key factor in implementation. The project needs to provide two implementations based on the ' GO ' language for converting ' ADEXP ' format information (the ' ICAO ' format is not reflected in this comparison):-Bad implementation (' package ' pack name: [Ill] (https://github.com/ Teivah/golang-good-code-bad-code/tree/master/bad)-The implementation after refactoring (' package ' bundle name: [Good] (https://github.com/teivah/ Golang-good-code-bad-code/tree/master/good)-the ' adexp ' format information example can be in [here] (https://raw.githubusercontent.com/teivah/ Golang-good-code-bad-code/master/resources/tests/adexp.txt) found. In this contrast structure, in the ' adexp ' message, the parsing process is only a subset, so far it is still related to some of the errors common in ' Go '. # # Analysis In summary, a ' adexp ' message is a ' tokens ' collection, which can be any of the following:-Simple >-arcid ACA878 represents ' arcid ' (Flight ' ID ') is ' ACA878 '. -Circular >-eetfir ehaa 0853 >-eetfir Ebbu 0908 This example represents a ' fir ' (Flight information area) list, the first flight is ' Ehaa 0853 ', and the second ' Ebbu 0908 '. -Complex >-geo-geoid geo01-lattd 490000n-longtd 0500000W >-geo-geoid geo02-lattd 500000n-longtd 0400000W a cyclic ' t Okens ' list, each row contains a sub ' token ' list (in this case ' GEOID ', ' lattd ', ' LONGTD '). In combination with the project background, it is important to achieve a parallel execution of the conversion code. So there are the following algorithms:-a preprocessing process to clean up and rearrange the input (we need to clear possible spaces, rearrange multiple lines of ' tokens ' such as ' COMMENT ')-then split each line into a single thread. Each process will be responsible for processing one line of ' tokens ' and returning the result. -The last step is equally important, consolidating the results and returning a ' message ' to the struct, which is a public structure, whether it's a ' adexp ' or ' ICAO ' type of information. Each package contains a ' adexp.go ' file that exposes the main ' parseadexpmessage () ' method. # # Progressive Comparison Now let's take a look at the bad code I think, and how I refactor it. # # String Type vs []byte type restricting input type is not good for string type. ' Go ' provides strong support for the ' byte ' type of processing (such as the basic ' trim ', ' regexp ', etc.), and the input will be much similar to ' []byte ' (Given that ' AFTN ' information is received through the ' TCP ' protocol), there is actually no reason to force the required character The input of the string type. # # Error Handling error handling implementation is a bit bad. We will find that we neglect some of the possible errors that are returned in the second argument: ' gopreprocessed, _: = Preprocess (String) ' good implementation is to capture every possible error: ' ' gopreprocessed, err: = preprocess (bytes) If err! = Nil {return message{}, err} ' ' We can do this in the bad code belowcan also find some mistakes made: "' GOif len (in) = = 0 {return" ", FMT. Errorf ("Input is Empty")} "the first error is syntactically. Depending on the syntax of Go, the string for the error message is neither uppercase nor punctuation-terminated. The second error is if an error message is a simple string constant (no formatting required), use the lightweight ' errors. New () ' performance will be better. The good implementation is as follows: "' GOif len (in) = = 0 {return nil, errors. New ("Input is Empty")} ' # # Avoid nesting ' mapline () ' Functions is a good example of avoiding nested calls. The bad code is as follows: "' Gofunc mapline (msg *message, in string, ch Chan string) {if!startwith (in, stringcomment) {token, Value: = Pars Eline (IN) if token! = "" {f, contains: = factory[string (token)]if!contains {ch <-"OK"} else {data: = f (token, value) en Richmessage (msg, data) ch <-"OK"}} else {ch <-"OK" return}} else {ch <-"OK" return}} "" Instead, the good code is very clear: "' Gofunc ma PLine (in []byte, ch Chan interface{}) {//Filter empty lines and comment Linesif len (in) = = 0 | | Startwith (IN, bytescommen T) {ch <-nilreturn}token, Value: = ParseLine (in) if token = = Nil {ch <-nillog. WARNF ("Token name is empty on line%v", string (in)) Return}stoken: = String (Token) if f, contains: = Factory[stoken]; contains {ch <- F (Stoken, value) return}log. WARNF ("Token%v is not managed by the parser", "string", "ch <-nil}", which in my opinion makes the code more readable. In addition, this flattening processing should also be added to the error capture code, the following example: "' Goa, err: = F1 () if Err = = Nil {b, err: = F2 () if Err = = Nil {return B, nil} else {return Nil, err}} else {return nil, err} ' should be modified to: ' ' Goa, err: = F1 () if err! = Nil {return nil, err}b, err: = F2 () If Err = Nil {return nil, Err}return B, Nil ' Also, the second piece of code is more readable. # # Value or reference preprocessing method implementation is not good: "' Gofunc preprocess (container, error) {} '" in conjunction with the project background (considering performance), Given that the structure of a message is likely to be larger, the better way is to pass in the pointer within the ' container ' structure, otherwise, the value of ' container ' in the code of the example above will be overwritten each time it is called. Good implementation code will not have this problem because it is handled in a single way (a simple 24-byte structure regardless of what type of data). The "Func preprocess (in []byte) ([][]byte, error) {}" is more broadly said that whether the parameter is passed by reference or by value is not a language-accustomed usage. Passing data by value can also help to determine that a method will not have any side effects (just like passing data in the input of a function). There are several benefits to this, such as unit testing, refactoring on code concurrency (otherwise we need to check each sub-function to determine if delivery is complete) I'm sure that this writing needs to be carefully used in terms of the actual project background. # # Concurrency Bad implementations originate from an initial good idea: using a co-process to process data concurrently (one process handles a row). This led to repeated calls to ' mapline () ' In a co-thread. ' For I: = 0; I < Len (lines); i++ {Go mapline (&msg, lines[I], ch)} ' ' Mapline () ' method three parameters:-Returns a pointer to the last ' Message ' structure. This means that each ' mapline () ' will be populated with the same variable. -Current line-a ' channels ' channel is used to send a message when the line is completed in order to share ' message ' messages to pass a pointer against the GO basic principle:> do not communicate through shared memory, but should communicate to share memory pass shared variables have two main drawbacks:-Disadvantages #1: Splitting a consistent adornment because the structure contains some slices that can be modified simultaneously (by two or more concurrent processes) we have to deal with mutually exclusive issues. For example, the message structure contains a ' estdata []estdata ', and by adding another ' estdata ' modify this part must be handled as follows: ' ' Gomutexestdata.lock () for _, V: = Range Value {fl: = Extractflightlevel (V[SUBTOKENFL]) msg. Estdata = Append (msg. Estdata, Estdata{v[subtokenptid], V[subtokeneto], FL})}mutexestdata.unlock () "In fact, exclude specific usage, use ' mutex ' (mutex) in the process is not a good choice. -Disadvantage #2: It is not a good way for pseudo-sharing to share memory through a thread or a coprocessor because there may be pseudo-shares (a process in the ' CPU ' cache can be cached by another ' CPU '). This means that we need to avoid sharing variables that need to be modified by threading and the thread as much as possible. In this case, though, I don't think that pseudo-sharing has a big impact in cases where the input file teaches less (the performance test results of adding files in the ' message ' structure are more or less the same), but in my opinion it's also a very important point to keep in mind. Now let's take a look at the good Concurrency: "' gofor _, Line: = Range ' {Go mapline (lines, ch)} ' Now, ' mapline () ' accepts only two parameters:-Current line-' channel ', after the current row has been processed, The channel here is no longer simply used for messages, but also for transmitting actual results. This means that the final message structure should not be modified using a co-process. The results are consolidated in the parent's association. (The resulting ' mapline () ' method is called in the respective process) ' gomsg: = message{}for range in {data : = <-chswitch data. (type) {//Modify msg variable}} "This code is more consistent, in my opinion, according to the principle of ' Go ': to share memory through communication. Modifying message variables with a single thread prevents modifications and pseudo-sharing problems that may result from concurrency. The potential problem with this part of the code is that each line is caused by a single thread, and this implementation will be able to run, because the ' adexp ' information line will not be too large, and in this simple implementation, a request will produce a co-process that will not be considered in generating capacity. A better option is to create a co-pool multiplexing process. # # line processing notification in the bad code above, once ' mapline () ' finishes processing a line, we need to identify it in the parent's process. This section will be called by using the ' Chan string ' Channel and method call: ' ' Goch <-' OK ' because the parent process does not check the results of the channel passing through, the better processing is through ' Chan struct{} ' using ' ch <-struct{}{} ', or better choice (' GC ' will be worse) ' is handled by ' Chan interface{} ' using ' ch <-nil '. Another similar approach (even more concise in my opinion) is the use of ' sync '. Waitgroup ', because when each ' mapline () ' is executed, the parent's process still needs to run. # # If condition judgment in the ' Go ' conditional judgment statement, allow to be assigned before the condition. An improved version of the code: "' Gof, contains: = factory[string (token)]if contains {//Do something} ' ' is as follows: ' ' GOif f, contains: = Factory[stok EN]; Contains {//Do something} "" makes the code more readable. The other error that is made in the switch selection code is that the ' default ' option in ' switch ' is not set: ' ' ' GoSwitch simpletoken.token {case tokentitle:msg. Title = Valuecase tokenadep:msg. ADEP = Valuecase tokenaltnz:msg. Alternate = value//Other cases} ' if the developer can take all the circumstances into account, ' switch ' ' default ' is optional, but it would be better to capture a special situation like this. "' GOSWItch Simpletoken.token {case tokentitle:msg. Title = Valuecase tokenadep:msg. ADEP = Valuecase tokenaltnz:msg. Alternate = value//other cases Default:log. Errorf ("Unexpected token type%v", Simpletoken.token) return message{}, FMT. Errorf ("Unexpected token type%v", Simpletoken.token)} "handling the ' default ' option will help developers capture the ' bugs ' that may result from the development process. # # recursive ' Parsecomplexlines () ' is a method of parsing complex ' tokens ', which is used in bad code as a recursive process: ' ' Gofunc parsecomplexlines (in string, Currentmap map[ String]string, out []map[string]string] []map[string]string {match: = Regexpsubfield.find ([]byte (in)) if match = = nil { out = append (out, Currentmap) return out}sub: = string (Match) H, L: = ParseLine (sub) _, Contains: = Currentmap[string (h)]if C ontains {out = append (out, currentmap) Currentmap = Make (map[string]string)}currentmap[string (h)] = string (strings. Trim (L, Stringempty)) return Parsecomplexlines (In[len (sub):], Currentmap, out)} ' but ' Go ' does not support tail call optimization recursion, Good code uses an iterative algorithm to get the same result: "' Gofunc parsecomplextoken (token string, value []byte) interface{} {if value = = Nil {log. WarNF ("Empty value") return Complextoken{token, Nil}}var v []map[string]stringcurrentmap: = Make (map[string]string) Matches: = Regexpsubfield.findall (value,-1) for _, Sub: = Range matches {h, L: = ParseLine (sub) If _, contains: = Currentma P[string (h)]; Contains {v = append (V, currentmap) Currentmap = Make (map[string]string)}currentmap[string (h)] = string (bytes. Trim (L, stringempty))}v = append (V, currentmap) return Complextoken{token, v}} ' ' The second way of writing is better than the first in performance. # # Constant Management we need to manage a constant to differentiate between ' adexp ' and ' ICAO ' types of messages. The bad wording is as follows: "' goconst (adexptype = 0//TODO constanticaotype = 1) ' Instead, use ' Go ' ' iota ' (constant counter) to write more elegant code:" ' Goconst ( Adexptype = iotaicaotype) ' Output is consistent, but avoids possible errors. # # Receive method each transformation provides a function to determine whether each ' Message ' is related to the previous level (at least at level 350 above the route point) the bad code is as follows: ' ' ' Gofunc isupperlevel (M Message) bool {for _, R: = RA Nge m.routepoints {if r.flightlevel > Upperlevel {return true}}return false} ' means we have to put ' Message ' as the parameter of the function. Conversely, the good code is to provide a simple function as the recipient of ' Message '. "' Gofunc (M *message) isupperlevel () bool {for _, R: = Range M.routepoints {if R.flightlevel > Upperlevel {return true}}return false} ' the second method is preferable, we simply let the message implement a specific behavior. This is also the first step in using ' Go ' to implement the interface, and if one day we need to create another structure of the same behavior (' Isupperlevel () '), this part of the code does not need to be refactored (because ' Message ' has inherited the behavior). # # Comments This part of the question is obvious, but bad comments are seldom mentioned. On the other hand, I try to make good comments on real projects, even though I'm not the kind of person who will annotate every line, I still believe that it's important to at least write a comment on each function and the core of a complex function. For example: "go//Split every line in a goroutinefor _, line: = range in {Go mapline (line, ch)}msg: = message{}//Gather the Gor Outine Resultsfor Range in {//...} Providing an additional example in the comments will also provide a great help: ' go//Parse a line by returning the header (token name) and the value. Example:-comment TEST must returns COMMENT and TEST (in byte slices) func parseline (in []byte) ([]byte, []byte)} {///: .} An additional example of this is helpful for another developer to better understand the current project. Last but not least, according to the ' Go ' best practice, the package itself needs to be commented. "' Go/*package good is a library for parsing the ADEXP messages. An intermediate format Message was built by the Parser.*/package Good ' # # # log processing Another obvious problem is the lack of log processing. Because I don't like the standard log ' package ' packages that ' Go ' provides, I'm using a third-party log package called ' Logrus ' in this project. # # Go's FMT package ' go ' provides a powerful toolset for ' Go fmt '. RegretIs that we forget to use it. # # DDD ' DDD ' brings the concept of universal language to emphasize the importance of sharing languages among all projects (' Business experts ', ' dev ', ' testers ', etc.). This may not be measurable on this project, but it is also important to maintain a simple, compatible context structure of ' Message ', considering the maintainability of the entire project. # # Performance results in the ' i7-7700 4x 3.60Ghz ' hardware environment, I use benchmark to compare two kinds of code:-Good code: 60430 ns/op-Bad code: 45996 ns/op Bad code is more than 30% slower than good code. # # Conclusion In my opinion, it is very difficult to give a general definition of good code and bad code. In one environment, a piece of code can be thought of as good code, but in another environment it may be considered bad code. One obvious feature is that good code solves the problem well in a given environment, which is futile if the code is efficient but does not meet the requirements. At the same time, it is important for developers to consider the simplicity, maintainability, and efficiency of the code. The improvement of > performance is accompanied by an increase in code complexity. A good developer can find a balance in the above features in a given environment. As in ' DDD ', ' context ' is a solution.
via:https://teivah.io/blog/good-code-vs-bad-code-in-golang/
Author: teivah Harsanyi Translator: m1sery proofreading: polaris1119
This article by GCTT original compilation, go language Chinese network honor launches
This article was originally translated by GCTT and the Go Language Chinese network. Also want to join the ranks of translators, for open source to do some of their own contribution? Welcome to join Gctt!
Translation work and translations are published only for the purpose of learning and communication, translation work in accordance with the provisions of the CC-BY-NC-SA agreement, if our work has violated your interests, please contact us promptly.
Welcome to the CC-BY-NC-SA agreement, please mark and keep the original/translation link and author/translator information in the text.
The article only represents the author's knowledge and views, if there are different points of view, please line up downstairs to spit groove
704 Reads
