1

So I have made a pull request to change this code:

//Controller:
        ...
        [HttpPost]
        public ActionResult Unauthorize(int userID, int permissionID)
        {
            _permissionService.UnauthorizeInternalUserPermission(userID, permissionID);
            return Json(new Result());
        }
        ...
//cshtml:
...
<script>
    window.permMaintenance = new PermMaintenance(
        {
            userIdColumnId: '@nameof(PermissionMaintenanceInternalUserDTO.UserID)',
            permissionIdColumnId: '@nameof(PermissionMaintenanceInternalUserDTO.PermissionID)',
        }
    );
</script>
...
//JavaScript:
        ...
        $.ajax({
            type: 'POST',
            url: me.urls.unauthorizeUrl,
            data: {
                userID: dataItem[me.constants.userIdColumnId],
                permissionID: dataItem[me.constants.permissionIdColumnId]
            },
            ...

into this:

//Controller:
        ...
        [HttpPost]
        public ActionResult Unauthorize(int userID, int permissionID)
        {
            _permissionService.UnauthorizeInternalUserPermission(userID, permissionID);
            return Json(new Result());
        }
        ...
//cshtml:
@{
    var unauthorizeParams = typeof(HomeController).GetMethod(nameof(HomeController.Unauthorize)).GetParameters();
}
<script>
    window.permMaintenance = new PermMaintenance(
        {
            userIdColumnId: '@nameof(PermissionMaintenanceInternalUserDTO.UserID)',
            permissionIdColumnId: '@nameof(PermissionMaintenanceInternalUserDTO.PermissionID)'
            userIdParameterId: '@unauthorizeParams[0].Name',
            permissionIdColumnId: '@nameof(PermissionMaintenanceInternalUserDTO.PermissionID)',
            permissionIdParameterId: '@unauthorizeParams[1].Name'
        }
    );
</script>
...
//JavaScript:
        ...
        var data = {};
        data[me.constants.userIdParameterId] = dataItem[me.constants.userIdColumnId];
        data[me.constants.permissionIdParameterId] = dataItem[me.constants.permissionIdColumnId];
        $.ajax({
            type: 'POST',
            url: me.urls.unauthorizeUrl,
            data: data,
            ...

A coworker brought forth the opinion that all I've done is make the code harder to read for no gain; that if the API (specifically, the names of the Controller method's parameters) change, then we have to change the JavaScript as well.

However, I am of the opinion that it's cleaner to decouple the JavaScript from the C#. Though, I am on the fence since this isn't a perfect solution - if we swapped the order of the parameters, for example, we would get a nasty runtime bug

What would best practices say here?

1 Answer 1

1

It is harder to read, it is also more dynamic.

The real question: Is it worth it?

On the one hand you will spend more time trying to read this code. If this code becomes the de-facto standard (and perhaps described) that effort can be reduced, but it will still slow down programmers new to the code base.

It also runs a little slower hopefully just at build time, but more likely at run time as type information must be accessed. It also makes the javascript more brittle as it relies on me.constants.userIdParameterId and me.constants.permissionIdParameterId being unique, well-defined, and correct. There is a lack of validation and error handling in the javascript around those introduced requirements.

The sole bonus is that there is exactly one place that actually encodes the name (excluding any automated tests).


I would actually go the otherway here.

//Controller:
        ...
        [HttpPost]
        public ActionResult Unauthorize(int userID, int permissionID)
        {
            _permissionService.UnauthorizeInternalUserPermission(userID, permissionID);
            return Json(new Result());
        }
        ...
//JavaScript:
        ...
        $.ajax({
            type: 'POST',
            url: this.unauthorizeUrl,
            data: {
                userID: dataItem['UserID'],
                permissionID: dataItem['PermissionID']
            },
       ...

Because if you've constructed the javascript well, that $.ajax() resides in a single class. That simplifies encapsulation as the url can be a property of that object assigned by the constructor (or later for testing), and the data layout is already strictly known. Therefore we can forgo globals, and all of the coupling that they impose.

Also in favour here is that there are only two files containing relevant information (down from three).

This also addresses your concern. There is no coupling between the C# and the javascript here. There is still a dependency as both are being constrained by the communication protocol (http post with a given data layout), but you could easily swap the c# or javascript for anything else, and it will still work.

It also addresses your colleagues concern. I would argue that this is easier to read.

Your Answer

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

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