Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use trim method as trim isn't a property #9605

Conversation

bjarnef
Copy link
Contributor

@bjarnef bjarnef commented Jan 3, 2021

Prerequisites

  • I have added steps to test this contribution in the description below

Description

I noticed this PR #8213 introduced a private method isEmptyOrSpaces() which use trim as property instead of trim() as a method.

I also renamed the function to isNullOrWhitespace().

I considered adding this as a String prototype extension in Extensions.js, but is seems to throw an error if called as str.isNullOrWhitespace() and str is null.

For now I have omitted this, but it could be useful as we use check for null, undefined and empty string in many part of the code.

if (!String.prototype.isNullOrEmpty) {

    /** isNullOrEmpty extension method for string*/
    String.prototype.isNullOrEmpty = function () {
        var s = this;
        return s === null || s === null || s === "";
    };
}

if (!String.prototype.isNullOrWhitespace) {

    /** isNullOrWhitespace extension method for string*/
    String.prototype.isNullOrWhitespace = function (value) {
        var s = this;
        return s.isNullOrEmpty() || s.trim().length < 1;
    };
}

@nathanwoulfe
Copy link
Contributor

@bjarnef why not let JS do it's thing with type coercion, and check !str => this will be true when str is null, undefined or '' (since an empty string is falsy) .

I guess that would fail if the string was whitespace only, but probably also worth checking where in the codebase we'd allow a whitespace string to be set (doesn't make sense to let that happen).

Given all that, it's really two options - extension where we can't check null, or coercion where we don't check whitespace...

@nathanwoulfe
Copy link
Contributor

@bjarnef how about adding it as a method in Utilities.js - not as clean as an extension, but can handle null/undefined without issue

@bjarnef
Copy link
Contributor Author

bjarnef commented Jan 20, 2021

@nathanwoulfe yes, would could have a isNullOrUndefined (or isNullOrEmpty if it also check for an empty string like in C#) and maybe also isNullOrWhitespace which could replace this local function.

We already have a isUndefined function which could be re-used in new utility functions:
https://github.com/umbraco/Umbraco-CMS/blob/v8/contrib/src/Umbraco.Web.UI.Client/src/utilities.js#L47

Maybe @nul800sebastiaan and @nielslyngsoe have some input on this?

@bjarnef
Copy link
Contributor Author

bjarnef commented Jan 20, 2021

A isNullOrEmpty could e.g. be useful in this PR #8638 to check if disableDirtyCheck passed in to umbChecbox umbRadiobutton not is null or undefined.

@c9mb
Copy link

c9mb commented Jan 20, 2021

FWIW - I realize it's easy to create function clutter, but personally I find negating tests to be more readable in many cases - e.g. isNotNullOrEmpty()

@bjarnef
Copy link
Contributor Author

bjarnef commented Jan 20, 2021

@c9mbundy In that case I prefer to use ! (not) operator or === false, because it is similar to C# and also e.g. existing angular.isUndefined() function.

@nathanwoulfe
Copy link
Contributor

Also easy enough to add both versions, where one just returns the negated result of the other.

One of my first tasks in a new C# project is to add HasValue() and HasNoValue() string extensions, to roll up the IsNullOrEmpty and IsNullOrWhitespace check into convenience extensions. s.HasNoValue() just returns !s.HasValue() but helps to keep things readable, which is a win.

Long story short, no reason both versions couldn't exist.

@bjarnef
Copy link
Contributor Author

bjarnef commented Jan 20, 2021

@nathanwoulfe I have added two new functions to Utilities in 90ac70d

@nul800sebastiaan
Copy link
Member

I didn't fully read this, but the rule is: utilities.js should not become a class for throwing random stuff in, it was specifically meant to catch AngularJS methods that can be removed from the rest of the codebase so as to make the dependencies on AngularJS less.

Any new method you throw in there will need to be fully supported by HQ and we're not ready to commit to that.
I know I'm not very flexible here but we need to be careful that this doesn't turn into a huge class that does all the things. I would move the logic to a local function in umbeditorheader.directive.js. I know that means it won't be reusable but again: limit utilities.js to replacing functions we don't need from AngularJS.

A comment at the top of utilities.js to describe that is what it's for would be good 👍

@bjarnef
Copy link
Contributor Author

bjarnef commented Jan 20, 2021

@nul800sebastiaan yes, I agree with it could easily become a new "UmbracoHelper" class.
We already have extensions method in Extensions.js reused many places e.g. Object.toBoolean(). However it seems string extension method isNullOrEmpty and isNullOrWhitespace doesn't work on a value if it is null or undefined.

For example in this PR we have the same local toBool function in three different files.
#9607

I'll revert the change for now.

@bjarnef
Copy link
Contributor Author

bjarnef commented Jan 20, 2021

It might be something to consider in a rewrite of the backoffice though and mayve if TypeScript will be used.

I saw this example String.isNullOrEmpty(val):
https://stackoverflow.com/questions/17843215/typescript-extend-string-static

@nathanwoulfe @filipbech

@nathanwoulfe
Copy link
Contributor

Also worth remember too that a move to modular JS means we can import functions as required - either from NPM packages or including custom stuff.

@nul800sebastiaan nul800sebastiaan merged commit 5e4d326 into umbraco:v8/contrib Feb 21, 2021
@nul800sebastiaan
Copy link
Member

Alright, this one is good to go for now and I think, indeed, we'll be building a library for what we need in vNext.

Thanks for the updates in this PR @bjarnef! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants