Skip to content

Instantly share code, notes, and snippets.

@nebrius
Last active December 15, 2015 20:59
Show Gist options
  • Select an option

  • Save nebrius/5322312 to your computer and use it in GitHub Desktop.

Select an option

Save nebrius/5322312 to your computer and use it in GitHub Desktop.

Comments on the Tizen Repository

General Comments

  • Run your code through JSHint. You have lots of mixed tabs and spaces, missing semicolons, trailing whitespace, undeclared variables, unused variables, etc. I use the following options:
{
	"curly": true,
	"noempty": true,
	"undef": true,
	"unused": true,
	"trailing": true,
	"es5": true,
	"expr": true,
	"laxcomma": true,
	"node": true,
	"strict": false
}
  • Don't use API.info
    • use console.log instead
    • API.info just wraps console.log, so you don't gain anything aside from a performance penalty by using it

File Specific Comments, Round 2

titanium/Ti/Gesture.js

  • There are still two different mechanisms for detecting orientation change.
    • Unless there is a reason that you need to use both, you should pick one and stick with it.

titanium/Ti/Contacts.js

  • Line 143: updateTizenContacts is only called once from this line. You should inline this code and get rid of the function
  • Line 101, 112: If members and supportedMembers are not supported, then remove the stub.

titanium/Ti/Contacts/Tizen.js

titanium/Ti/Contacts/Group.js

  • If members and supportedMembers are not supported, then remove the stub.
    • Never stub out methods you don't support, the proper approach is to leave them as undefined (it will still throw an exception if someone tries to call them) and make sure that the docs correctly indicate that Tizen does not support those methods.

titanium/Ti/Media.js

  • Line 74, 75, 125: Don't define functions/objects on a single line. Always have a newline after an opening bracket and before a closing bracket.

titanium/Ti/Platform/DisplayCaps.js

  • Inline the initDisplayCaps code. It does not need to be a function.

titanium/Ti/UI/EmailDialog.js

  • Line 8, 12: Hidden properties never go in constants or properties. Take these two out of constants.

titanium/Ti/UI/Label.js

  • Line 48: Why didn't you just use a regex with the /g flag? Much more performant

titanium/Ti/UI/OptionDialog.js

  • Line 25: Remove this custom view
  • Line 124: Property 'tizenView' is not part of the API. Remove it.

titanium/Ti/UI/View.js

  • Remove the toImage call. It relies on canvas under the hood which means that it is subject to the same origin policy. In practice, this code will be worthless. It's better to not have it at all than have it break most of the time

modules/tizen/src/_/WebAPIError.js

  • Don't use a getter, it's wasteful. Instead, set the constant to the arg value in the constructor.

modules/tizen/src/_/SystemSettings.js

modules/tizen/src/_/SystemInfo.js

File Specific Comments, Round 1 (deprecated)

titanium/Ti/Gesture.js

  • There are two mechanisms for detecting orientation change
    • One is on line 11 and I think is the original mobile web mechanism
    • The other is on line 73 and is Tizen specific
    • One of these needs to be removed

titanium/Ti/Contacts.js

  • Line 54: throw new Error('This function is not supported here. Use Ti.Contacts.Tizen.getPeopleWithName instead.');
    • Why is this not implemented here?
  • Line 76: updateTizenContact is only used this one time, so it should not go in the helper class
  • Line 87, 91, 152, 176: Ti should NEVER be referenced in code! It breaks dependency analysis! ALWAYS include the namespace you need as a dependency in the AMD definition. In this case, Ti/UI should be a dependency, and then you call UI.createBlah
  • Line 176: Inline the callbacks. There is no reason to assign it to a variable

titanium/Ti/Contacts/Tizen.js

  • This file is supposed to go in the Tizen module, not attached to the global namespace
  • Line 52: Do not reference Ti. Add it as a dependency

titanium/Ti/Contacts/Person.js

  • Line 22: Do not reference Ti. Add it as a dependency

titanium/Ti/Contacts/Group.js

  • Line 21: Do not reference Ti. Add it as a dependency
  • Line 31: Why is members not supported here?
  • Line 43: Why is sortedMembers not supported here?

titanium/Ti/_/Contacts/helper.js

  • Line 68, 77, 254, 255: Always place ternary oprators at the end of the line, not the beginning. It helps to avoid accidental automatic semicolon insertion bugs
  • Line 224: variable name mismatch (currentAnniversary vs currentAnniversaries)
  • Line 302: typo, result not reult
  • Line 377: organization is not defined

titanium/Ti/Media.js

  • Line 90, 116, 159: inline callbacks
  • Line 94: Do not reference Ti. Add it as a dependency

titanium/Ti/Media/AudioPlayer.js

  • Line 42: progress is not defined

titanium/Ti/Network.js

  • Line 52: Use console.error instead. Much more efficient, plus you don't have to bring in the Ti.API dependency
  • Line 76, 81: typeName is not defined

titanium/Ti/Network/HTTPClient.js

  • I rejected a pull request earlier because the changes to this file were completely wrong. I see that those wrong changes are still here. All of these changes need to be completely redone

titanium/Ti/Platform.js

  • Line 59, 70: Do not reference Ti. Add it as a dependency
  • Line 81: ternary operators go at the end of aline
  • Line 25: There is no reason for this to exist as a function, just plop the code directly where it's called

titanium/Ti/DisplayCaps.js

  • Line 22: inline this callback
  • Line 23, 53: Do not reference Ti. Add it as a dependency
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment