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\$
  • \$\begingroup\$ Please post the full source code for the object. While this may be working code, it is incomplete. \$\endgroup\$ – Greg Burghardt Jan 29 '15 at 17:30
  • \$\begingroup\$ @Greg Burghardt OK, I posted the whole thing. They are Meteor events: meteor.com \$\endgroup\$ – janoChen Jan 30 '15 at 1:13
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\$
  • \$\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\$ – Guffa Jan 29 '15 at 22:15
  • \$\begingroup\$ @Guffa Then the framework should really be a tag so we can understand what is going on. \$\endgroup\$ – Chrillewoodz Jan 29 '15 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\$ – janoChen Jan 30 '15 at 1:10
  • \$\begingroup\$ @Chrillewoodz I included the framework and updated the code. \$\endgroup\$ – janoChen Jan 30 '15 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\$ – Greg Burghardt Jan 30 '15 at 1:56

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.