5
\$\begingroup\$

I've been playing around with Jekyll and CoffeeScript the last few days and made a Periodic Table to learn those two.

Now, to create the Periodic Table, I take an array of lis as input which then replaces itself with a table using a custom format like so:

periodicTable = [
    [1, "s:16", 2],
    [3, 4, "s:10", 5, "-", 10],
    [11, 12, "s:10", 13, "-", 18],
    [19, "-", 36],
    [37, "-", 54],
    [55, 56, "*", 72, "-", 86],
    [87, 88, "**", 104, "-", 118],
    ["s:18"],
    ["s:2", "*", 57, "-", 71],
    ["s:2", "**", 89, "-", 103]
]
  • s:n means that the next td should be empty with a colspan of n
  • "-" is a range indicator, meaning the next and previous items are the start and end of the range respectively

To create the actual table, the following CoffeeScript file is used:

window.PeriodicTable = {}

periodicTable = [
    [1, "s:16", 2],
    [3, 4, "s:10", 5, "-", 10],
    [11, 12, "s:10", 13, "-", 18],
    [19, "-", 36],
    [37, "-", 54],
    [55, 56, "*", 72, "-", 86],
    [87, 88, "**", 104, "-", 118],
    ["s:18"],
    ["s:2", "*", 57, "-", 71],
    ["s:2", "**", 89, "-", 103]
]

spacerIdentifier = "s"
rangeSymbol = "-"

# Get a list item searching by atomic number
PeriodicTable.getListItemByAtomicNumber = (list, atomicNumber) ->
    i = 0
    while i < list.length
        item = list[i]        

        if !item.nodeName
            continue

        if +item.dataset['atomicNumber'] == atomicNumber
            return item

        i++

    return null


# Create the periodic table
PeriodicTable.createTable = (list) -> 
    table = document.createElement "table"
    table.className = "periodic-table"

    ul = list[0].parentNode

    for i of periodicTable
        row = PeriodicTable.createRow list, periodicTable[i]
        table.appendChild row

    ul.parentNode.insertBefore table, ul

    return true


# Create a table row
PeriodicTable.createRow = (list, items) ->
    row = document.createElement "tr"

    i = 0
    skipNext = false

    while i < items.length

        if skipNext
            skipNext = false
            i++
            continue

        item = items[i]

        # Not a number has a special meaning
        if isNaN item
            if item.indexOf(spacerIdentifier) != -1
                # Item is a spacer
                col = PeriodicTable.createCol list, "", +(item.substring(2))

            else if item == rangeSymbol
                # Item indicates range 
                col = PeriodicTable.createColSequence list, items[i - 1], items[i + 1]
                skipNext = true
            else
                # Item has some other special meaning, just print it
                col = PeriodicTable.createCol list, item

        # If the next one is a range symbol, tell the script to skip this one
        else if items[i + 1] == rangeSymbol
            i++
            continue

        # Print the column
        else
            col = PeriodicTable.createCol list, item


        if !(col instanceof Array)
            col = [col]

        row.appendChild c for c in col

        i++

    return row


# Create a column
PeriodicTable.createCol = (list, atomicNumber, offset) ->
    col = document.createElement "td"

    if !isNaN atomicNumber
        listContent = PeriodicTable.getContentFromListItem(list, atomicNumber)

        if typeof listContent == "string"
            content = listContent

        else
            content = listContent.content
            col.className = PeriodicTable.getClassName listContent.type

    else
        content = atomicNumber

    col.innerHTML = content || "&nbsp;"

    if offset != undefined && !isNaN offset
        col.setAttribute "colspan", offset
        col.className = 'spacer'

    return col

# Create a range of columns
PeriodicTable.createColSequence = (list, start, end) ->
    cols = []

    i = start
    while i <= end
        col = PeriodicTable.createCol list, i
        cols.push col
        i++

    return cols


# Get the content from the list item with said atomic number
PeriodicTable.getContentFromListItem = (list, atomicNumber) ->
    li = PeriodicTable.getListItemByAtomicNumber list, atomicNumber

    if li         
        atomicNumber = li.dataset["atomicNumber"]
        symbol = li.dataset["symbol"]
        element = li.innerText
        elementType = li.dataset["elementType"]
        href = (li.getElementsByTagName "a")[0].getAttribute("href")

        return {
            content: "<a href=\"" + href + "\"><p>" + element + "</p><p><span>" + atomicNumber + "</span><span>" + symbol + "</span></p></a>",
            type: elementType
        }

    else
        return ""


# Get the class name from the element type
PeriodicTable.getClassName = (type) ->    
    type.replace(/\s/g, "-").toLowerCase()

list = document.querySelectorAll "#elements li"
PeriodicTable.createTable list
ul = document.getElementById "elements"

ul.parentNode.removeChild ul

A list item is built up like so:

<li data-symbol="H" data-atomic-number="1" data-element-type="Other non-metal">
    <a href="/element/hydrogen">Hydrogen</a>
</li>

I'm now looking to optimize the script, like some way to turn all those while loops into for loops (I tried for in, but that resulted in 2 extra items being taken, the length of the array and something with function item() { [native code] }) without using the backticks to escape JavaScript.

I created a GitHub repo for the project.

Could anyone give me a few pointers on this?

table { border-collapse: collapse; }
td { border: 1px solid black; }
<script type="text/coffeescript">
window.PeriodicTable = {}

periodicTable = [
    [1, "s:16", 2],
    [3, 4, "s:10", 5, "-", 10],
    [11, 12, "s:10", 13, "-", 18],
    [19, "-", 36],
    [37, "-", 54],
    [55, 56, "*", 72, "-", 86],
    [87, 88, "**", 104, "-", 118],
    ["s:18"],
    ["s:2", "*", 57, "-", 71],
    ["s:2", "**", 89, "-", 103]
]

spacerIdentifier = "s"
rangeSymbol = "-"

# Get a list item searching by atomic number
PeriodicTable.getListItemByAtomicNumber = (list, atomicNumber) ->
    i = 0
    while i < list.length
        item = list[i]        
        
        if !item.nodeName
            continue
        
        if +item.dataset['atomicNumber'] == atomicNumber
            return item
        
        i++
        
    return null


# Create the periodic table
PeriodicTable.createTable = (list) -> 
    table = document.createElement "table"
    table.className = "periodic-table"
    
    ul = list[0].parentNode
    
    for i of periodicTable
        row = PeriodicTable.createRow list, periodicTable[i]
        table.appendChild row
        
    ul.parentNode.insertBefore table, ul
        
    return true

   
# Create a table row
PeriodicTable.createRow = (list, items) ->
    row = document.createElement "tr"
    
    i = 0
    skipNext = false
    
    while i < items.length
        
        if skipNext
            skipNext = false
            i++
            continue
        
        item = items[i]
        
        # Not a number has a special meaning
        if isNaN item
            if item.indexOf(spacerIdentifier) != -1
                # Item is a spacer
                col = PeriodicTable.createCol list, "", +(item.substring(2))

            else if item == rangeSymbol
                # Item indicates range 
                col = PeriodicTable.createColSequence list, items[i - 1], items[i + 1]
                skipNext = true
            else
                # Item has some other special meaning, just print it
                col = PeriodicTable.createCol list, item

        # If the next one is a range symbol, tell the script to skip this one
        else if items[i + 1] == rangeSymbol
            i++
            continue
        
        # Print the column
        else
            col = PeriodicTable.createCol list, item
               
        
        if !(col instanceof Array)
            col = [col]

        row.appendChild c for c in col
        
        i++
        
    return row
        

# Create a column
PeriodicTable.createCol = (list, atomicNumber, offset) ->
    col = document.createElement "td"
    
    if !isNaN atomicNumber
        listContent = PeriodicTable.getContentFromListItem(list, atomicNumber)
        
        if typeof listContent == "string"
            content = listContent
            
        else
            content = listContent.content
            col.className = PeriodicTable.getClassName listContent.type
        
    else
        content = atomicNumber
    
    col.innerHTML = content || "&nbsp;"
    
    if offset != undefined && !isNaN offset
        col.setAttribute "colspan", offset
        col.className = 'spacer'
    
    return col
    
# Create a range of columns
PeriodicTable.createColSequence = (list, start, end) ->
    cols = []
    
    i = start
    while i <= end
        col = PeriodicTable.createCol list, i
        cols.push col
        i++
        
    return cols
        

# Get the content from the list item with said atomic number
PeriodicTable.getContentFromListItem = (list, atomicNumber) ->
    li = PeriodicTable.getListItemByAtomicNumber list, atomicNumber
    
    if li         
        atomicNumber = li.dataset["atomicNumber"]
        symbol = li.dataset["symbol"]
        element = li.innerText
        elementType = li.dataset["elementType"]
        href = (li.getElementsByTagName "a")[0].getAttribute("href")
        
        return {
            content: "<a href=\"" + href + "\"><p>" + element + "</p><p><span>" + atomicNumber + "</span><span>" + symbol + "</span></p></a>",
            type: elementType
        }
        
    else
        return ""
    

# Get the class name from the element type
PeriodicTable.getClassName = (type) ->    
    type.replace(/\s/g, "-").toLowerCase()
        
list = document.querySelectorAll "#elements li"
PeriodicTable.createTable list
ul = document.getElementById "elements"

ul.parentNode.removeChild ul
</script>
<script type="text/javascript" src="http://cdnjs.cloudflare.com/ajax/libs/coffee-script/1.1.2/coffee-script.min.js"></script>

<ul id="elements">
    <li data-symbol="H" data-atomic-number="1" data-element-type="Other non-metal">
        <a href="/element/hydrogen">Hydrogen</a>
    </li>
    <li data-symbol="He" data-atomic-number="2" data-element-type="Noble gas">
        <a href="/element/lithium">Helium</a>
    </li>
    <li data-symbol="Li" data-atomic-number="3" data-element-type="Alkali metal">
        <a href="/element/lithium">Lithium</a>
    </li>
    <li data-symbol="Be" data-atomic-number="4" data-element-type="Alkaline earth metal">
        <a href="/element/beryllium">Beryllium</a>
    </li>
    <li data-symbol="B" data-atomic-number="5" data-element-type="Metalloid">
        <a href="/element/boron">Boron</a>
    </li>
    <li data-symbol="C" data-atomic-number="6" data-element-type="Other non-metal">
        <a href="/element/carbon">Carbon</a>
    </li>
    <li data-symbol="N" data-atomic-number="7" data-element-type="Other non-metal">
        <a href="/element/nitrogen">Nitrogen</a>
    </li>
    <li data-symbol="O" data-atomic-number="8" data-element-type="Other non-metal">
        <a href="/element/oxygen">Oxygen</a>
    </li>
    <li data-symbol="F" data-atomic-number="9" data-element-type="Other non-metal">
        <a href="/element/fluorine">Fluorine</a>
    </li>
    <li data-symbol="Ne" data-atomic-number="10" data-element-type="Noble gas">
        <a href="/element/neon">Neon</a>
    </li>
</ul>

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! Nice question. I've added a live demo to aid reviewers — feel free to fill in some details within the demo. \$\endgroup\$ – 200_success Dec 25 '14 at 22:42
2
\$\begingroup\$

CoffeeScript or otherwise, it does seem like you're working a bit too hard here.

Since the information you use is split between the list in the HTML and the array in the script, there's some coupling between the two and some busywork in weaving the two together. Optimally, you'd have all the information in one place to begin with, but I figure the idea is that the list already exists, and you're trying to lay it out as a table.

But in order to get closer to the single source of data, we can parse the list and the layout array first, and then go about building the table from that.

In the case of the array, we just want to expand the ranges (leaving spaces as-is), and for the list, we want to extract the stuff we need from content and data-* attributes and, say, put it in an object using numbers as keys.

Basically, instead of trying to do everything while building the table, we prepare the data first.

In terms of style, you're following good JavaScript style by creating a PeriodicTable namespace, but that isn't really necessary (at least not in the same way) in CoffeeScript. Everything is compiled inside an IIFE, so you can treat everything as local as simply expose what you want to expose. E.g.:

foo = -> ...
bar = -> ...
baz = -> ...

# expose some functions (if necessary)
window.someNamespace = {foo, bar}

The baz function will remain "private", while foo and bar will become available globally as someNamespace.foo and someNamespace.bar respectively. Meanwhile, within your script, you don't have to worry about prefixing everything with a namespace.

In this case, though, I'm not sure you actually need to expose anything - it seems pretty self-contained.

You've also got a bunch of explicit return keywords which aren't necessary.

The table you're building also contains invalid HTML. You're wrapping p elements in an a element, which isn't quite kosher, since a is an inline element, and p is a block-level element. You've also got bunch of span elements, which I'm not sure are necessary. They just seem to add an extra level to the nesting.

Lastly, I'd suggest using jQuery since you're dealing with the DOM, and perhaps underscore.js or lo-dash to help with the parsing. I'll use jQuery below just to simplify element construction.


The array can be expanded, row by row, like so (hey that rhymes)

expandLayout = (layout) ->
  # internal function to expand a row
  expandRow = (row) ->
    expanded = []
    for item, index in row
      if item is '-'
        start = expanded.pop() # previous number
        end   = row[index+1]   # next number
        range = [start...end]  # range excluding end
        expanded.push range...
      else
        expanded.push item
    expanded

  # expand the layout (returns a new array)
  expandRow row for row in layout

(You could also use reduce, but the syntax is kinda messy when you want to use an initial value for the accumulator.) I've left the spacer-items alone, though.

The list can be parsed pretty much how you're doing already, just collecting the data in an object.

In all, I get something like this

TABLE_LAYOUT = [
  [1, "s:16", 2],
  [3, 4, "s:10", 5, "-", 10],
  [11, 12, "s:10", 13, "-", 18],
  [19, "-", 36],
  [37, "-", 54],
  [55, 56, "*", 72, "-", 86],
  [87, 88, "**", 104, "-", 118],
  ["s:18"],
  ["s:2", "*", 57, "-", 71],
  ["s:2", "**", 89, "-", 103]
]

# expand ranges in the layout array
expandLayout = (layout) ->
  expandRow = (row) ->
    expanded = []
    for item, index in row
      if item is '-'
        start = expanded.pop()
        end   = row[index+1]
        range = [start...end]
        expanded.push range...
      else
        expanded.push item
    expanded

  expandRow row for row in layout

# extract data from a list of elements
parseList = (list) ->
  elements = {}
  list.children('li').each ->
    item = $ this
    number = item.data 'number' # seems a better name than "atomicNumber"
    return unless number? # skip if the item isn't actually an element
    elements[number] =
      symbol: item.data 'symbol'
      type: item.data 'type' # again, simpler name
      name: item.text()
      url: item.first('a').attr 'href'
  elements

# build the periodic table
buildTable = (elements, layout) ->
  typeToClassName = (type) ->
    type.toLowerCase().replace /\s+/g, '-'

  buildCell = (value) ->
    cell = $ '<td></td>'

    if match = String(value).match /^s:(\d+)$/ # spacer cell
      cell.attr 'colspan', match[1]

    else if element = elements[value] # element cell
      link = $('<a></a>').attr 'href', element.url
      link.append $('<span></span>').text value
      link.append $('<span></span>').text element.name
      link.append $('<span></span>').text element.symbol
      cell.addClass(typeToClassName element.type).append link

    else # text cell
      cell.text value

    cell

  buildRow = (row) ->
    tr = $ '<tr></tr>'
    tr.append buildCell(item) for item in row
    tr

  table = $ '<table></table>'
  table.append buildRow(row) for row in layout

  table


# on load, replace the list
$ ->
  list = $ '#elements'
  layout = expandLayout TABLE_LAYOUT
  elements = parseList list
  list.replaceWith buildTable(elements, layout)

Here's a jsfiddle (since stack snippets don't support CoffeeScript)

Again, you can forego jQuery in favor of using the raw DOM API, but it's just a lot of extra work, I think.

It's not great, but it's nicer, I think.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your input. I know using a p tag inside of an a tag isn't generally a good thing since they're block and inline elements respectively. I do however know it's okay when the a tag is a block element itself courtesy of display: block;. When I have more time I'll read your answer again and see if I can implement some of the suggested changes. :) \$\endgroup\$ – MisterBla Dec 29 '14 at 18:12
  • 1
    \$\begingroup\$ @RichardA Using display: block only really changes the presentation of the element, not its inherent meaning, so from a pure markup perspective, it'll still be kinda backwards. Of course you can do nest elements as you please, but the markup should (in my opinion) be valid in and of itself; styling is a separate matter. There's also the question of semantics; p denotes a paragraph of text, but is that really what we've got here? I'd say no: It's just little labels, i.e. spans. \$\endgroup\$ – Flambino Dec 29 '14 at 18:31
2
\$\begingroup\$

IMHO, you're making your work slightly harder for yourself with your encodings for ranges and spacers. How about using {s: 10} for a spacer, and [a, b] for a range, e.g.

[11, 12, {s:10}, [13, 18]]

thus making each entry just one single element in the array, and removing the need to parse strings or traverse multiple entries (to handle the range case)

\$\endgroup\$
  • \$\begingroup\$ That's a good one, I'll try that and I'll update my answer if it worked. \$\endgroup\$ – MisterBla Dec 29 '14 at 18:07

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.