类优化的不错例子

全局变量注入函数，函数方法放入类，用对象调用，最后包装给用户。都有详细的例子。

Initial Version: What a mess!

Please have a look through the source code for version 1. This game works (in most browsers, although it’s probably the worst game of all time), but it has a lot of code smells. Here are just a few:

• Everything is in the global namespace. This is considered pollution.

• There is an overlong method, “render”, that does pretty much everything – it has multiple responsibilities

• Magic numbers are used throughout

• Code is duplicated

• Intention is often unclear

• Variable naming is inconsistent (e.g. “x” and “duckPosX” do similar things for different entities)

• No encapsulation – no objects are used and arbitrary variables track everything

In a small example like this, you might get away with it, but this is only a very simple game. Assuming the plan is to grow it into a Duck Hunt clone; if these smells are allowed to stick around they’ll stink out the whole development. It’s what the Pragmatic Programmers refer to as the “Broken Windows Problem”: one broken window by itself means little, but it inspires others to break windows. The “broken windows” in our line of work (such as the code smells we’ve identified) mean a codebase falls gradually into disrepair – it gives people a sense that they can do as they please, take shortcuts, and generally contribute to the mess rather than work in a considerate, polished manner.

Let’s sort it out now before anyone sees it!

Version 2: Replace Magic Number

Having magic numbers in the code obscures your intention – it’s better to always be explicit, so we apply the “replace magic number with symbolic constant” all over the code.

For example, we change the shot detection code from:

function ev_mouseclick(ev) {

var clickX = ev.layerX;

var clickY = ev.layerY;

 

if ( Math.abs(clickX - duckPosX) < 10

    && Math.abs(clickY - duckPosY) < 10) {

duckHit = true;

}

}

To:

var SIGHT_SHOT_RADIUS_PX = 10;

// ...

 

function ev_mouseclick(ev) {

var clickX = ev.layerX;

var clickY = ev.layerY;

 

if ( Math.abs(clickX - duckPosX) < SIGHT_SHOT_RADIUS_PX

    && Math.abs(clickY - duckPosY) < SIGHT_SHOT_RADIUS_PX) {

duckHit = true;

}

}

Here, we have a constant, SIGHT_SHOT_RADIUS_PX, which has meaning, rather than the arbitrary 10.

View this version [SOURCE]. Diffing with the previous version [SOURCE] will highlight all changes.

Version 3 – Be Consistent

It is important to standardise on indentation, spacing and layout. Even if it isn’t your personal preference, stick with the project standard – otherwise, when people come to read your code, it looks disjointed. Our whitespace was a little off in the ev_mousemove function, so we fix it in version 3. We fix indentation and the spacing around operators. Here is an example:

Before:

function ev_mousemove (ev) {

..........x=ev.layerX;

..........y=ev.layerY;

}

 

function ev_mouseclick(ev) {

............var clickX = ev.layerX;

After:

function ev_mousemove (ev) {

............x = ev.layerX;

............y = ev.layerY;

}

 

function ev_mouseclick(ev) {

............var clickX = ev.layerX;

View this version [SOURCE]. Diffing with the previous version [SOURCE] will highlight all changes.

 

Version 4 – Extract Class

Part of the beauty of object oriented programming is bundling things up – encapsulating them. In the previous versions, we’ve just got a function and some variables running the whole show. In version 4, we identify classes that are “trying to get out”.

Because Positions are used in several places in the game, it makes sense to extract this into a simple object. This gives an object’s position context as well as being bundled up – the idea of position having an X and Y component is easy to grasp, and it makes a natural attribute of other objects.

function Position(x, y) {

this.x = x;

this.y = y;

}

We also extract the duck into its own object, bundling up all its properties using the “extract class” refactoring. Notice the duck now “has a” position, so we’ve actually done two refactorings in this stage, the second being “Replace Data Value with Object”.

var duck = {

DROP_SPEED_PX_PER_FRAME: 7,

FLY_SPEED_PX_PER_FRAME: 4,

MAX_Y_PX: 100,

MIN_Y_PX: 40,

WING_WIDTH_PX: 40,

WING_HEIGHT_PX: 30,

 

position: null,

frame: 0,

 

isGoingDown: false,

isHit: false,

 

init: function() {

this.position = new Position(40, 40);

}

};

 

View this version [SOURCE]. Diffing with the previous version [SOURCE] will highlight all changes.

 

Version 5 – Extract Method

We have the duck extracted, but the render() method still seems awfully interested in what the duck is doing – it’s handling the duck’s movement. We call this the “Feature envy” code smell. The duck should probably know how to move itself, so we move the code into duck itself, and just tell the duck to move. This decouples the render method from knowing how the duck moves.

var duck = {

// ... see version5 for full code ... //

 

move: function () {

if(this.hit) {

this.position.y += duck.DROP_SPEED_PX_PER_FRAME;

} else {

if(this.goingDown) {

this.position.x += this.FLY_SPEED_PX_PER_FRAME;

this.position.y += this.FLY_SPEED_PX_PER_FRAME;

if (duck.position.y > this.MAX_Y_PX) {

this.goingDown = false;

}

} else {

this.position.x -= this.FLY_SPEED_PX_PER_FRAME;

this.position.y -= this.FLY_SPEED_PX_PER_FRAME;

if (duck.position.y < duck.MIN_Y_PX) {

this.goingDown = true;

}

 

}

this.frame++;

}

}

};

View this version [SOURCE]. Diffing with the previous version [SOURCE] will highlight all changes.

 

Version 6 – replace global variable with dependency injection

In version 5, the context object (the reference to the area to render to) is shared. This is a bad assumption to make as it makes the code brittle – the objects simply draw to a global variable and global state is dangerous, particularly because it is difficult to unit test. In version 6, we pass in the context to the duck at initialisation time. We can also then use “extract method” and “move method” so the duck can render itself. In a larger system, you may want to have a DuckRenderer or similar, but here it’s small enough for the duck to do its own rendering.

init: function(targetContext) {

this.position = new Position(40, 40);

this.targetContext = targetContext;

},

With this change, the duck now renders to the context it has been given. This means that different objects could theoretically render to different locations, decoupling them from global scope.

View this version [SOURCE]. Diffing with the previous version [SOURCE] will highlight all changes.

 

Version 7 – Use Explanatory Variables

The duck’s render function has a lot of dense code talking to the canvas. Brevity in code is overrated – just because you can write some hideous compound statements doesn’t mean you should. As the C2 wiki famously states, “always code as if the person who ends up maintaining your code is a violent psychopath who knows where you live.”

In this version, we introduce some explanatory variables so we tell the reader exactly what we are doing.

Before:

render: function() {

this.targetContext.strokeStyle = "#00FF00";

this.targetContext.beginPath();

this.targetContext.moveTo(this.position.x - this.WING_WIDTH_PX, this.position.y + (this.WING_HEIGHT_PX * (this.frame % 2 === 0 ? -1 : 1)));

this.targetContext.lineTo(this.position.x, this.position.y);

this.targetContext.lineTo(this.position.x + this.WING_WIDTH_PX, this.position.y + (this.WING_HEIGHT_PX * (this.frame % 2 === 0 ? -1 : 1)));

this.targetContext.stroke();

}

After:

render: function() {

var isOddFrame = this.frame % 2 === 0;

var wingTipYOffset = this.WING_HEIGHT_PX * (isOddFrame ? -1 : 1);

var wingTipYPos = this.position.y + wingTipYOffset;

 

this.targetContext.beginPath();

this.targetContext.strokeStyle = "#00FF00";

this.targetContext.moveTo(this.position.x - this.WING_WIDTH_PX, wingTipYPos);

this.targetContext.lineTo(this.position.x, this.position.y);

this.targetContext.lineTo(this.position.x + this.WING_WIDTH_PX, wingTipYPos);

this.targetContext.stroke();

} 

 

Notice that the explanatory variables give the equation context at all times – we are referring to an abstraction rather than crunching numbers. Human brains like abstractions. If people don’t care how the variable is arrived at, give them the opportunity to ignore it.

View this version [SOURCE]. Diffing with the previous version [SOURCE] will highlight all changes.

Version 8 –Short, Specific Methods

“The first rule of functions is that they should be small. The second rule of functions is that they should be smaller than that.” - Bob Martin

A long method generally indicates sloppy code. It does too much in one place. A key rule in software is that everything should do only one thing, do it well, and have no side effects. A long method can be hard to understand or refactor as it is likely to contain too many local variables and it may have high cyclomatic complexity.

It is usually fairly straightforward to refactor a long method into sub actions using the extract method refactoring. The duck’s move function becomes a list of instructions:

move: function () {

if(this.hit) {

this.drop();

} else {

this.flap();

}

},

This is then followed by the drop() and flap() functions. They are all niladic (no-argument) functions. This is very similar to what we just did with explanatory variables – giving things context, and hiding detailed implementation until it is needed.

We try to keep related code close vertically, so note the sequence of functions:

1. move

2. drop

3. flap

The less scrolling around a user has to do, the better. These functions have what Bob Martin refers to as a “conceptual affinity” in his essential book, “Clean Code”:

“In general we want function call dependencies to point in the downward direction... As in newspaper articles, we expect the most important concepts to come first, and we expect them to be expressed with the least amount of polluting detail.”  - Bob Martin

This makes the code easy to read with less Q*bert style bouncing around in the IDE.

View this version [SOURCE]. Diffing with the previous version [SOURCE] will highlight all changes.

Version 9 – Replace Booleans with State

The duck has two Booleans – isGoingDown, and isHit. If we make a truth table, this has 4 possible permutations. However, really, the duck only has three states – flapping down, flapping up, or hit and plummeting to the ground.

Before:

isGoingDown: false,

isHit: false,

After:

action: null,

 

states: {

flappingDown: 0,

flappingUp: 1,

hit: 2

},

This is a much better representation of what the duck is actually doing. Having multiple booleans to indicates status gets messy quickly as the number of potential combinations of states is (number of Booleans)^2, so with just 3 states you have 9 possible combinations, some of which may be irrelevant.

In a more complex system, we would probably employ the State Pattern here.

View this version [SOURCE]. Diffing with the previous version [SOURCE] will highlight all changes.

Version 10 – Function Naming

The main render() function is poorly named – it also tells the duck to move. We rename it to gameLoop(), which represents what it is doing.

View this version [SOURCE]. Diffing with the previous version [SOURCE] will highlight all changes.

Version 11 – Extract (another) Class

We extract the sights into an object, just as we did with the duck.

View this version [SOURCE]. Diffing with the previous version [SOURCE] will highlight all changes.

Version 12 – Decompose Conditional and Extract Method

The logic to calculate a hit is a bit all over the shop in version 11, so in version 12 we put it in the sights class. We also make it explicit what we are testing for in the testForHits method using testForXAxisHit and testForYAxisHit sub methods. We’d make these methods private in a production system.

Before

function ev_mouseclick(ev) {

var clickX = ev.layerX;

var clickY = ev.layerY;

 

if (Math.abs(clickX - duck.position.x) < sights.SHOT_RADIUS_PX

    && Math.abs(clickY - duck.position.y) < sights.SHOT_RADIUS_PX) {

duck.state = duck.states.hit;

}

}

 

After

var sights = {

// ... code removed for brevity

testForHits: function(position) {

hitOnXAxis = this.testForXAxisHit(position.x);

hitOnYAxis = this.testForYAxisHit(position.y)

return hitOnXAxis && hitOnYAxis;

},

 

testForXAxisHit: function(x) {

return Math.abs(this.position.x - x) < this.SHOT_RADIUS_PX;

},

 

testForYAxisHit: function(y) {

return Math.abs(this.position.y - y) < this.SHOT_RADIUS_PX;

},

 

moveTo: function(x, y) {

this.position.x = x;

this.position.y = y;

}

};

// ... code removed for brevity

function ev_mouseclick(ev) {

sights.moveTo(ev.layerX, ev.layerY);

if(sights.testForHits(duck.position)) {

duck.hit();

}

}

View this version [SOURCE]. Diffing with the previous version [SOURCE] will highlight all changes.

Version 13 – extract package

We bundle the whole game up into a function and run it, so there is only one thing in the global namespace.

var Game = (function() {

// ... the game code

});

Game();

That’s probably enough for this example! The code has ended up in a consistent, easily maintainable state, ready to add graphics, behaviour, objects and game play.

View this version [SOURCE]. Diffing with the previous version [SOURCE] will highlight all changes.