Harvest after Codereview

Abstract

I just entered the company and developed two pieces of code. I met the boss who was in charge of it. I started standard PR and Codereview and learned some things.

Naming conventions and presentation issues

This should be modified more of the problem, to cite a few more typical chestnut:

1. Express questions

In fact, I did not find something in the database

throw new Error('Did not find the package whose ID is 2.')
Copy the code

D: Garbage: L

• D: Did your PE teacher teach you English? Where did you write that…
• L: wow! My English was taught by my PE teacher, so I translated it on the Internet.
• D: I’m afraid it’s a fake translation app.
• L: Google Translate has pictures and truth!

• D: You are not learning Chinese well.

No matter I am not good at English or Chinese, I do not think much about it and I do not have the craftsman spirit to keep improving. I’m sure no one can say that I’m wrong, but the truth is that it’s not good.

2. API design specification

// bad
get('/package/package_with_app_version? appid=xxx&version=xxx');
// good
get('/package? appid=xxx&version=xxx');
Copy the code

After checking the API design specifications, all of them prefer RESTful API style.

Actions are described using HTTP Method

Method	Corresponding database behavior	describe
GET	SELECT	Fetching resources from the server
POST	CREATE	Create a new resource on the server
PUT	UPDATE	Update the resource on the server (the client provides the full resource after the change)
PATCH	UPDATE	Update resources on the server (client provides changed properties)
DELETE	DELETE	Deletes resources from the server

Example:

// Get the package list
get('/package');
// Get the package with ID 12
get('/package/12');
// Strictly RESTFUL, the return list should be filtered by these two parameters
// But I think it is not suitable in the business, so I change it according to the business, directly return the corresponding package
get('/package? appid=xxx&version=xxx');
// If you want to be strict with the specification, you should write it like this
get('/app/:appid/package/:version')
// Specify the page number and the number of records per page.
get('/package? page=2&per_page=100');
// Update package, the client provides all information about the package, similar to replacement
put('/package/:id');
// Update package. The client provides information that needs to be updated
patch('/package/:id');
/ / delete the package
delete('/package/:id');
/ / create a package
post('/package');
Copy the code

More formal design API, you can see at a glance what the API will do.

Code error

1. Problems occurred when reading the file splicing buffer

The following is the wrong way to write, the big guy looked at the wrong, let change. I was so frustrated that it took me a long time to experiment and finally it turned out that my notation really does get garbled when I parse it. Post code

const fs = require('fs');
let rs = fs.createReadStream('./Zn-utf8.txt');
let body = ' ';
rs.on("data", (chunk) => {
  body += chunk;
});
rs.on("end", () = > {console.log(body);
});
Copy the code

It’s really garbled!

Error: Here really appear garbled code, the following analysis of the reason.

An English character takes up one byte, while a Chinese character takes up three bytes. When the file is large and needs to be cut.

Assumption: every 17 characters are cut once in reading

'when a three bytes of Chinese characters' = = >' a (1, 2, 3) (4 and 6) han (7,8,9) words (10) of the final three three (13,14,15) (16) as word (verse 19,20,21) (22,23,24) '= = > chunk1:' a three Chinese characters? ? '+ chunk2:'? Byte 'Copy the code

However, in the above code, the implicit toString() appears when the string is spliced, which converts the incomplete chunk into a string, resulting in the incorrect parsing of incomplete Chinese characters and garbled characters.

That’s why you get those three garbled characters, right

Solutions:

let body = [];
rs.on('data', (chunk) => {
  body.push(chunk);
});
rs.on('end', () => {
  body = Buffer.concat(body).toString();
  console.log(body);
});
Copy the code

2. Use the timer to check the status

let status = await checkStatus();
if (status === 'success' || status === 'faild') {
  if (status === 'success') {
    console.log('success! I'm gonna do whatever I have to do. ')}else if (status === 'faild') {
    console.error('failure! I can't do anything I want. '); }}else {
  this.statusTimer = setInterval(async () => {
    status = await checkStatus();
    if (status === 'success' || status === 'faild') {
      clearInterval(this.statusTimer);
      if (status === 'success') {
        console.log('success! I'm gonna do whatever I have to do. ')}else if (status === 'faild') {
        console.error('failure! I can't do anything I want. '); }}},200);
}
Copy the code

CheckStatus (); faild; pending; checkStatus(); If the state is pending, a setInterval is turned on and checked every 200 milliseconds until the result is generated.

It sounds like there is no problem, AND I also thought so at first, but the result is really beyond my expectation, because in the local debugging, the request time is too short to find this problem, the big guy can see it at a glance, put a piece of code and run the result, we can see at a glance.

// The concept version of the above logic
var timer = setInterval(async() = > {console.log('What to do before sending the request')
    await new Promise(resolve= > setTimeout(resolve, 5000));
    console.log('Something to do after receiving a request');
 }, 1000)
Copy the code

To execute a function every second, await will block the content after the function for 5 seconds, clear the timer with clearInterval, meaning that the desired result has been received and the function will not be executed again. However, there were five other ‘things to do after receiving a request’ outputs.

That is to say, when the network situation is not good, my previous code will cause, after checking the status is successful, after clearing the timer, it is likely that there are still several functions not completed, will check the status is successful, trigger the successful thing to do, resulting in serious consequences.

Optimization scheme, solve the problem by recursion +sleep function:

Sleep is also used to reduce invalid requests, and the main problem is recursion.

async function checkStatus() {
  const status = await checkStatus();
  if (status === 'success' || status === 'faild') {
    if (status === 'success') {
      console.log('success! I'm gonna do whatever I have to do. ')}else if (status === 'faild') {
      console.error('failure! I can't do anything I want. '); }}else {
    console.log('pending... ');
    await new Promise(resolve= > setTimeout(resolve, 500));
    awaitcheckStatus(); }}Copy the code

Perfect problem solving

Exception handling

I was checked twice by the boss about exception catching:

1. Your requests are not tried… How do you catch the wrong one?
2. I changed after >>>>>> you how everywhere try… Catch says, was it fun?

So I silently researched where to catch exceptions, when not to catch exceptions, where to catch exceptions, and whether every request should be tried… catch?

I know that there are many better capture modes. For example, I particularly like the exception handling of egg. Errors in the Service layer controller layer are wrapped, thrown out, and processed uniformly through middleware.

• Q: When was it not captured?
• A: In JS, trycatch is useless for asynchronous callback functions. If you can’t catch it, then the native uncaughtException catches all uncaught errors, at which point you can’t do proper error handling.
• Q: How should asynchronous functions be captured?
• A: Basically, as long as you don’t jump out of the asynchronous call chain, you can catch it. There are many solutions, such as using promise to catch asynchronous errors, reject. Or with async await, you can catch exceptions easily.
• Q: Should exceptions be caught everywhere?
• A: Of course not. Exceptions thrown in the inner layer will be caught in the process of rising, so there is no need to write everywhere. Throwing exceptions, printing errors when you feel you should, and catching errors when you need to handle them is a scale THAT I probably need more experience with.

I have learned something about this, but it is definitely far from enough. Let’s accumulate experience of this piece slowly. But now you can at least use the minimum try… Catch, take care of all the mistakes.

Learn to put yourself in the user’s shoes

It’s not about code, it’s not about programs, it’s not about logic, it’s about perspective.

The following is the design of CLI commands during development

The first edition

# build locally
seeder-package [packageName] -o ./dist/package.tgz
# Deploy test environment (go live automatically)
seeder-package [packageName] -d ./dist/package.tgz
# Deploy the production environment (manual online)
seeder-package [packageName] -d ./dist/package.tgz -e production
# Update version number
seeder-package [packageName] --pv major
seeder-package [packageName] --pv minor
seeder-package [packageName] --pv patch
seeder-package [packageName] --pv 3.2.5
Check the current version number
seeder-package [packageName] --pv current
Copy the code

The second edition

# build locally
seeder [packageName] -o ./dist/package.tgz
Deploy the package at the specified path to the test environment
seeder [packageName] -d ./dist/package.tgz
Package and deploy to the test environment
seeder [packageName] -d -o
Pull the latest version of the package from the test environment to the production environment
seeder [packageName] -d --prod
# Update package deployment test live
seeder [packageName] -o -d --pv [xxx]
Short for update package Deployment Test go live
seeder [packageName]
Copy the code

The third edition

Seeder [packageName] # Package and update patch version seeder [packageName] # Package and update patch version seeder [packageName] --pv [XXX] # Sync to production seeder [packageName] -pCopy the code

The three versions are a huge change

First edition ==> second edition

This change was something I wanted to do on my own, as I felt during development that the usability was so poor that I had to input too much at a time. In general, this change is designed to reduce the amount of content that users type.

• Shorten the Seeder-package directive to seeder
• The deployment environment will need to be actively declared. The default value of the -e environment is the test environment and only needs to be set for production environment.
• Merge the package deployment two commands without passing long parameters.
• Instructions like -o-d –pv [XXX] can do several jobs at once.
• I made the shorthand

Second edition ==> Third edition

Both the conflict and the gain occurred at this time.

---

-o output local files -d deployment — PROd describes the environment — PV version related operations, one command one action, very clear. Do what you want to do with the instructions, should not be simplified, the logic is very clear. I’ve eliminated all the input items that I could have left out.

---

Big guy’s point of view: Simplify again, simplify to the extreme (so that I think the command action description is not clear), big guy thinks I said my clear logic is for developers, not from a technical point of view, think it can be implemented logically. I’ve been lying to myself about being logical when IT turns out I’m doing it myself. (Take a tumble)

---

A third overhaul simplified the order to the point where it was indispensable. These two changes to the code are the biggest, but they are also the biggest gains I made this time. I learned what is humanization and what is thinking from the perspective of users, forgetting that I am a developer.

Thank you

In the past, I always felt that CodereView was useless, but this time I gained a lot. There were still many business problems that were not easy to summarize, and many problems without characteristics were not listed. In this development, I also learned to write test cases.

Sometimes you don’t need to code to fulfill a requirement. It’s more like polishing a craft, and in the spirit of craftsman, your attitude determines the quality of your code. Thanks to the big guy to teach their own, I believe that the future will also learn a lot, their code quality will be dawdling to improve.

---

From a child who has been on the job for two weeks

I want to share this with you and I want to progress with you and this code is very comfortable to tap