TEAMMATES / teammates

This is the project website for the TEAMMATES feedback management tool for education
https://teammatesv4.appspot.com/
GNU General Public License v2.0
1.66k stars 3.29k forks source link

The function isBlank in multiple JavaScript files can be simplified #3922

Closed junhaoyap closed 8 years ago

junhaoyap commented 9 years ago

screen shot 2015-07-08 at 4 47 14 pm

Regex can be slow, not everyone understands regex (I don't, yet) and the check can be as simple as

str.trim() === "", we don't need to add an extra layer of function to do that particular check

wkurniawan07 commented 9 years ago

I did a search on the workspace, apparently this method is stolen from typeahead.bundle.min.js. :stuck_out_tongue:

damithc commented 9 years ago

Any benefit of regex check over the simpler comparison?

wkurniawan07 commented 9 years ago

Referring to this doc, this is how trim is defined:

return this.replace(/^[\s\uFEFF\xA0]+|[\s\uFEFF\xA0]+$/g, '');

So trim itself is using regex, but the implementation is hidden from us (which can be a good or a bad thing). str.trim() === "" does look simpler, though.

damithc commented 9 years ago

OK then, we'll go for the simpler one.

wkurniawan07 commented 9 years ago

I just tried something. The regex method can handle things like:

isBlank(0) // true
isBlank(123) // false
isBlank(true) // false

Whereas the trim method will throw errors on the above three cases. Proper usage of the method won't require us to handle those cases, but in JS where variables are untyped, we may need some sort of protection against such misuses. Should we keep the regex method?

Gisonrg commented 8 years ago

@wkurniawan07 Do you think it's a good idea that we also test if the given input is a String? If it is not a String then let the method return false. In that way we could prevent misuses, and the trim() method could be used as well :P

wkurniawan07 commented 8 years ago

Oops sorry I somehow missed this! @Gisonrg yeah sure, that's one way to deal with it. You wanna work on it? Btw, if you haven't done one d.FirstTimers issue before, you should do one of those first.

Gisonrg commented 8 years ago

@wkurniawan07 Yea sure I can work on this one I will do one d.FirstTimers on weekend. Quite busy this week :smile:

wkurniawan07 commented 8 years ago

Suit yourself bro :+1: Just submit PRs when you're done. Remember to follow the workflow as specified here.