3
\$\begingroup\$

I am working on a simple Rails app. For the Expense resource I use a DataTable. So, in my CoffeeScript for this resource I basically do several things:

  • initialize the datatabe;
  • since I use a custom search field (not the one, provided by DataTables), I add a search function in the table api;
  • initialize the Select2 plugin;
  • check to see if the request is made from a mobile device - if yes -> change the type of an input field to date, otherwise initialize Datepicker
  • submit a from when the date from the datepicker is selected

Here is my code

$.fn.dataTable.ext.type.order['currency-bg-pre'] = parseCurrency

changeTextInputToDate = (input) ->
  input.each(-> $(@).clone().attr('type', 'date').insertBefore(@)).remove()

dataTableFooterCallback = (row, data, start, end, display) ->
    api = @api()
    total = api.column(1).data().reduce sumCurrency, 0
    $(api.column(1).footer()).html("#{total.toFixed(2)} лв")

initializeDatepicker = (element) ->
  element.datepicker
    autoclose: true
    clearBtn: true
    disableTouchKeyboard: true
    format: 'yyyy-mm-dd'
    orientation: 'top auto'
    todayBtn: true
    todayHighlight: true

initializeDateTable = (element) ->
  element.DataTable
    columns: [
        type: 'date',
        searchable: false
      ,
        type: 'currency-bg'
        searchable: false
      ,
        type: 'string'
      ,
        type: null
        orderable: false
        searchable: false
    ]
    footerCallback: dataTableFooterCallback
    order: [[0, 'desc']]
    paging: false
    dom: 't'

initializeSelect2 = (element) ->
  element.select2
    ajax:
      url: '/tags'
      data:
        (query) ->
          query: query.term
      processResults:
        (data) ->
          results: data
      cache: true
    placeholder: 'Enter tag(s)'
    tags: true
    theme: 'bootstrap'
    tokenSeparators: [',']

isMobile = ->
  check = false
  ((a) ->
    if /(android|bb\d+|meego).+mobile|avantgo|bada\/|blackberry|blazer|compal|elaine|fennec|hiptop|iemobile|ip(hone|od)|iris|kindle|lge |maemo|midp|mmp|mobile.+firefox|netfront|opera m(ob|in)i|palm( os)?|phone|p(ixi|re)\/|plucker|pocket|psp|series(4|6)0|symbian|treo|up\.(browser|link)|vodafone|wap|windows ce|xda|xiino/i.test(a) or /1207|6310|6590|3gso|4thp|50[1-6]i|770s|802s|a wa|abac|ac(er|oo|s\-)|ai(ko|rn)|al(av|ca|co)|amoi|an(ex|ny|yw)|aptu|ar(ch|go)|as(te|us)|attw|au(di|\-m|r |s )|avan|be(ck|ll|nq)|bi(lb|rd)|bl(ac|az)|br(e|v)w|bumb|bw\-(n|u)|c55\/|capi|ccwa|cdm\-|cell|chtm|cldc|cmd\-|co(mp|nd)|craw|da(it|ll|ng)|dbte|dc\-s|devi|dica|dmob|do(c|p)o|ds(12|\-d)|el(49|ai)|em(l2|ul)|er(ic|k0)|esl8|ez([4-7]0|os|wa|ze)|fetc|fly(\-|_)|g1 u|g560|gene|gf\-5|g\-mo|go(\.w|od)|gr(ad|un)|haie|hcit|hd\-(m|p|t)|hei\-|hi(pt|ta)|hp( i|ip)|hs\-c|ht(c(\-| |_|a|g|p|s|t)|tp)|hu(aw|tc)|i\-(20|go|ma)|i230|iac( |\-|\/)|ibro|idea|ig01|ikom|im1k|inno|ipaq|iris|ja(t|v)a|jbro|jemu|jigs|kddi|keji|kgt( |\/)|klon|kpt |kwc\-|kyo(c|k)|le(no|xi)|lg( g|\/(k|l|u)|50|54|\-[a-w])|libw|lynx|m1\-w|m3ga|m50\/|ma(te|ui|xo)|mc(01|21|ca)|m\-cr|me(rc|ri)|mi(o8|oa|ts)|mmef|mo(01|02|bi|de|do|t(\-| |o|v)|zz)|mt(50|p1|v )|mwbp|mywa|n10[0-2]|n20[2-3]|n30(0|2)|n50(0|2|5)|n7(0(0|1)|10)|ne((c|m)\-|on|tf|wf|wg|wt)|nok(6|i)|nzph|o2im|op(ti|wv)|oran|owg1|p800|pan(a|d|t)|pdxg|pg(13|\-([1-8]|c))|phil|pire|pl(ay|uc)|pn\-2|po(ck|rt|se)|prox|psio|pt\-g|qa\-a|qc(07|12|21|32|60|\-[2-7]|i\-)|qtek|r380|r600|raks|rim9|ro(ve|zo)|s55\/|sa(ge|ma|mm|ms|ny|va)|sc(01|h\-|oo|p\-)|sdk\/|se(c(\-|0|1)|47|mc|nd|ri)|sgh\-|shar|sie(\-|m)|sk\-0|sl(45|id)|sm(al|ar|b3|it|t5)|so(ft|ny)|sp(01|h\-|v\-|v )|sy(01|mb)|t2(18|50)|t6(00|10|18)|ta(gt|lk)|tcl\-|tdg\-|tel(i|m)|tim\-|t\-mo|to(pl|sh)|ts(70|m\-|m3|m5)|tx\-9|up(\.b|g1|si)|utst|v400|v750|veri|vi(rg|te)|vk(40|5[0-3]|\-v)|vm40|voda|vulc|vx(52|53|60|61|70|80|81|83|85|98)|w3c(\-| )|webc|whit|wi(g |nc|nw)|wmlb|wonu|x700|yas\-|your|zeto|zte\-/i.test(a.substr(0, 4))
      check = true
    return
  ) navigator.userAgent or navigator.vendor or window.opera
  check

parseCurrency = (amount) ->
  switch typeof amount
    when 'string' then amount.match(/\d+(?:\.\d+)?/)[0] * 1
    when 'number' then amount
    else 0

sumCurrency = (a, b) -> parseCurrency(a) + parseCurrency(b)

$ ->
  api = initializeDateTable $('.datatable')

  $('#search').on 'keyup', -> api.search(@value).draw()

  initializeSelect2 $('#expense_tag_list')

  if isMobile()
    changeTextInputToDate $('form input:text')
  else
    initializeDatepicker $('.input-group.date')

  $('.datepicker').on 'change', -> $('form').submit()

So what I do is basically this:

  • figure out what must happen on the page;
  • write the function that does that;
  • call it on page ready;

Although I have provided a particular example, I generally write JavaScript/CoffeeScript this way. Would you provide some basic example of how you would reorganize this code. In particular I am interested in using CoffeeScript's classes. Also, at what point does one decide that the code has become too messy and one needs to start using Angular/React etc.?

\$\endgroup\$
1
\$\begingroup\$

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 isMobile function 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, isMobile is just a boolean value. The do keyword means it's evaluated immediately. Note I've also added a "" fallback for userAgent just in case. Since you might call substr on userAgent, it must be a string, or the script will just fail.

  • You changeTextInputToDate doesn'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 this instead 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 the this object itself, I prefer to use this. Just personal preference.)

  • In dataTableFooterCallback it'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 in initializeDataTable, 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 the columns array, 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 the reduce callback. This makes for a simpler expression, and eliminates sumCurrency completely.
  • parseCurrency I'd write as

    parseCurrency = (value) ->
      switch typeof value
        when 'number' then value
        when 'string' then (value.match(/\d+(\.\d+)?/)?[0] or 0) * 1
        else 0
    

    The ?[0] or 0 guards against match returning null. Without the null-coalescing ? your script just fails as null[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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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