Skip to content

Lodash methods and helper functions#149

Closed
alexanderroidl wants to merge 6 commits intoorangecoding:masterfrom
alexanderroidl:refactor/lodash-and-helper-functions
Closed

Lodash methods and helper functions#149
alexanderroidl wants to merge 6 commits intoorangecoding:masterfrom
alexanderroidl:refactor/lodash-and-helper-functions

Conversation

@alexanderroidl
Copy link
Contributor

@alexanderroidl alexanderroidl commented Jul 26, 2025

This MR aims to refactor by:

  • Switching from our nullOrEmpty helper functions to check for truthy or falsy values natively.
  • Switching some methods on our end for Lodash methods as it's a dependency of ours anyways.

return;
}
if (nullOrEmpty(username) || nullOrEmpty(password) || nullOrEmpty(password2)) {
if (!username || !password || !password2) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the previous pr. your check is not the same as mine.

let a = 0;
nullOrEmpty(a); //false
!a; //true

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in my case I want to explicitly check for null or empty

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, I changed it back to the same logic but slimer:

const nullOrEmpty = (str) => str == null || str.length === 0;

is the same as

const nullOrEmpty = (str) => Boolean(str?.length);

as the if-block will convert everything inside it's brackets to a boolean value for checking, we can inline it:

  if (!username?.length || !password?.length || !password2?.length) {

Let me know if that works for you.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that is not true... (Or I'm missing something here):
Your version: const nullOrEmpty = (str) => Boolean(str?.length);
returns false on empty string.

const nullOrEmpty = (str) => Boolean(str?.length);
console.log(nullOrEmpty(''); //false, but should be true

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I typed it the wrong way, I meant

const nullOrEmpty = (str) => !Boolean(str?.length);

as an if-block implicitly converts statements inside it to a boolean for checking it, we can check for !str?.length directly instead of implementing and calling isNullOrEmpty(str)

function duringWorkingHoursOrNotSet(config, now) {
const {workingHours} = config;
if (workingHours == null || nullOrEmpty(workingHours.from) || nullOrEmpty(workingHours.to)) {
if (!workingHours?.from || !workingHours?.to) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd consider checking for null or empty by doing this truthy/falsy check relatively unsafe for reason mentioned above. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're making a good point generally!

In this case I think we're still good though as the check will only be true when workingHours

  • is an object (=neither null, nor undefined)
  • has a from property
  • the content of that property is truthy

Which would actually make the check more complex and safer here.

> console.log(undefined?.test)
undefined
> console.log(Boolean(undefined?.test))
false
> console.log(""?.test)
undefined
> console.log(Boolean(""?.test))
false
> console.log(''?.test23abc)
undefined
> console.log(Boolean(''?.test23abc))
false

Copy link
Owner

@orangecoding orangecoding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added 1 comment. I think your "nullOrEmpty" check does not work as intended. Also there are now a couple of conflicts. Would you mind resolving them? :)

Copy link
Owner

@orangecoding orangecoding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pr results in Test failures. Tbh, I'm loving what you have contributed so far, but I'm not convinced about this one...

@orangecoding
Copy link
Owner

@alexanderroidl I'd close this pr as I don't see any activity. Thanks a lot tho

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants