0

Data Structure coming back from the server

[
  {
    id: 1,
    type: "Pickup",
    items: [
      { 
        id: 1, 
        description: "Item 1"
      }
    ]
  },
  {
    id: 2,
    type: "Drop",
    items: [
      { 
        id: 0, 
        description: "Item 0"
      }
    ]
  },
  {
    id: 3,
    type: "Drop",
    items: [
      { 
        id: 1, 
        description: "Item 1"
      },
      { 
        id: 2, 
        description: "Item 2"
      }
    ]
  },
  {
    id: 0,
    type: "Pickup",
    items: [
      { 
        id: 0, 
        description: "Item 0"
      },
      { 
        id: 2, 
        description: "Item 2"
      }
    ]
  }
];
  • Each element represents an event.
  • Each event is only a pickup or drop.
  • Each event can have one or more items.

Initial State

On initial load, loop over the response coming from the server and add an extra property called isSelected to each event, each item, and set it as false as default. -- Done.

This isSelected property is for UI purpose only and tells user(s) which event(s) and/or item(s) has/have been selected.

// shove the response coming from the server here and add extra property called isSelected and set it to default value (false)
const initialState = {
  events: []
}

moveEvent method:

const moveEvent = ({ events }, selectedEventId) => {
  // de-dupe selected items 
  const selectedItemIds = {};
  
  // grab and find the selected event by id
  let foundSelectedEvent = events.find(event => event.id === selectedEventId);
  
  // update the found event and all its items' isSelected property to true
  foundSelectedEvent = {
    ...foundSelectedEvent, 
    isSelected: true,
    items: foundSelectedEvent.items.map(item => {
      item = { ...item, isSelected: true };
      // Keep track of the selected items to update the other events.
      selectedItemIds[item.id] = item.id;
      return item;
    }) 
  };

  events = events.map(event => {
    // update events array to have the found selected event
    if(event.id === foundSelectedEvent.id) {
      return foundSelectedEvent;
    }
    
    // Loop over the rest of the non selected events
    event.items = event.items.map(item => {
      // if the same item exists in the selected event's items, then set item's isSelected to true.
      const foundItem = selectedItemIds[item.id];
      // foundItem is the id of an item, so 0 is valid
      if(foundItem >= 0) {
        return { ...item, isSelected: true };
      }
      return item;  
    });
    
    const itemCount = event.items.length;
    const selectedItemCount = event.items.filter(item => item.isSelected).length;
    
    // If all items in the event are set to isSelected true, then mark the event to isSelected true as well.
    if(itemCount === selectedItemCount) {
      event = { ...event, isSelected: true };
    }

    return event;
  });

  return { events }
}

Personally, I don't like the way I've implemented the moveEvent method, and it seems like an imperative approach even though I'm using find, filter, and map. All this moveEvent method is doing is flipping the isSelected flag.

  1. Is there a better solution?
  2. Is there a way to reduce the amount of looping? Maybe events should be an object and even its items. At least, the lookup would be fast for finding an event, and I don't have to use Array.find initially. However, I still have to either loop over each other non selected events' properties or convert them back and forth using Object.entries and/or Object.values.
  3. Is there more a functional approach? Can recursion resolve this?

Usage and Result

// found the event with id 0
const newState = moveEvent(initialState, 0);

// Expected results
[
  {
    id: 1,
    type: 'Pickup',
    isSelected: false,
    items: [ { id: 1, isSelected: false, description: 'Item 1' } ]
  }
  {
    id: 2,
    type: 'Drop',
    // becasue all items' isSelected properties are set to true (even though it is just one), then set this event's isSelected to true
    isSelected: true,
    // set this to true because event id 0 has the same item (id 1)
    items: [ { id: 0, isSelected: true, description: 'Item 0' } ]
  }
  {
    id: 3,
    type: 'Drop',
    // since all items' isSelected properties are not set to true, then this should remain false.
    isSelected: false,
    items: [
      { id: 1, isSelected: false, description: 'Item 1' },
      // set this to true because event id 0 has the same item (id 2)
      { id: 2, isSelected: true, description: 'Item 2' }
   ]
  }
  {
    id: 0,
    type: 'Pickup',
    // set isSelected to true because the selected event id is 0
    isSelected: true,
    items: [
      // since this belongs to the selected event id of 0, then set all items' isSelected to true
      { id: 0, isSelected: true, description: 'Item 0' },
      { id: 2, isSelected: true, description: 'Item 2' }
    ]
  }
]
5
  • This is probably better suited to codereview.stackexchange.com Commented Nov 18, 2020 at 0:48
  • Thank you. I have posted on the other site. Commented Nov 18, 2020 at 0:58
  • I think the over-complication of your code is because you're storing selectedEventId and isSelected inside the event object. It's difficult for us to recommend a solution without seeing your UI code too, but I suggest moving selectedEventId to the root of the state (on the same level as events: []). This will drastically reduce the amount of state permutations, therefore less chance of bugs. The components can then be enhanced to display the correct UI by looking up selectedEventId. Always try to make state as simple as possible :) Commented Nov 18, 2020 at 1:11
  • Yeah, the UI is way too complicated to explain it, and I have over-simplified it here. I don't understand why I need to add selectedEventId in state. moveEvent is just a function that receives the state and selected event id and return a new piece of state. Commented Nov 18, 2020 at 2:00
  • It's not just one event that a user can select. So, the moveEvent method will be call each time a user selects a specific event and updating the state. Commented Nov 18, 2020 at 2:09

1 Answer 1

1

One of the problems with the current solution is data duplication. You are basically trying to keep the data between the different items in sync. Instead of changing all items with the same id, make sure there are no duplicate items by using an approach closer to what you would find in a rational database.

Let's first normalize the data:

const response = [...]; // data returned by the server

let data = { eventIds: [], events: {}, items: {} };
for (const {id, items, ...event} of response) {
  data.eventIds.push(id);
  data.events[id] = event;
  event.items = [];
  for (const {id, ...item} of items) {
    event.items.push(id);
    data.items[id] = item;
  }
}

This should result in:

const data {
  eventIds: [1, 2, 3, 0], // original order
  events: {
    0: { type: "Pickup", items: [0, 2] },
    1: { type: "Pickup", items: [1]    },
    2: { type: "Drop",   items: [0]    },
    3: { type: "Drop",   items: [1, 2] },
  },
  items: {
    0: { description: "Item 0" },
    1: { description: "Item 1" },
    2: { description: "Item 2" },
  },
};

The next thing to realize is that the isSelected property of an event is computed based on the isSelected property of its items. Storing this would mean more data duplication. Instead calculate it though a function.

const response = [{id:1,type:"Pickup",items:[{id:1,description:"Item 1"}]},{id:2,type:"Drop",items:[{id:0,description:"Item 0"}]},{id:3,type:"Drop",items:[{id:1,description:"Item 1"},{id:2,description:"Item 2"}]},{id:0,type:"Pickup",items:[{id:0,description:"Item 0"},{id:2,description:"Item 2"}]}];

// normalize incoming data
let data = { eventIds: [], events: {}, items: {} };
for (const {id, items, ...event} of response) {
  data.eventIds.push(id);
  data.events[id] = event;
  event.items = [];
  for (const {id, ...item} of items) {
    event.items.push(id);
    data.items[id] = item;
    item.isSelected = false;
  }
}

// don't copy isSelected into the event, calculate it with a function 
const isEventSelected = ({events, items}, eventId) => {
  return events[eventId].items.every(id => items[id].isSelected);
};

// log initial data
console.log(data);
for (const id of data.eventIds) {
  console.log(`event ${id} selected?`, isEventSelected(data, id));
}

// moveEvent implementation with the normalized structure
const moveEvent = (data, eventId) => {
  let { events, items } = data;
  for (const id of events[eventId].items) {
    items = {...items, [id]: {...items[id], isSelected: true}};
  }
  return { ...data, items };
};

data = moveEvent(data, 0);

// log after data applying `moveEvent(data, 0)`
console.log(data);
for (const id of data.eventIds) {
  console.log(`event ${id} selected? `, isEventSelected(data, id));
}

// optional: convert structure back (if you still need it)
const convert = (data) => {
  const { eventIds, events, items } = data;
  return eventIds.map(id => ({
    id,
    ...events[id],
    isSelected: isEventSelected(data, id),
    items: events[id].items.map(id => ({id, ...items[id]}))
  }));
};

console.log(convert(data));
Check browser console, for better ouput readability.

I'm not sure if this answers solves your entire problem, but I hope you got something useful info out of it.

Sign up to request clarification or add additional context in comments.

1 Comment

I appreciate the effort, and it gave me some ideas that I can build on. However, I liked the other answer (on codereview.stackexchange) better. It seems simpler, and it doesn't require to keep track of the lookups. By the way, you have an error in the moveEvent method. You're using const itemsLookup and then reassigning it later, which throws an error.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.