The
isMobilefunction doesn't need to be a function with a nested IIFE and a closure. Just run it:isMobile = do -> userAgent = navigator.userAgent or navigator.vendor or window.opera or "" /.../.test(userAgent) or /.../.test(userAgent.substr(0,4))Now,
isMobileis just a boolean value. Thedokeyword means it's evaluated immediately. Note I've also added a""fallback foruserAgentjust in case. Since you might callsubstronuserAgent, it must be a string, or the script will just fail.You
changeTextInputToDatedoesn't actually change the inputs, really. It replaces them. But changing them seems much easier:changeTextInputToDate = (inputs) -> inputs.each -> $(this).attr type: 'date'(I'm using
thisinstead of@just because I always think@looks weird by itself. I like using it as a prefix for things, but if I'm referring to thethisobject itself, I prefer to usethis. Just personal preference.)In
dataTableFooterCallbackit'd be nicer to fetch the column once, since that's what you need for the expressions. I'd also make the footer callback a nested function ininitializeDataTable, too. Especially as that function references@api, which doesn't make sense except in the context of a data table callback. So there's no reason for the function to float around in a wider scope:initializeDateTable = (element) -> footerCallback = -> column = @api().column(1) total = column.data().map(parseCurrency).reduce (sum, value) -> sum + value $(column.footer()).html "#{total.toFixed(2)} лв" element.DataTable columns: [ { type: 'date', searchable: false } { type: 'currency-bg', searchable: false } { type: 'string' } { type: null, orderable: false, searchable: false } ] footerCallback: footerCallback order: [[0, 'desc']] paging: false dom: 't'You'll note a few more changes:
- I'm using explicit
{and}around objects in thecolumnsarray, rather than relying solely on whitespace - makes it easier to read, I think. - I'm using
map+reduce, rather than doing all the work in thereducecallback. This makes for a simpler expression, and eliminatessumCurrencycompletely.
- I'm using explicit
parseCurrencyI'd write asparseCurrency = (value) -> switch typeof value when 'number' then value when 'string' then (value.match(/\d+(\.\d+)?/)?[0] or 0) * 1 else 0The
?[0] or 0guards againstmatchreturningnull. Without the null-coalescing?your script just fails asnull[0]doesn't work.
The
isMobilefunction doesn't need to be a function with a nested IIFE and a closure. Just run it:isMobile = do -> userAgent = navigator.userAgent or navigator.vendor or window.opera or "" /.../.test(userAgent) or /.../.test(userAgent.substr(0,4))Now,
isMobileis just a boolean value. Thedokeyword means it's evaluated immediately. Note I've also added a""fallback foruserAgentjust in case. Since you might callsubstronuserAgent, it must be a string, or the script will just fail.You
changeTextInputToDatedoesn't actually change the inputs, really. It replaces them. But changing them seems much easier:changeTextInputToDate = (inputs) -> inputs.each -> $(this).attr type: 'date'(I'm using
thisinstead of@just because I always think@looks weird by itself. I like using it as a prefix for things, but if I'm referring to thethisobject itself, I prefer to usethis. Just personal preference.)In
dataTableFooterCallbackit'd be nicer to fetch the column once, since that's what you need for the expressions. I'd also make the footer callback a nested function ininitializeDataTable, too. Especially as that function references@api, which doesn't make sense except in the context of a data table callback. So there's no reason for the function to float around in a wider scope:initializeDateTable = (element) -> footerCallback = -> column = @api().column(1) total = column.data().map(parseCurrency).reduce (sum, value) -> sum + value $(column.footer()).html "#{total.toFixed(2)} лв" element.DataTable columns: [ { type: 'date', searchable: false } { type: 'currency-bg', searchable: false } { type: 'string' } { type: null, orderable: false, searchable: false } ] footerCallback: footerCallback order: [[0, 'desc']] paging: false dom: 't'You'll note a few more changes:
- I'm using explicit
{and}around objects in thecolumnsarray, rather than relying solely on whitespace - makes it easier to read, I think. - I'm using
map+reduce, rather than doing all the work in thereducecallback. This makes for a simpler expression, and eliminatessumCurrencycompletely.
- I'm using explicit
parseCurrencyI'd write asparseCurrency = (value) -> switch typeof value when 'number' then value when 'string' then (value.match(/\d+(\.\d+)?/)?[0] or 0) * 1 else 0The
?[0] or 0guards againstmatchreturningnull. Without the null-coalescing?your script just fails asnull[0]doesn't work.
The
isMobilefunction doesn't need to be a function with a nested IIFE and a closure. Just run it:isMobile = do -> userAgent = navigator.userAgent or navigator.vendor or window.opera or "" /.../.test(userAgent) or /.../.test(userAgent.substr(0,4))Now,
isMobileis just a boolean value. Thedokeyword means it's evaluated immediately. Note I've also added a""fallback foruserAgentjust in case. Since you might callsubstronuserAgent, it must be a string, or the script will just fail.You
changeTextInputToDatedoesn't actually change the inputs, really. It replaces them. But changing them seems much easier:changeTextInputToDate = (inputs) -> inputs.each -> $(this).attr type: 'date'(I'm using
thisinstead of@just because I always think@looks weird by itself. I like using it as a prefix for things, but if I'm referring to thethisobject itself, I prefer to usethis. Just personal preference.)In
dataTableFooterCallbackit'd be nicer to fetch the column once, since that's what you need for the expressions. I'd also make the footer callback a nested function ininitializeDataTable, too. Especially as that function references@api, which doesn't make sense except in the context of a data table callback. So there's no reason for the function to float around in a wider scope:initializeDateTable = (element) -> footerCallback = -> column = @api().column(1) total = column.data().map(parseCurrency).reduce (sum, value) -> sum + value $(column.footer()).html "#{total.toFixed(2)} лв" element.DataTable columns: [ { type: 'date', searchable: false } { type: 'currency-bg', searchable: false } { type: 'string' } { type: null, orderable: false, searchable: false } ] footerCallback: footerCallback order: [[0, 'desc']] paging: false dom: 't'You'll note a few more changes:
- I'm using explicit
{and}around objects in thecolumnsarray, rather than relying solely on whitespace - makes it easier to read, I think. - I'm using
map+reduce, rather than doing all the work in thereducecallback. This makes for a simpler expression, and eliminatessumCurrencycompletely.
- I'm using explicit
parseCurrencyI'd write asparseCurrency = (value) -> switch typeof value when 'number' then value when 'string' then (value.match(/\d+(\.\d+)?/)?[0] or 0) * 1 else 0The
?[0] or 0guards againstmatchreturningnull. Without the null-coalescing?your script just fails asnull[0]doesn't work.
I can't tell you when something like Angular/React will make more sense. It's a cost/benefit question, but they're your costs and your benefits.
I'd say you code is okay. In some places it seems overly terse, while in others it's spread out too much. And there are a few places where you perhaps abuse CoffeeScript syntax to the point where it becomes harder to read.
There's also a number of things that can just be simplified:
The
isMobilefunction doesn't need to be a function with a nested IIFE and a closure. Just run it:isMobile = do -> userAgent = navigator.userAgent or navigator.vendor or window.opera or "" /.../.test(userAgent) or /.../.test(userAgent.substr(0,4))Now,
isMobileis just a boolean value. Thedokeyword means it's evaluated immediately. Note I've also added a""fallback foruserAgentjust in case. Since you might callsubstronuserAgent, it must be a string, or the script will just fail.You
changeTextInputToDatedoesn't actually change the inputs, really. It replaces them. But changing them seems much easier:changeTextInputToDate = (inputs) -> inputs.each -> $(this).attr type: 'date'(I'm using
thisinstead of@just because I always think@looks weird by itself. I like using it as a prefix for things, but if I'm referring to thethisobject itself, I prefer to usethis. Just personal preference.)In
dataTableFooterCallbackit'd be nicer to fetch the column once, since that's what you need for the expressions. I'd also make the footer callback a nested function ininitializeDataTable, too. Especially as that function references@api, which doesn't make sense except in the context of a data table callback. So there's no reason for the function to float around in a wider scope:initializeDateTable = (element) -> footerCallback = -> column = @api().column(1) total = column.data().map(parseCurrency).reduce (sum, value) -> sum + value $(column.footer()).html "#{total.toFixed(2)} лв" element.DataTable columns: [ { type: 'date', searchable: false } { type: 'currency-bg', searchable: false } { type: 'string' } { type: null, orderable: false, searchable: false } ] footerCallback: footerCallback order: [[0, 'desc']] paging: false dom: 't'You'll note a few more changes:
- I'm using explicit
{and}around objects in thecolumnsarray, rather than relying solely on whitespace - makes it easier to read, I think. - I'm using
map+reduce, rather than doing all the work in thereducecallback. This makes for a simpler expression, and eliminatessumCurrencycompletely.
- I'm using explicit
parseCurrencyI'd write asparseCurrency = (value) -> switch typeof value when 'number' then value when 'string' then (value.match(/\d+(\.\d+)?/)?[0] or 0) * 1 else 0The
?[0] or 0guards againstmatchreturningnull. Without the null-coalescing?your script just fails asnull[0]doesn't work.
Finally, I'm not sure I understand these last lines:
if isMobile()
changeTextInputToDate $('form input:text')
else
initializeDatepicker $('.input-group.date')
$('.datepicker').on 'change', -> $('form').submit()
So, if it's a mobile client you change all text inputs in any form. If it's not, you initialize a datepicker with a completely different - and much more specific - selector. It just reads as "either do this, or do something completely different". Why aren't the selectors more similar? I'd imagine it's the same inputs you're dealing with.
And finally, the change event listener. That's yet another selector!
So that's confusing.