Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test for subnetwork reducers #42

Open
1 of 2 tasks
nolleto opened this issue Oct 17, 2019 · 3 comments
Open
1 of 2 tasks

Add test for subnetwork reducers #42

nolleto opened this issue Oct 17, 2019 · 3 comments

Comments

@nolleto
Copy link
Collaborator

nolleto commented Oct 17, 2019

This issue can ben two PRs but, do the tests first to make sure nothing breaks.
Also, you can use ramda to refactor:

// current
case LOAD_NETWORK: {
  const { subnetworks } = action.payload.state.network;
  return subnetworks || [];
}
// with Ramda
import { path, defaultTo } from 'ramda';

const pathPayloadStateNetwork = path(['payload', 'state', 'network'])
const defaultToEmptyArray = defaultTo([]);

case LOAD_NETWORK: {
  return defaultToEmptyArray(pathPayloadStateNetwork(action))
}
// or
const onLoadNetwork = pipe(pathPayloadStateNetwork, defaultToEmptyArray);
case LOAD_NETWORK: {
  return onLoadNetwork(action)
}

Other details about this reducer:

  • There is a method called colorsNol that should be something like getRandomColor. I believe you can extract this to a new utils like /src/utils/colors or use a package for it. Just to be clear: a super-node is a sub-network that will receive a random color. Yes, I'm awere that super-node is a really bad name but I will change it in another PR.
  • There is a condition that I don't know if is valid: if (nodes && positions) {. This was made because we can receive two different networks: the current format and the old.

networks.zip

In this .zip you can find the two networks. You can move theses networks to a __fixtures__ folder and then use it in your tests.

  • first-version-network.json - the first/legacy version
  • simple-network.json - the current version

Well, thats it 😄

@ufocoder
Copy link
Contributor

Take this issue

@ufocoder
Copy link
Contributor

@nolleto I think that is no need to refactor with ramda because it will be verbose and it there will be refactoring for refactoring only. For example defaultToEmptyArray, onRemoveSuperNode and onLoadNetwork functions not improve code readability for developer:

const pathSubnetwork = path(['payload', 'state', 'network', 'subnetworks']);
const defaultToEmptyArray = defaultTo([]);
const onLoadNetwork = pipe(pathSubnetwork, defaultToEmptyArray);
const onRemoveSuperNode = (action) => filter(propEq('id', action.payload.id))

export default (state = [], action) => {
  switch (action.type) {
    case NEW_NETWORK:
      return defaultToEmptyArray();
    case ADD_SUPER_NODE:
      return [
        ...state,
        formatNetwork(action.payload.state),
      ];
    case REMOVE_SUPER_NODE:
      return onRemoveSuperNode(action)(state)

    case LOAD_NETWORK: {
      return onLoadNetwork(action)
    }
    default:
      return state;
  }
};

@nolleto
Copy link
Collaborator Author

nolleto commented Oct 29, 2019

Hello @ufocoder

Yeah, the refactor of this small reducer has not a great impact on readability, for example. This is just a refactor to keep it more functional and I believe just REMOVE_SUPER_NODE will take more advantage of it.
A good thing about using ramda here is that it never will throw an error or change the object.

const removeSuperNode = (state, action) => 
  state.filter(n => n.id !== action.payload.id);

const getPayloadId = path(['payload', 'id']);
const removeSuperNodeWithRamda = (state, action) => 
  reject(propEq('id', getPayloadId(action)), state);

removeSuperNode([], {}) // throws an error
removeSuperNodeWithRamda([], {}) // returns a new state: []

Another thing, I believe the refactor will be something like:

const addNetworkProps = ({ network, nodes, positions }) => ({
  ...network,
  nodes,
  positions,
  color: getRandomColor(),
})

const formatNetwork = ifElse(hasNodesAndPositions, addNetworkProps, prop('network'))
const getPayloadState = path(['payload', 'state'])
const getPayloadSubnetworks = path(['payload', 'state', 'network', 'subnetworks'])
const getPayloadId = path(['payload', 'id'])

const getNetworkFromPayloadState = pipe(getPayloadState, formatNetwork)
const getPayloadSubnetworksOrDefault = pipe(getPayloadSubnetworks, defaultTo([]))

export default (state = [], action = {}) => {
  switch (action.type) {
    case NEW_NETWORK:
      return [];
    case ADD_SUPER_NODE:
      return append(getNetworkFromPayloadState(action), state);
    case REMOVE_SUPER_NODE:
      return reject(propEq('id', getPayloadId(action)), state);
    case LOAD_NETWORK: 
      return getPayloadSubnetworksOrDefault(action);
    default:
      return state;
  }
};

😄

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

No branches or pull requests

2 participants