0
\$\begingroup\$

I'm fairly new to JavaScript. I would like some feedback on the naming and grouping of the following code. Is there any change that will make it more readable?

Template.documentPage.events({
  'click .hint-text': function(e) {
    e.preventDefault()

    var currentHintId = $(e.target).attr('id')
    Session.set('currentHintId', currentHintId)

    $('.hint-popup').show()
  },

  'click .hint-submit': function(e) {
    e.preventDefault()

    var currentHintId = Session.get('currentHintId')
    var popupInput = $('.hint-popup input').val()

    $('#' + currentHintId).text(popupInput)
    Documents.update(this._id, {$set: {content: $('.content').html()}}, function() {
      console.log('Saved.')
    })

    $('.hint-popup').hide()
    $('.hint-popup input').val('Enter text')
  }
})
\$\endgroup\$
2
  • \$\begingroup\$ Please post the full source code for the object. While this may be working code, it is incomplete. \$\endgroup\$ Commented Jan 29, 2015 at 17:30
  • \$\begingroup\$ @Greg Burghardt OK, I posted the whole thing. They are Meteor events: meteor.com \$\endgroup\$ Commented Jan 30, 2015 at 1:13

1 Answer 1

Reset to default
2
\$\begingroup\$

To start off, I can't see a single semicolon being used here. You should always close your statements with a semicolon.

Also I can't quite understand what you've done here:

'click .hint-text': function(e) {
    e.preventDefault()

    var currentHintId = $(e.target).attr('id')
    Session.set('currentHintId', currentHintId)

    $('.hint-popup').show()
},

Is this a property in an object? If so why have you named it 'click .hint-text'? Give it a proper name such as togglePopup or something along those lines to make it more obvious what it does. Now it's just confusing.

And for the other function you could name it submit(whatever you're submitting).

If you included some additional markup it would help your question a bit.

\$\endgroup\$
5
  • \$\begingroup\$ The object is used in a method in some framework, where the property names are used to define what events should be bound. It's a bit strange way to write settings, but when you pick a framework you also get to use its odd design choises. \$\endgroup\$ Commented Jan 29, 2015 at 22:15
  • \$\begingroup\$ @Guffa Then the framework should really be a tag so we can understand what is going on. \$\endgroup\$ Commented Jan 29, 2015 at 22:17
  • \$\begingroup\$ @Chrillewoodz I read this post about omitting semicolons by the author of Bootstrap: wordsbyf.at/2011/10/31/i-dont-write-javascript I wanted to find out whether it was true, whether something can go wrong if I didn't use semicolons. So far nothing had happened. My app works just fine. \$\endgroup\$ Commented Jan 30, 2015 at 1:10
  • \$\begingroup\$ @Chrillewoodz I included the framework and updated the code. \$\endgroup\$ Commented Jan 30, 2015 at 1:14
  • 2
    \$\begingroup\$ @janoChen: The danger in removing semi colons comes from logic errors introduced as a part of JavaScript minification. When semi colons are omitted, the newline character becomes the statement delimiter. When you remove newlines, you could accidentally change the logic. \$\endgroup\$ Commented Jan 30, 2015 at 1:56

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.