Component that reacts on data changed from the store

Question

I have a React/Flux web app and want to follow the architecture best practices:

I will focus on one view for my question, the component used to insert/update an entity (a User in occurrence).

Everything is working with this code, but the core of my question is about the onUserDetailChanged function in my AdminUserDetail component, which I find too contrived and confusing.

Core of my question

I would like to refactor this code found in my component:

// ALL OF THIS IS UGLY
onUserDetailChanged: function() {
    var data = AdminUserDetailStore.getData();
    GuiHelper.blockGuiLoading(false);

    // Check first if there is a feedback message
    if (typeof data.FeedbackMessage !== 'undefined') {
        if (data.FeedbackMessage === ActionConstants.ERROR) {
            // Unexpected error
            GuiHelper.unexpectedError();
        }
        else if (data.FeedbackMessage === ActionConstants.SUCCESS) {
            // User saved successfully
            GuiHelper.infoDialog(BootstrapDialog.TYPE_SUCCESS, LanguageLoader.langDict.msgUserSaved);
        }
    }
    // If there is no feedback message, check for errors
    else {
        if (data.Errors !== null && data.Errors.length > 0) {
            // Errors
            GuiHelper.infoDialog(BootstrapDialog.TYPE_DANGER, GuiHelper.renderErrors(data.Errors));
        }
        else {
            // No errors, data changed (either from initial load or from user input)
            this.setState({ entity: data.Data });

            // Load the user permissions if required
            // (CAN I PLACE THIS CODE ELSEWHERE THAN IN THIS FUNCTION?)
            var userPermissions = AdminUserDetailPermissionsStore.getData().Data;
            if (this.state.entity.Id !== 0 && typeof userPermissions === 'undefined') {
                this.loadUserPermissions(data.Data.Username);
            }
        }
    }
},

What I don't like about this architecture is that all the api calls (getUserDetail(userId), getUserDetailPermissions(username), postUserDetail(requestData)) end up calling this function and I find it confusing how to notify the user about the result of the API call. Also, this function is even called on every data change caused by user input so I find things are all mixed up.

The possible outcomes all supported by this contrived code:

getUserDetail(userId) can end in SUCCESS with data or ERROR when an unexpected error arises (e.g. the api server is not responding)
getUserDetailPermissions(username) can end in SUCCESS with data or ERROR when an unexpected error arises (e.g. the api server is not responding)
postUserDetail(requestData) is a bit more complex since there are server validations on the data posted. It can end in SUCCESS or ERROR as the other calls, but it can also return a list of validation errors which must be displayed to the user.

Could I, for example, use different onChange functions to distinguish from each API call type and from a change in data caused by user input?

WebApiClient.js

import request from 'superagent';
var Promise = require('es6-promise').Promise;

module.exports = {
    get: function (url) {
        return new Promise(function (resolve, reject) {
            request
                .get(url)
                .end(function (err, res) {
                    if (err !== null) {
                        reject();
                    }
                    else {
                        if (res.status === 404) {
                            reject();
                        }
                        else {
                            resolve(JSON.parse(res.text));
                        }
                    }
                });
        });
    },

    post: function (url, data) {
        return new Promise(function (resolve, reject) {
            request
                .post(url)
                .send(data)
                .end(function (err, res) {
                    if (err !== null) {
                        reject();
                    }
                    else {
                        if (res.status === 404) {
                            reject();
                        }
                        else {
                            resolve(JSON.parse(res.text));
                        }
                    }
                });
        });
    }
}

ActionsCreator.js

var Dispatcher = require('./Dispatcher');
var ActionConstants = require('./ActionConstants');
var Promise = require('es6-promise').Promise;
var Api = require('../Util/WebApiClient');

var ActionCreator = {
    apiServer: 'http://localhost:55010/api',

    getUserDetail: function (userId) {
        Api.get(this.apiServer + '/v1/User/' + userId)
            .then(function (responseData) { Dispatcher.handleAction({ actionType: ActionConstants.GET_USER, data: responseData }); })
            .catch(function () { Dispatcher.handleAction({ actionType: ActionConstants.ERROR }); });
    },

    getUserDetailPermissions: function (username) {
        Api.get(this.apiServer + '/v1/Permission/ForUser?username=' + username)
            .then(function (responseData) { Dispatcher.handleAction({ actionType: ActionConstants.GET_USERPERMISSIONS, data: responseData }); })
            .catch(function () { Dispatcher.handleAction({ actionType: ActionConstants.ERROR }); });
    },

    postUserDetail: function (requestData) {
        Api.post(this.apiServer + '/v1/User/', requestData)
            .then(function (responseData) { Dispatcher.handleAction({ actionType: ActionConstants.POST_USER, data: responseData }); })
            .catch(function () { Dispatcher.handleAction({ actionType: ActionConstants.ERROR }); });
    }
};

module.exports = ActionCreator;

ActionConstants.js

module.exports = {
  GET_USER: 'GET_USER',
  GET_USERPERMISSIONS: 'GET_USERPERMISSIONS',
  POST_USER: 'POST_USER',

  ERROR: 'ERROR',
  SUCCESS: 'SUCCESS'
};

Dispatcher.js

var Dispatcher = require('flux').Dispatcher;

// Create dispatcher instance
var Dispatcher = new Dispatcher();

// Convenience method to handle dispatch requests
Dispatcher.handleAction = function(action) {
    this.dispatch({
        source: 'VIEW_ACTION',
        action: action
    });
}

module.exports = Dispatcher;

AdminUserDetailStore.js

var Dispatcher = require('../Core/Dispatcher');
var ActionConstants = require('../Core/ActionConstants');
var EventEmitter = require('events').EventEmitter;
var assign = require('object-assign');

var CHANGE_EVENT = 'change';

var AdminUserDetailStore = assign({}, EventEmitter.prototype, {
    data: {},

    // Called after the initial GET to load the initial User entity
    // or from user input
    setData: function(data, doEmitChange) {
        this.data = data;

        if (typeof doEmitChange !== 'undefined' && doEmitChange) {
            this.emitChange();
        }
    },

    // Called from a POST when trying to save the User entity
    // or when getting an unexpected error (e.g. api server unavailable)
    setFeedbackMessage: function(data, message) {
        if (typeof message !== 'undefined')
            this.data.FeedbackMessage = message;
        else {
            if (data.Errors.length > 0)
                this.data.Errors = data.Errors;
            else
                this.data.FeedbackMessage = ActionConstants.SUCCESS;
        }
    },

    emitChange: function () {
        this.emit(CHANGE_EVENT);
    },

    addChangeListener: function (callback) {
        this.on(CHANGE_EVENT, callback);
    },

    removeChangeListener: function (callback) {
        this.removeListener(CHANGE_EVENT, callback);
    },

    getData: function () {
        return this.data;
    }
});

Dispatcher.register(function(payload) {
    var action = payload.action;

    switch(action.actionType) {
        case ActionConstants.ERROR:
            // Force a feedback message
            AdminUserDetailStore.setFeedbackMessage(null, ActionConstants.ERROR);
            break;
        case ActionConstants.GET_USER:
            AdminUserDetailStore.setData(action.data);
            break;
        case ActionConstants.POST_USER:
            // Check for errors wthin the data object and set a SUCCESS feedback message if none are found
            AdminUserDetailStore.setFeedbackMessage(action.data);
            break;
        default:
            return true;
    }

    AdminUserDetailStore.emitChange();
    return true;
});

module.exports = AdminUserDetailStore;

AdminUserDetail.js

import React from 'react';
import ReactDOM from 'react-dom';
import ActionCreator from '../../Core/ActionCreator';
import ActionConstants from '../../Core/ActionConstants';

import AdminUserDetailStore  from '../../Stores/AdminUserDetailStore';
import AdminUserDetailPermissionsStore  from '../../Stores/AdminUserDetailPermissionsStore';

import LanguageLoader from '../../Util/LanguageLoader';
import GuiHelper from '../../Util/GuiHelper';
import BootstrapDialog from 'bootstrap-dialog';

import blockUI from '../../../Content/js/jquery.blockUI.js';

import {AgGridReact} from 'ag-grid-react';
import BoolCellEditor from '../AgGrid/BoolCellEditor';

$ = window.$ = window.jQuery = require('jquery');

var AdminUserDetail = React.createClass({
    getInitialState: function () {
        return {
            entity: {
                Id: 0,
                Username: '',
                Email: ''
            },

            userPermissions: null,
            userPermissionsColumns: [
                { headerName: LanguageLoader.langDict.lblRole, field: 'role' },
                { headerName: LanguageLoader.langDict.lblPermission, field: 'permission' },
                { headerName: LanguageLoader.langDict.lblDescription, field: 'description', width:600 }
            ],

            gridOptions: {
                context: this,
                singleClickEdit: true,
                enableColResize: true,
                onGridReady: function(event) {
                    //event.api.autoSizeColumns();
                },
            },
        };
    },

    getDefaultProps: function () {
        return { };
    },

    componentWillMount: function () {
        AdminUserDetailStore.addChangeListener(this.onUserDetailChanged);
        AdminUserDetailPermissionsStore.addChangeListener(this.onUserDetailPermissionsChanged);
    },

    componentWillUnmount: function () {
        AdminUserDetailStore.removeChangeListener(this.onUserDetailChanged);
        AdminUserDetailPermissionsStore.removeChangeListener(this.onUserDetailPermissionsChanged);
    },

    // ALL OF THIS IS UGLY
    onUserDetailChanged: function() {
        var data = AdminUserDetailStore.getData();
        GuiHelper.blockGuiLoading(false);

        // Check first if there is a feedback message
        if (typeof data.FeedbackMessage !== 'undefined') {
            if (data.FeedbackMessage === ActionConstants.ERROR) {
                // Unexpected error
                GuiHelper.unexpectedError();
            }
            else if (data.FeedbackMessage === ActionConstants.SUCCESS) {
                // User saved successfully
                GuiHelper.infoDialog(BootstrapDialog.TYPE_SUCCESS, LanguageLoader.langDict.msgUserSaved);
            }
        }
        // If there is no feedback message, check for errors
        else {
            if (data.Errors !== null && data.Errors.length > 0) {
                // Errors
                GuiHelper.infoDialog(BootstrapDialog.TYPE_DANGER, GuiHelper.renderErrors(data.Errors));
            }
            else {
                // No errors, data changed (either from initial load or from user input)
                this.setState({ entity: data.Data });

                // Load the user permissions if required
                // (CAN I PLACE THIS CODE ELSEWHERE THAN IN THIS FUNCTION?)
                var userPermissions = AdminUserDetailPermissionsStore.getData().Data;
                if (this.state.entity.Id !== 0 && typeof userPermissions === 'undefined') {
                    this.loadUserPermissions(data.Data.Username);
                }
            }
        }
    },

    onUserDetailPermissionsChanged: function() {
        var data = AdminUserDetailPermissionsStore.getData();
        GuiHelper.blockGuiLoading(false);

        if (typeof data.FeedbackMessage !== 'undefined' && data.FeedbackMessage === ActionConstants.ERROR) {
            GuiHelper.unexpectedError();
        }
        else {
            var userPermissions = data.Data.map(function(i, idx) {
                return { role: i.Key, permission: i.Value, description: LanguageLoader.langDict['P_' + i.Value] };
            });
            this.setState({ userPermissions: userPermissions });
        }
    },

    componentDidMount: function () {
        // Initialize/clear the stores
        AdminUserDetailStore.setData({});
        AdminUserDetailPermissionsStore.setData({});

        if (this.props.id !== null) {
            GuiHelper.blockGuiLoading(true);
            ActionCreator.getUserDetail(this.props.id);
        }
    },

    loadUserPermissions :function(username) {
        GuiHelper.blockGuiLoading(true);
        ActionCreator.getUserDetailPermissions(username);
    },

    saveUser: function () {
        GuiHelper.blockGuiLoading(true);
        ActionCreator.postUserDetail(this.state.entity);
    },

    onUsernameChanged: function(e) {
        var entity = this.state.entity;
        entity.Username = e.target.value;
        AdminUserDetailStore.setData({ Errors: null, Data: entity }, true);
    },

    onEmailChanged: function(e) {
        var entity = this.state.entity;
        entity.Email = e.target.value;
        AdminUserDetailStore.setData({ Errors: null, Data: entity }, true);
    },

    render: function () {
        return (
            <div>
                <h1>{LanguageLoader.langDict.lblUserDetail}</h1>

                <form>
                    <div className="row">
                        <div className="col-lg-3 col-md-4 col-sm-6 col-xs-9 col-centered">
                            <div className="form-group">
                                <label htmlFor="userUsername">{LanguageLoader.langDict.lblUsername}</label>
                                <input type="text" className="form-control" id="userUsername" value={this.state.entity.Username} placeholder={LanguageLoader.langDict.lblUsername} onChange={this.onUsernameChanged} />
                            </div>
                        </div>
                    </div>
                    <div className="row">
                        <div className="col-lg-3 col-md-4 col-sm-6 col-xs-9 col-centered">
                            <div className="form-group">
                                <label htmlFor="userEmail">{LanguageLoader.langDict.lblEmailAddress}</label>
                                <input type="text" className="form-control" id="userEmail" value={this.state.entity.Email} placeholder={LanguageLoader.langDict.lblEmail} onChange={this.onEmailChanged} />
                            </div>
                        </div>
                    </div>
                </form>

                <div className={this.state.userPermissions === null ? 'hidden' : ''}>
                    <div className="row">
                        <div className="col-lg-3 col-md-4 col-sm-6 col-xs-9 col-centered">
                            <div className="form-group">
                                <label>{LanguageLoader.langDict.lblPermissionsFromRole}</label>
                                <div style={{height: 400}} className="ag-fresh">
                                    <AgGridReact gridOptions={this.state.gridOptions} columnDefs={this.state.userPermissionsColumns} rowData={this.state.userPermissions} />
                                </div>
                            </div>
                        </div>
                    </div>
                </div>

            </div>
        );
    }
});

module.exports = AdminUserDetail;

Finally, from the server-side, an api call always return an object with 2 properties:

public dynamic Data { get; set; }
public ICollection<string> Errors { get; set; }

user34073user34073 · Accepted Answer · 2016-11-29 17:41:27Z

You can simplify your ifs:

.end(function (err, res) {
    if (err !== null) {
        reject();
    }
    else {
        if (res.status === 404) {
            reject();
        }
        else {
            resolve(JSON.parse(res.text));
        }
    }
});

Notice how you call reject()` for two paths. This can be simplified to:

.end(function (err, res) {
    if (err !== null || res.status === 404) {
        reject();
    }
    else {
        resolve(JSON.parse(res.text));
    }
});

This is a lot simpler to parse. Alternatively, if you still want to have the separate cases, I would use else if instead of nesting them:

.end(function (err, res) {
    if (err !== null) {
        reject();
    }
    else if (res.status === 404) {
        reject();
    }
    else {
        resolve(JSON.parse(res.text));
    }
});

Your bracing isn't consistent:

setFeedbackMessage: function(data, message) {
    if (typeof message !== 'undefined')
        this.data.FeedbackMessage = message;
    else {
        if (data.Errors.length > 0)
            this.data.Errors = data.Errors;
        else
            this.data.FeedbackMessage = ActionConstants.SUCCESS;
    }
}

You should always use braces, but even more important is always being consistent.

In this function, your return value is meaningless:

Dispatcher.register(function(payload) {
    var action = payload.action;

    switch(action.actionType) {
        case ActionConstants.ERROR:
            // Force a feedback message
            AdminUserDetailStore.setFeedbackMessage(null, ActionConstants.ERROR);
            break;
        case ActionConstants.GET_USER:
            AdminUserDetailStore.setData(action.data);
            break;
        case ActionConstants.POST_USER:
            // Check for errors wthin the data object and set a SUCCESS feedback message if none are found
            AdminUserDetailStore.setFeedbackMessage(action.data);
            break;
        default:
            return true;
    }

    AdminUserDetailStore.emitChange();
    return true;
});

You never return false, so returning true doesn't give you any meaningful information. I would make this a void method and just return.

Thanks for your input! I will make use of all these improvements, however they are not tackling the core of my question :) — Bruno, Commented Nov 29, 2016 at 17:53
Yep, I left that for other reviewers as I'm not a JS person. Also, the rules for the site are any and every point is up for discussion, even if some points are more equal than others. — user34073, Commented Nov 29, 2016 at 17:57
And your pertinent intervention is appreciated, thanks again! — Bruno, Commented Nov 29, 2016 at 18:00

Stack Exchange Network

Component that reacts on data changed from the store

Core of my question

1 Answer 1

Your Answer

Not the answer you're looking for? Browse other questions tagged
javascript
react.js
or ask your own question.

Hot Network Questions

Component that reacts on data changed from the store

Core of my question

1 Answer 1

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged javascriptreact.js or ask your own question.

Related

Hot Network Questions

Not the answer you're looking for? Browse other questions tagged
javascript
react.js
or ask your own question.