0
\$\begingroup\$

I have this code that controls the user permissions provided to a user. In my case, I constructed a simple example of CRUD. When the admin checks the boxes, it will grant the user access to perform that operation. For example, if I provide the user the ability to create, they can do so.

The code works but I believe there is a better approach to make a simpler or more short and concise code using what I have mentioned or made in my code below.

Is there any way to shorten and make my code more efficient, and can you provide me any ideas, advice or tips on how to create better and more efficient code?

Code:


 const [permission, setPermission] = useState({
    create: true,
    view: true,
    edit: true,
    delete: true,
  });

  const createPerms = () => {
    setPermission({ ...permission, create: !permission.create });
  };

  const viewPerms = () => {
    setPermission({ ...permission, view: !permission.view });
  };

  const editPerms = () => {
    setPermission({ ...permission, edit: !permission.edit });
  };

  return (
    <>
      <h1>
        <center> User Permission </center>
      </h1>

      <table className="ui celled table">
        <thead>
          <tr>
            {Object.keys(permission).map((key) => (
              <th key={key}>{key}</th>
            ))}
          </tr>
        </thead>
        <tbody>
          <tr>
            {Object.keys(permission).map((key) => (
              <td key={key}>
                <input type="checkbox" onChange={eval(key + "Perms")} />
              </td>
            ))}
          </tr>
        </tbody>
      </table>

      {permission.create && <h3>Create</h3>}

      {permission.view && <h3>View</h3>}
    </>
  );
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Welcome to CR! Please choose a title that describes the application's purpose, rather than the goal for the review. Thanks. \$\endgroup\$
    – ggorlen
    Nov 6 at 11:18
2
\$\begingroup\$

The main points on your code that can be improved are described in the following sections. I can't really find anything that can improve the performance of your code, so i will focus more on how you can shorten your code and make it more readable.

Checkboxes state

You chose to store all 4 permissions in an object which is just fine. The key of the object is the type of the permission and the value is the corresponding flag indicating if the permission is on/off (enabled). You haven't used the enabled part in your checkboxes in order to reflect the current state. That's why although you have set all permissions initially to true, the checkboxes are not checked.

You can use Object.entries(permission).map(([key, enabled]) instead and provide the enabled as the checked attribute of the checkbox element.

Use of eval

In general you should avoid using eval. You can see a pattern in your createPerms, editPerms etc methods. They all have the same syntax and the only thing changing is the type of the permission. That makes me think that you can create one method (instead of 4) and pass the type of the permission as a parameter. Your method could look like this:

  const oneToRuleThemAll = (type) => {
    setPermission({ ...permission, [type]: !permission[type] });
  };

Naming

Naming is really important and helps other people reading your code (or even your future self) to quickly understand what your code and variables are intended to do. Some key points that can improve readability are:

  • rename permission state to permissions since it keeps all of them
  • when iterating through the keys of your permission object you can use a more indicative name for each key, that states what this key represents. In your case type (or even permissionType) is much more explanatory than the generic key name.
  • the method's name createPerms, editPerms etc are not really useful because they do not describe what each method does. These methods toggle the permission of the corresponding type (e.g. createPerms toggles the value of create permission). So a more valid name would be toggleCreatePermission. (But remember you don't have to do this since we will get rid of these 4 methods and just create one accepting type as an argument)

Final code

 const [permissions, setPermissions] = useState({
    create: true,
    view: true,
    edit: true,
    delete: true
  });

  const togglePermission = (type) => {
    setPermissions({ ...permissions, [type]: !permissions[type] });
  };

  return (
    <>
      <h1>
        <center> User Permission </center>
      </h1>

      <table className="ui celled table">
        <thead>
          <tr>
            {Object.keys(permissions).map((type) => (
              <th key={type}>{type}</th>
            ))}
          </tr>
        </thead>
        <tbody>
          <tr>
            {Object.entries(permissions).map(([type, enabled]) => (
              <td key={type}>
                <input
                  type="checkbox"
                  onChange={() => togglePermission(type)}
                  checked={enabled}
                />
              </td>
            ))}
          </tr>
        </tbody>
      </table>

      {permissions.create && <h3>Create</h3>}
      {permissions.view && <h3>View</h3>}
      {permissions.edit && <h3>Edit</h3>}
      {permissions.delete && <h3>Delete</h3>}
    </>
);

Extra stuff

The enhancement described here is an overkill for you component currently, but i added it to demonstrate how you can make your code even more extensible in the future and for educational purposes

So another improvement that can possibly make your code more easily extensible would be to store your permission types in a constant object and create them dynamically. Let me elaborate on that.

Let's suppose you want to add one more permission to your component and let's name this new permission authorize. With the code above you need to add this new permission in your initial permissions state like authorize: true and add a new rendering section at the end to show if this is enabled or not like {permissions.authorize && <h3>Authorize</h3>}.

You can create a static object that would look like this:

const PERMISSIONS = {
  create: { name: 'Create', initial: true },
  view: { name: 'View', initial: true },
  edit: { name: 'Edit', initial: true },
  delete: { name: 'Delete', initial: true }
}

and then create your initial state dynamically once like below:

const INITIAL_STATE = Object.entries(PERMISSIONS).reduce((result, [type, permission]) => {
  result[type] = permission.initial;
  return result;
}, {})

and initialize your state: useState(INITIAL_STATE)

Then you can also change the permissions rendering section to make it fully dynamic and make it like below:

 Object.entries(permissions)
  .filter(([type, enabled]) => enabled)
  .map(([type]) => <h3>PERMISSIONS[type].name</h3>)

Having done all that you can add/remove permissions by just adding a new entry on your static PERMISSIONS object. For the new authorize example this would be:

const PERMISSIONS = {
  create: { name: 'Create', initial: true },
  view: { name: 'View', initial: true },
  edit: { name: 'Edit', initial: true },
  delete: { name: 'Delete', initial: true },
  authorize: { name: 'Authorize', initial: false }
}
\$\endgroup\$
2
  • \$\begingroup\$ Thank you so much for the explanation, this is the one I needed. :)) \$\endgroup\$ Nov 7 at 5:29
  • \$\begingroup\$ And also for the extra info yes I'll be possibly needing it for the future purpose \$\endgroup\$ Nov 7 at 5:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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