0

Using node.js I'm creating a function to update a file that contains a JSON list by appending a new element to the list. The updated list is rewritten back to the file. If the file doesn't exist I create it.

Below, __list_append(..) does the list append and file update.

My question is if (and should) I can restructure this code to not have two calls to __list_append. I'm a bit new to node.js, and don't have a good feel for the asynchronous tactics.

function list_append(filename, doc) {
  fs.exists(filename, function(exists) {
    if (exists) {
      fs.readFile(filename, function(err, data) {
        if (err)
          throw err;
        __list_append(filename, JSON.parse(data), doc);
      });
    } else
      __list_append(filename, [], doc);
  });
}
4

3 回答 3

2

It's easy to get a bit pedantic with "best practices," but when I'm writing code and I get a gut feeling that something's not right or that something could be changed, I go over some well known best practices and attempt to see if the code I'm writing adheres to them. SOLID, while being principles of object oriented programming, can be useful to think about in other contexts. In this case, it seems to me that the function is violating the Single Responsibility Principle:

One of the most foundational principles of good design is:

Gather together those things that change for the same reason, and separate those things that change for different reasons.

This principle is often known as the Single Responsibility Principle or SRP. In short, it says that a subsystem, module, class, or even a function, should not have more than one reason to change.

(This could perhaps be exchanged for Separation of Concerns or other similar principles for this example, but the concept is the same.)

In this case, the function has two responsibilities: (1) getting the current (or default) list associated with a filename, and (2) appending data to said list. A first pass at separating these concerns might look something like this:

function get_current_list(filename, callback) {
  fs.exists(filename, function(exists) {
    if (exists) {
      fs.readFile(filename, function(err, data) {
        if (err)
          return callback(err);
        callback(null, JSON.parse(data));
      });
    } else
      callback(null, []);
  });
}

function list_append(filename, doc) {
  get_current_list(filename, function(err, list) {
    if(err) throw err;
    __list_append(filename, list, doc);
  });
}

Now, get_current_list is only responsible for getting the current list in a file (or an empty array if there is no file), and __list_append is (assumed to be) only responsible for appending to it; list_append is now a simple integration point between these two functions. The functions are a bit more reusable and can also be tested more easily (as an aside, a test-first or TDD approach to programming can help you notice these kinds of things up front). Furthermore, repeating callback in get_current_list is quite a bit more generic than repeating __list_append; if you need to change __list_append to something else, it now is only called in one place.

于 2012-07-24T19:18:29.317 回答
0

This case always feels unsatisfying to me because yes, you do have to repeat your call to __list_append on both branches because only one of the branches is synchronous.

于 2012-07-24T18:43:24.347 回答
0

I like and up voted Brandon's answer, but this also works: 

function list_append(filename, doc) {
  fs.exists(filename, function(exists) {
    var data = [];
    if (exists) {
      data = fs.readFileSync(filename, "utf8");
    }
    __list_append(filename, data, doc);
  });
}

于 2012-07-24T19:49:26.400 回答