A side by side comparison of a component before/after refactor

README.md

ResourceHistoryMaster.jsx

I was working on a refactor, I thought I would create this gist as a way to show some patterns that I like.

The rewritten file is ResourceHistoryMaster-AFTER.jsx. I think it's worth looking over the original file, before looking at the new one.

The rewritten file is heavily commented.

ResourceHistoryMaster-AFTER.jsx

import React from "react";

import ResourceActions from "actions/ResourceActions";
import subscribe from "utilities/subscribe";
import RaisedButton from "material-ui/RaisedButton";

export default subscribe(React.createClass({
    displayName: "MyResourceRequestsPage",

    // This handles all the resource logic
    fetch() {
        // By using props.subscriptions we're guaranteed that our app will
        // rerender when these stores are populated.
        let { 
            IdentityStore, ProfileStore, StatusStore, ResourceRequestStore 
        } = this.props.subscriptions;

        let profile = ProfileStore.get();
        let statuses = StatusStore.getAll();
        
        // I like to declare the variable above the statement where it is
        // assigned a value. Very quickly I can read this and know, that I'm
        // assigning requests, and request dependes on profile
        let requests;
        if (profile) {
            requests = ResourceRequestStore.findWhere({
                "created_by.username": profile.get('username')
            });
        }

        // Return an object with all resources, even if some are not resolved
        // yet, because:
        // 1. This makes the structure obvious
        // 2. This allows for destructuring that won't throw errors on missing
        //    props in render() below
        return {
            statuses,
            requests, 
            profile 
        }
    },

    // Although this function does end up with calling a closeAction, its
    // completely parametrized. Said differently, it's a function of its
    // inputs. This is much easier to test than if statuses, was moved inside
    // the function as a store.getAll call.
    onClose(request, statuses) {
        let closedStatus = statuses.findWhere({ "name": "closed" });
        ResourceActions.close({
            request: request,
            status: closedStatus
        });
    },

    render() {
        let { statuses, requests, profile } = this.fetch();

        // Render here is completely unconcerned with the logic of fetching
        // resources, it just knows that if they are falsy, it should "load"
        if (!(requests && statuses && profile)) {
            return <div className="loading" />
        }

        let viewProps = {
            statuses, 
            requests,
            onClose: this.onClose
        };

        // Here we call a separate component to do the dumb view stuff. It's
        // the easiest component to test, its just a function of its props.
        // Because this parent component did all the loading, the view doesn't
        // have to worry if its resources are present, which is really nice,
        // no wrapping if (resource) concerns.
        return <MyResourceRequestsPageView {...viewProps} />
    }

}), ["ProfileStore", "StatusStore", "ResourceRequestStore"]);

let MyResourceRequestsPageView = React.createClass({
    displayName: "MyResourceRequestsPageView",

    propTypes: {
        statuses: React.PropTypes.object.isRequired,
        requests: React.PropTypes.object.isRequired,
        onClose: React.PropTypes.func.isRequired
    },

    renderRequestRow(request) {
        let statuses = this.props.statuses;
        let statusName = request.get("status").name;

        let className = "active"
        if (statusName == "approved") {
            className = "success";
        } else if (statusName == "denied") {
            className = "denied";
        }

        let closeButton = (
        <RaisedButton primary 
            className="pull-right" 
            onTouchTap={() => this.props.onClose(request, statuses)} 
            label="Close" />
        );

        // I want to draw attention to the line below with the ternary. There
        // are other ways to do this. I could have give the closeButton a
        // default of null above, and gotten rid of the ternary below. It's
        // really easy to make jsx /much/ less readable as you start throwing
        // expressions into it. 
        // Compare:
        //    { closeButton }
        //    { statusName == "pending" ? closeButton : null}
        // 
        // So I normally advocate for moving complex expressions out of jsx. A
        // template language's greatest offering is its clarity (think about
        // writing html in js). However I prefer the ternary because it's not
        // deceiving. When you read { closeButton } you would likely assume
        // that each table row has a button.
        return (
        <tr key={request.id} className={className}>
            <td className="col-md-5">
                {request.get("request")}
            </td>
            <td className="col-md-5">
                {request.get("description")}
            </td>
            <td className="col-md-2">
                {statusName}
                {statusName == "pending" ? closeButton : null}
            </td>
        </tr>
        );
    },

    renderBody() {
        let requests = this.props.requests;

        // This is just a short example of writing code to minimize
        // indentation. Since I'm returning here, I can use the space below
        // the if as an implicit else. I always move the simple returns to the
        // top, and leave the complex highly indented bit below.
        if (!requests.length) {
            return (
            <p>You have not made any resource requests.</p>
            );
        }

        return (
        <table className="table table-condensed image-requests">
            <tbody>
                <tr>
                    <th>
                        <h3 className="t-title">Request</h3>
                    </th>
                    <th>
                        <h3 className="t-title">Reason</h3>
                    </th>
                    <th>
                        <h3 className="t-title">Status</h3>
                    </th>
                </tr>
                { requests.map(this.renderRequestRow) }
            </tbody>
        </table>
        );
    },

    render() {
        return (
        <div className="container">
            { this.renderBody() }
        </div>
        );
    }
});

ResourceHistoryMaster.jsx

import React from "react";

import ResourceActions from "actions/ResourceActions";
import subscribe from "utilities/subscribe";
import RaisedButton from "material-ui/RaisedButton";

const MyResourceRequestsPage = React.createClass({
    displayName: "MyResourceRequestsPage",

    closeRequest: function(request) {
        let { StatusStore } = this.props.subscriptions;
        var closedStatus = StatusStore.findWhere({
            "name": "closed"
        }).models[0].id;
        ResourceActions.close({
            request: request,
            status: closedStatus
        });
    },

    render: function() {
        let { IdentityStore, ProfileStore, StatusStore, ResourceRequestStore } = this.props.subscriptions;
        var username = ProfileStore.get().id,
            statusTypes = StatusStore.getAll();

        if (username == null || !statusTypes) {
            return <div className="loading"></div>
        }

        var requests = ResourceRequestStore.findWhere({
            "created_by.username": username
        });


        if (requests == null) {
            return <div className="loading"></div>;
        }

        if (!requests.models[0]) {
            return (
            <div className="container">
                <p>
                    You have not made any resource requests.
                </p>
            </div>
            );
        }

        var displayRequests = requests.map(function(request) {

            // Handler to allow close buton to call React class closeRequest with the proper argument
            var close = function() {
                this.closeRequest(request)
            }.bind(this);

            var closeButton;
            var text = request.get("status").name;

            // set the color of the row based on the status of the request
            var trClass = "active";

            if (text === "approved") {
                trClass = "success";
            } else if (text === "denied") {
                trClass = "denied";
            } else if (text === "pending") {
                closeButton = (
                    <RaisedButton
                        primary
                        className="pull-right"
                        onTouchTap={close}
                        label="Close"
                    />
                );
            }

            return (
            <tr key={request.id} className={trClass}>
                <td className="col-md-5">
                    {request.get("request")}
                </td>
                <td className="col-md-5">
                    {request.get("description")}
                </td>
                <td className="col-md-2">
                    {request.get("status").name}
                    {closeButton}
                </td>
            </tr>
            );

        }.bind(this));

        return (
        <div className="container">
            <table className="table table-condensed image-requests">
                <tbody>
                    <tr>
                        <th>
                            <h3 className="t-title">Request</h3>
                        </th>
                        <th>
                            <h3 className="t-title">Reason</h3>
                        </th>
                        <th>
                            <h3 className="t-title">Status</h3>
                        </th>
                    </tr>
                    {displayRequests}
                </tbody>
            </table>
        </div>
        );
    }
});

export default subscribe(MyResourceRequestsPage, ["ProfileStore", "StatusStore", "ResourceRequestStore"]);

ResourceHistoryMaster.png

I really like the fetch() method here, encapsulating all of your network logic in one place also makes it easy to test different results being sent to your components.

The example above is rather simple in that you are always going to getAll(), so I'm naturally curious what this starts to look like as the page starts to grow... (Thinking of "monster 'Networked Resource Components'" like InstanceLaunchWizardModal)

As an example this page may some day have:

• Searchable requests
• Filter down requests based on "type", on "user", etc.
• Multi-level dependency (ResourceRequestStore has a different fetchWhere based on the results of IdentityStore, StatusStore, ...)

Stay tuned! :)