current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 0 down vote favorite

So I have created this function that will add a disk if needed and it takes a disk and a size as inputs. The idea is that I need to update data.disks with a few properties. Disk is called from an external request and it follows the pattern of "disk1", "disk2" and so on. Looking at it there are a few hard-coded values and some that are null. However I left them on purpose to show the structure of data.disk for the next person that might read it or modify it. Because of this it looks verbose and I don't really like it all that much. Do you see a way to make this function more readable/better in any way?

function addDisk(disk, size) {
    var diskNumber = disk.slice(-1);
    var diskLabel = "Hard disk " + diskNumber;
    var diskIndex = diskNumber - 1;

    //Updating JSON data object
    data.disks[diskIndex] = {
        "componentTypeId" : "com.vmware.csp.iaas.blueprint.service",
        "componentId" : null,
        "classId" : "Infrastructure.Compute.Machine.MachineDisk",
        "typeFilter" : null,
        "data" : {
            "capacity" : size,
            "custom_properties" : null,
            "id" : null,
            "initial_location" : "",
            "is_clone" : false,
            "label" : diskLabel,
            "storage_reservation_policy" : "",
            "userCreated" : true,
            "volumeId" : diskIndex
        }
    }
    log(diskLabel + " has capacity of: " + size + " GBs");
}
share|improve this question
    
Could you add more context to the post e.g how this is called , it's functionality. – Tolani Jaiye-Tikolo yesterday
1  
Or at least provide how you would call this, especially the format of disk does not seem evident – konijn yesterday
    
I've added details to make this case more clear – Magnum yesterday
up vote 3 down vote

You could define a default disk object, so you can show all fields on default values and document the values allowed for each:

var DiskSkelethon = {
    "componentTypeId" : "com.vmware.csp.iaas.blueprint.service",
    "componentId" : null,
    "classId" : "Infrastructure.Compute.Machine.MachineDisk",
    "typeFilter" : null,
    "data" : {
        "capacity" : -1,
        "custom_properties" : null,
        "id" : null,
        "initial_location" : "",
        "is_clone" : false,
        "label" : "",
        "storage_reservation_policy" : "",
        "userCreated" : true,
        "volumeId" : -1
    }
}

Then in your function you will make a copy of it and override just the fields you're changing:

function addDisk(disk, size) {
    var diskNumber = parseInt(disk.slice(-1), 10);
    if (isNaN(diskNumber)) {
       log("some error...");
       return false;
    }
    var diskLabel = "Hard disk " + diskNumber;
    var diskIndex = diskNumber - 1;

    //Updating JSON data object
    var diskData = Object.assign({}, DiskSkelethon)

    diskData.data.capacity = size;
    diskData.data.label = diskLabel;
    diskData.data.volumeId = diskIndex

    data.disks[diskIndex] = diskData
    log(diskLabel + " has capacity of: " + size + " GBs");
}
share|improve this answer
    
+1, great idea. Though, Skeleton has no 'h' ;) I would go with diskTemplate – konijn yesterday
2  
Warning: Object.assign doesn't do deep copy. – Arnial yesterday
    
@Arnial I test the case on nodejs and worked well, if the object structure became more complex it should be reconsidered. – Mario Alexandro Santini yesterday
up vote 1 down vote

I think this code is fine, if you use it professionally I would

  • Document that size must be provided in Gigs
  • Verify that size actually is a number, or can be parsed as a number, throw an exception otherwise
  • Document how the parameter disk must be formatted

  • Verify that disk.slice(-1) is a number, or can be parsed as a number, throw an exception otherwise

  • Note that data.disks seems a global variable, personally, I would have prefered to see a setter function for updating a disk object.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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