Skip to content

Instantly share code, notes, and snippets.

@imjul1an
Created February 13, 2016 21:56
Show Gist options
  • Save imjul1an/8402475d1071704dde1b to your computer and use it in GitHub Desktop.
Save imjul1an/8402475d1071704dde1b to your computer and use it in GitHub Desktop.
import * as types from '../constants';
import combineTypes from '../utils';
const item = (state, action) => {
const actions = {
[types.UPDATE_ITEM_SIZE_REQUEST]: () => ({
...state,
isUpdatingSize: true
}),
[types.UPDATE_ITEM_SIZE_SUCCESS]: () => action.payload,
[types.UPDATE_ITEM_SIZE_FAILURE]: () => ({
...state,
isUpdatingSize: false,
error: action.payload.error
}),
[types.DELETE_ITEM_REQUEST]: () => ({
...state,
isDeleting: true,
isDeletingTimerOn: true
}),
[types.DELETE_ITEM_SUCCESS]: () => ({
...state,
isDeleting: false,
isDeletingTimerOn: false,
isDeleted: true
}),
[types.DELETE_ITEM_FAILURE]: () => ({
...state,
isDeleting: true,
isDeletingTimerOn: true,
error: action.payload.error
}),
[types.ADD_ITEM_TO_CART_REQUEST]: () => ({
...state,
isUpdatingCart: true
}),
[types.ADD_ITEM_TO_CART_SUCCESS]: () => ({
...state,
isAddingToCart: false,
article: {
isOnCart: true
}
}),
[types.ADD_ITEM_TO_CART_FAILURE]: () => ({
...state,
isAddingToCart: false,
error: action.payload.error
}),
default: () => state
};
return (actions[action.type] || actions.default)();
};
const items = (state, action) => {
const itemId = action.payload.itemId;
const actions = {
[combineTypes({
by: action.type,
with: [
types.UPDATE_ITEM_SIZE_REQUEST,
types.UPDATE_ITEM_SIZE_SUCCESS,
types.UPDATE_ITEM_SIZE_FAILURE,
types.DELETE_ITEM_REQUEST,
types.DELETE_ITEM_SUCCESS,
types.DELETE_ITEM_FAILURE,
types.ADD_ITEM_TO_CART_REQUEST,
types.ADD_ITEM_TO_CART_SUCCESS,
types.ADD_ITEM_TO_CART_FAILURE
]
})]: () => ({
...state,
[itemId]: item(state[itemId], action)
}),
default: () => state
};
return (actions[action.type] || actions.default)();
};
const collectionData = (state, action) => {
const { collectionId, itemCount } = action.payload;
const actions = {
[combineTypes({
by: action.type,
with: [
types.UPDATE_ITEM_SIZE_REQUEST,
types.UPDATE_ITEM_SIZE_SUCCESS,
types.UPDATE_ITEM_SIZE_FAILURE,
types.DELETE_ITEM_REQUEST,
types.DELETE_ITEM_SUCCESS,
types.DELETE_ITEM_FAILURE,
types.ADD_ITEM_TO_CART_REQUEST,
types.ADD_ITEM_TO_CART_SUCCESS,
types.ADD_ITEM_TO_CART_FAILURE
]
})]: () => ({
...state,
items: items(state[collectionId], action)
}),
[types.SHOW_MORE_ITEMS]: () => ({
...state,
visibleItems: itemCount
}),
default: () => state
};
return (actions[action.type] || actions.default)();
};
const itemsByCollection = (state, action) => {
const { collectionId } = action.payload;
const actions = {
[combineTypes({
by: action.type,
with: [
types.SHOW_MORE_ITEMS,
types.UPDATE_ITEM_SIZE_REQUEST,
types.UPDATE_ITEM_SIZE_SUCCESS,
types.UPDATE_ITEM_SIZE_FAILURE,
types.DELETE_ITEM_REQUEST,
types.DELETE_ITEM_SUCCESS,
types.DELETE_ITEM_FAILURE,
types.ADD_ITEM_TO_CART_REQUEST,
types.ADD_ITEM_TO_CART_SUCCESS,
types.ADD_ITEM_TO_CART_FAILURE
]
})]: () => ({
...state,
[collectionId]: collectionData(state[collectionId], action)
}),
default: () => state
};
return (actions[action.type] || actions.default)();
};
export default itemsByCollection;
@ahmed1490
Copy link

Nice playing with es6 stuffs :D 👍
Just the name combineTypes is unclear(matchInArray/filterArray.. better name?).. As i understand that function is dynamically given a key for the action Object, based on the reducer action params. Nice but feels tricky.

Apart, About the code being in our app..
I feel the program has become.. larger and also the simplicity has reduced.
Reading through

switch (action.type) {
        case types.SHOW_MORE_ITEMS:
        case types.UPDATE_ITEM_SIZE_REQUEST:
        case types.UPDATE_ITEM_SIZE_SUCCESS:
        case types.UPDATE_ITEM_SIZE_FAILURE:
        case types.REMOVE_ITEM_REQUEST:
        case types.REMOVE_ITEM_SUCCESS:
        case types.REMOVE_ITEM_FAILURE:
        case types.ADD_ITEM_TO_CART_REQUEST:
        case types.ADD_ITEM_TO_CART_SUCCESS:
        case types.ADD_ITEM_TO_CART_FAILURE:
            return {
                ...state,
                [collectionId]: collectionData(state[collectionId], action)
            };

or a regex match in the case:
VS

const actions = {
 [combineTypes({
            by: action.type,
            with: [
                types.SHOW_MORE_ITEMS,
                types.UPDATE_ITEM_SIZE_REQUEST,
                types.UPDATE_ITEM_SIZE_SUCCESS,
                types.UPDATE_ITEM_SIZE_FAILURE,
                types.DELETE_ITEM_REQUEST,
                types.DELETE_ITEM_SUCCESS,
                types.DELETE_ITEM_FAILURE,
                types.ADD_ITEM_TO_CART_REQUEST,
                types.ADD_ITEM_TO_CART_SUCCESS,
                types.ADD_ITEM_TO_CART_FAILURE
            ]
        })]: () => ({
            ...state,
            [collectionId]: collectionData(state[collectionId], action)
        }),

Comparatively the simple switch case even with repeated logic stands clear to trace..

Another point that makes me uneasy is redefinition of the object everytime. It makes it slower (https://jsperf.com/switch-vs-object-literal-vs-module/14) and even slower with variable as keys (https://babeljs.io/repl/#?experimental=false&evaluate=true&loose=false&spec=false&code=const%20ABC%20%3D%20'HEllo'%0Aconst%20ABC1%20%3D%20'HEllo'%0A%0Aconst%20a%20%3D%20%7B%0A%20%20%5BABC%5D%20%3A%203%2C%0A%20%20%5BABC1%5D%3A%203%0A%7D)

In terms of doing it the object literal way, It is better to repeat keys mapped to same anonymous function than dynamic regeneration of object literal.

@ahmed1490
Copy link

.. continuing.. better to repeat keys.. eg below..

const stateForCollectionReducer = (state, action) => ({
        ...state,
        [action.collectionId]: collectionData(state[collectionId], action)
})
const itemsByCollectionReduction = {
        [types.SHOW_MORE_ITEMS]: stateForCollectionReducer,
        [types.UPDATE_ITEM_SIZE_REQUEST]: stateForCollectionReducer,
        [types.UPDATE_ITEM_SIZE_SUCCESS]: stateForCollectionReducer,
        [types.UPDATE_ITEM_SIZE_FAILURE]: stateForCollectionReducer,
        [types.DELETE_ITEM_REQUEST]: stateForCollectionReducer,
        [types.DELETE_ITEM_SUCCESS]: stateForCollectionReducer,
        [types.DELETE_ITEM_FAILURE]: stateForCollectionReducer,
        [types.ADD_ITEM_TO_CART_REQUEST]: stateForCollectionReducer,
        [types.ADD_ITEM_TO_CART_SUCCESS]: stateForCollectionReducer,
        [types.ADD_ITEM_TO_CART_FAILURE]: stateForCollectionReducer,
        [types.SHOW_MORE_ITEMS]: stateForCollectionReducer
};
const defaultReduction = (state) => state;

const itemsByCollection = (state, action) => (itemsByCollectionReduction[action.type] || defaultReduction)(state, action);

I will argue this is better than redefinition of object literal dynamically. This code is easy for anyone to understand and reason. (and switch statement even more reasonable in my opinion.. )..

itemsByCollection is a top level key reducer which will be called for every action which is dispatched. And we cannot afford to redefine an object literal every action dispatch. Add to it.. In the process of redefining the object literal, a function call is done (combineTypes) and add to it ..the dynamic object keys.. its adds at least two more function call for the dynamic object keys as well.

So if you want to go with object literal way.. Probably find a way to cache a static/constant object. One way is the above way. But again.. It seems like the code is going through unnecessary splitting to keep the simplicity. Or maybe its not unnecessary. :) Lets talk on these.

Lastly I still feel swtich or simple if else is the best :)

@imjul1an
Copy link
Author

I have to admit that it was a reasonable argues, however, I think this is not a case where we have to really pay attention to performance issues.

I do agree that a switch statement is more readable, but it doesn't mean that it is the best way of matching objects in JavaScript.

  • I'll say switch statement is a habit that developers inherited from OOP languages, like C++, C#, Java etc.
  • IMHO I believe object-literals is the natural way of JavaScript to operate on object matching operations.

There is a very good article on the subject here

@ahmed1490
Copy link

Totally agree with this :).
I liked object literals and I have always used it against switch wherever possible coz its less verbose and more readable.
But unexpectedly switch brings more readability in the above code. Also for redux, I personally prefer switch more, due to redux docs.. coz in my experience "conventions" in code always added to developer friendlyness.

But object literal is fine as well. In that case, the object literal just have to be made more simple to follow through. The combineType still remains tricky. What do you think about the second comment where the code snippet doesnt have the dynamic object generation?

@imjul1an
Copy link
Author

I believe it was included to JavaScript for the same reason as Brendon Eich did it with the initial name of JavaScript which was Mocha then it was LiveScript and finally became JavaScript. It was a marketing strategy - to attract Java developers to switch to JavaScript. The same story with a new operator, var julian = new Person() - it's just about business and nothing about real philosophy of JavaScript :)

Let's be more Object'ish :)

@imjul1an
Copy link
Author

I'm not really trying to push with object-literals I'm just really upset because Flux started using it and Redux just follow them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment