-2
\$\begingroup\$

I currently need some advice regarding the best practices and whether this is code is valid from a review perspective.

Needs some tips of what i should be writing like and whether there are any clear pointers on how i can improve.

var Structure = function () {
    var _this = this;
    this.Version = "1.0";

    this.GetDataFromCache = function (relatedNewsRequest) {
        _this.listRequest = _this.SetDefaultsProperties(relatedNewsRequest);
        return Product.AddIn.Cache.Get(Product.Digispace.SiteContext.WebId + _this.GetCacheKey());
    };

    this.GetCacheInterval = function () {
        var validCacheInterval =
            typeof (_this.listRequest.CacheInterval !== "undefined") && _this.listRequest.CacheInterval !== null && !isNaN(_this.listRequest.CacheInterval);
        if (validCacheInterval) {
            if (_this.listRequest.CacheInterval === -1) {
                return Product.Digispace.ConfigurationContext.CachingStrategyInterval;
            }
            else {
                return parseInt(_this.listRequest.CacheInterval);
            }
        }
        else {
            return 0;
        }
    };

    this.GetCacheKey = function () {
        return Product.Digispace.ConfigurationContext.getCacheKey(_this.listRequest.SenderId);
    };

    this.GetPropertyValue = function (requestIn, key, defaultValue) {
        var propertyValue = "";
        for (var prop in requestIn) {
            if (key.toLowerCase() === prop.toLowerCase()) {
                propertyValue = requestIn[prop];
                break;
            }
        }
        return (propertyValue === undefined || propertyValue.toString().trim() === "") ? defaultValue : propertyValue;
    };

    this.SetDefaultsProperties = function (requestIn) {
        var requestOut = requestIn;
        requestOut.SenderId = _this.GetPropertyValue(requestIn, "id", "");
        requestOut.rootWeb = _this.GetPropertyValue(requestIn, "isroot", true);
        requestOut.selectFields = _this.GetPropertyValue(requestIn, "selectfields", "ID,Title,TOImage,TOLink");
        requestOut.viewXml = _this.GetPropertyValue(requestIn, "viewxml",
            `<View>
                <Query>
                    <OrderBy><FieldRef Name='Title' Ascending='True' /></OrderBy>
                </Query>
            </View>`);

        return requestOut;
    };

    this.Init = function (properties) {
        _this.listRequest = _this.SetDefaultsProperties(properties);
        _this.listRequest.EditMode = Product.AddIn.Utilities.getEditMode();
        _this.listRequest.listControl = _this;
        window["Example.Widgets.Structure.GetLandingItems"] = _this.GetLandingItems;
        _this.Prerender();
        var control = new Product.AddIn.GenericListControlWidget();
        control.Init(_this.listRequest);
        _this.listControl = control;
    };

    this.Prerender = function () {
        Product.Digispace.AppPart.Eventing.Subscribe('/widget/updated/', _this.RefreshWidget, _this.listRequest.SenderId);
    };

    this.Render = function () {
        _this.listControl.Render();
    };

    this.RefreshWidget = function (newProps) {
        if (newProps.id === _this.listRequest.id) {
            newProps = _this.SetDefaultsProperties(newProps);
            var control = new Product.AddIn.GenericListControlWidget();
            control.Init(newProps);
            _this.listControl = control;
            _this.Render();
        }
    };

    this.GetLandingItems = function (data) {
        var dataItem, newItem, newData = {}, itemList = [], id = _this.listRequest.id;
        if (id === '12312-213123-513dasd') {
            newData.Header = 'Corporate Teams';
        } else {
            newData.Header = 'Structure';
        }

        try {
            for (var i = 0; i < data.Items.length; i++) {
                dataItem = data.Items[i];
                newItem = {
                    Name: dataItem.Title,
                    Url: dataItem.TOLink ? dataItem.TOLink.get_url() : '',
                    Image: dataItem.TOImage ? dataItem.TOImage.get_url() : ''
                };
                itemList.push(newItem);
            }
            newData.Items = itemList;
            newData.HasItems = newData.Items.length > 0 ? true : false;
            return newData;
        } catch (err) {
            console.log(err);
        }
    };
};

module.exports = Structure;
\$\endgroup\$

closed as off-topic by t3chb0t, Mast, Sᴀᴍ Onᴇᴌᴀ, Heslacher, Quill Nov 5 '18 at 6:46

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – t3chb0t, Mast, Sᴀᴍ Onᴇᴌᴀ, Heslacher, Quill
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 1
    \$\begingroup\$ What version of Node are you using? \$\endgroup\$ – FreezePhoenix Oct 27 '18 at 20:25
  • 2
    \$\begingroup\$ Welcome to Code Review. The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How to Ask for examples, and revise the title accordingly. \$\endgroup\$ – Zeta Oct 27 '18 at 21:32
  • \$\begingroup\$ Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site. Please take a look at the help center. \$\endgroup\$ – Mast Nov 4 '18 at 9:49
3
\$\begingroup\$

(Potential) Mistakes

typeof (_this.listRequest.CacheInterval !== "undefined")

This is not syntactically wrong, but it's not going to work the way you intended. This is going to return the type of the expression (_this.listRequest.CacheInterval !== "undefined"), which is of course a boolean. Therefore, the value of the expression typeof (_this.listRequest.CacheInterval !== "undefined") will always be the string 'boolean', which is a truthy value, so this condition will always evaluate as true and fail in its purpose as a short-circuit. You can just leave out the brackets:

typeof _this.listRequest.CacheInterval !== "undefined"

Or, in the case of undefined, you can write it even more simply:

_this.listRequest.CacheInterval !== undefined

Also, there's this line:

window["Example.Widgets.Structure.GetLandingItems"] = _this.GetLandingItems;

Again, this isn't wrong, but I think this isn't what you meant. If you want to access this property later, you'll be able to it with window["Example.Widgets.Structure.GetLandingItems"], but not window.Example.Widgets.Structure.GetLandingItems. I.e. this line will create a property on the window object with a very long name, and not a property on the Structure object called GetLandingItems.

If you want to use that line to define a static method on the class of your object, you can use the constructor property, which is available on all objects and refers to the constructor function:

this.constructor.GetLandingItems = _this.GetLandingItems;

Class declaration

The first thing I'd recommend is to read up on ES6 class syntax, which is the preferred way of declaring classes these days.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes

class Structure {
    constructor() {
        this.version = '1.0';
    }

    getDataFromCache(request) {
        // ...etc.
    }
}

Note however that this syntax still does not introduce private class members, and it doesn't make it possible to use a closure like you did. There's already a proposal in progress to introduce member visibility in JavaScript, but until then you might want to consider using TypeScript if you need that kind of control.

https://www.typescriptlang.org/

This is all completely optional though; functionally, your code looks good.

Style

These are just very minor things, but if you're going to work together with other JS developers, they will probably come up at some point.

  • We start method and member names with lowercase letters: getDataFromCache(), this.cacheInterval, etc.
  • We always delimit strings with single quotes '. Right now you're mixing single and double.

If you're using an editor such as VSCode, you can download a plugin called ESLint that marks all the style mistakes in your code.

https://code.visualstudio.com/

Hope that helps.

\$\endgroup\$
  • \$\begingroup\$ And you can find really good style guides (the most widely adopted from javascript community) here : github.com/airbnb/javascript \$\endgroup\$ – Hugeen Nov 2 '18 at 15:51

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