Permalink
Browse files

Various fixes and improvements

- fixed missing keyboard shortcut for sublime on windows
- added grade to sublime plugin report
- added reviewer for comments with if/or/and/but
- moved test scripts in tests directory
  • Loading branch information...
1 parent 7df6923 commit f114587a4b27a6ea837e3281e8c4b86c7e049a6b Patrick Brosset committed Jan 24, 2012
Showing with 730 additions and 536 deletions.
  1. +6 −0 Default (Windows).sublime-keymap
  2. +4 −2 TODO.md
  3. +1 −0 package-metadata.json
  4. +27 −17 reporters/console.py
  5. +77 −17 reviewers/comments.py
  6. +1 −1 reviewers/complexity.py
  7. +20 −0 reviewers/helpers/general.py
  8. +3 −5 reviewers/helpers/similartocode.py
  9. +2 −2 tests/runtests.py
  10. +15 −0 tests/scripts/CommentedClass.js
  11. 0 {testscripts → tests/scripts}/ImageResizer.js
  12. 0 {testscripts → tests/scripts}/SmartInputDetector.js
  13. 0 {testscripts → tests/scripts}/goodfile.js
  14. +82 −0 tests/scripts/reports/CommentedClass.js-report.html
  15. +82 −0 tests/scripts/reports/ImageResizer.js-report.html
  16. +82 −0 tests/scripts/reports/SmartInputDetector.js-report.html
  17. +82 −0 tests/scripts/reports/goodfile.js-report.html
  18. +82 −0 tests/scripts/reports/simple.js-report.html
  19. +82 −0 tests/scripts/reports/sproutcore-ajax.js-report.html
  20. 0 {testscripts → tests/scripts}/reports/syntaxerror-report.html
  21. +82 −0 tests/scripts/reports/syntaxerror.js-report.html
  22. 0 {testscripts → tests/scripts}/simple.js
  23. 0 {testscripts → tests/scripts}/sproutcore-ajax.js
  24. 0 {testscripts → tests/scripts}/syntaxerror.js
  25. +0 −82 testscripts/reports/ImageResizer.js-report.html
  26. +0 −82 testscripts/reports/SmartInputDetector.js-report.html
  27. +0 −82 testscripts/reports/goodfile.js-report.html
  28. +0 −82 testscripts/reports/simple.js-report.html
  29. +0 −82 testscripts/reports/sproutcore-ajax.js-report.html
  30. +0 −82 testscripts/reports/syntaxerror.js-report.html
@@ -0,0 +1,6 @@
+[
+ {
+ "keys": ["ctrl+shift+c"],
+ "command": "cleanjs"
+ }
+]
View
@@ -8,15 +8,16 @@ Missing reviewers:
- Check jsdoc format and accepted @ statements
- Check that jsdoc block have description
- Check that file has first a license, then a jsdoc block that is at least some nb of lines with example. Display info if there is no `pre` tag cause it means there's no example
- - Verify if a jsdoc block was just copied from another function/class ??
- unused
- Unused fields this. In class
- code duplication
- comparing branches of the AST ?
-- jslint
+- syntax
+ - jslint
- new Array() and new Object()
- check for global vars assignments (missing var)
- check ; is present after assignements
+ - Check that when a local method is called, it is called with the right number of arguments
- complexity
- inline function is often sign of difficult to read code
- comments:
@@ -46,3 +47,4 @@ Other misc stuff to do:
- https://gist.github.com/1607354 is a good example
- http://ominian.com/2012/01/06/working-with-using-pynarcissus-to-parse-javascript-in-python/
- parser originally is pynarcissus
+- Find a way to have errors/warnings/infos into the editor directly, rather than in the console
View
@@ -0,0 +1 @@
+{"url": "https://github.com/captainbrosset/cleanjs", "description": "Javascript clean code checker tool that will hurt your feelings!"}
View
@@ -25,30 +25,40 @@ def output_messages(result, file_data):
report += _get_report_post_border(max_content_length)
report += "\n"
+ # Output the rating
+ report += _output_one_message("Rating | ", max_header_length, result["rating"], max_content_length)
+
# Now output messages
for index, message in enumerate(messages):
- pre_padding_length = 0
- if len(headers[index]) < max_header_length:
- pre_padding_length = max_header_length - len(headers[index])
-
- post_padding_length = 0
- if len(contents[index]) < max_content_length:
- post_padding_length = max_content_length - len(contents[index])
-
- report += "|"
- report += "".join([" " for i in range(pre_padding_length)])
- report += headers[index]
- report += contents[index]
- report += "".join([" " for i in range(post_padding_length)])
- report += "|\n"
- report += _get_report_pre_border(max_header_length)
- report += _get_report_post_border(max_content_length)
- report += "\n"
+ report += _output_one_message(headers[index], max_header_length, contents[index], max_content_length)
report += "\n"
return report
+def _output_one_message(header, max_header_length, content, max_content_length):
+ report = ""
+
+ pre_padding_length = 0
+ if len(header) < max_header_length:
+ pre_padding_length = max_header_length - len(header)
+
+ post_padding_length = 0
+ if len(content) < max_content_length:
+ post_padding_length = max_content_length - len(content)
+
+ report += "|"
+ report += "".join([" " for i in range(pre_padding_length)])
+ report += header
+ report += content
+ report += "".join([" " for i in range(post_padding_length)])
+ report += "|\n"
+ report += _get_report_pre_border(max_header_length)
+ report += _get_report_post_border(max_content_length)
+ report += "\n"
+
+ return report
+
def _get_message_report_header(message):
header = ""
header += message.reviewer + " " + message.type
View
@@ -1,6 +1,7 @@
import re
from helpers import similartocode
+from helpers import general
class Reviewer():
MAX_CODE_COMMENT_RATIO_IN_FUNCTION = 0.3
@@ -9,6 +10,9 @@ class Reviewer():
SEPARATOR_CHARACTERS = ["-", "\|", "!", "#", "\.", "\*", "=", "/", "~", "+", "\\"]
MAX_NUMBER_OF_SEPARATOR_CHARACTERS_IN_COMMENTS = 3
+ WARN_MAX_NB_OF_BUTIFORAND_CONDITION = 0
+ ERROR_MAX_NB_OF_BUTIFORAND_CONDITION = 2
+
def get_name(self):
return "comments"
@@ -79,12 +83,38 @@ def review_comment_code_similarity(self, lines, message_bag):
if similartocode.is_code_and_comment_similar(code, comment):
message_bag.add_warning(self, "It seems this comment is very similar to the code directly beneath it. Don't you think you can get rid of it?", line.line_number)
+ def get_all_comment_blocks(self, lines):
+ blocks = []
+ for line in lines:
+ if not line.is_only_comments():
+ # No comments, start a new block
+ blocks.append({"line_nb": None, "comment": ""})
+ else:
+ # Comments, put that into the last comment block opened
+ if len(blocks) == 0:
+ blocks.append({"line_nb": None, "comment": ""})
+ blocks[len(blocks) - 1]["comment"] += line.comments
+ if not blocks[len(blocks) - 1]["line_nb"]:
+ blocks[len(blocks) - 1]["line_nb"] = line.line_number
+ return general.filter_empty_items_from_dict_list(blocks, "comment")
+
+ def review_multiple_responsibilities(self, lines, message_bag):
+ # 'if', 'but', 'and','or', then it's likely that the commented code has more than one responsibility
+ comment_blocks = self.get_all_comment_blocks(lines)
+ for comment in comment_blocks:
+ nb_of_conditions = len(re.findall("and|or|but|if", comment["comment"]))
+ if nb_of_conditions > Reviewer.WARN_MAX_NB_OF_BUTIFORAND_CONDITION:
+ message_bag.add_warning(self, "It seems this comment tries to explain a piece of code that has several responsibilities", comment["line_nb"])
+ elif nb_of_conditions > Reviewer.ERROR_MAX_NB_OF_BUTIFORAND_CONDITION:
+ message_bag.add_error(self, "For sure, this comment corresponds to code that has more than 1 responsibility", comment["line_nb"])
+
def review(self, file_data, message_bag):
self.review_multiple_comment_lines(file_data.lines.all_lines, message_bag)
self.review_comments_ratio_in_functions(file_data.functions, message_bag)
self.review_comments_after_statements(file_data.lines.all_lines, message_bag)
self.review_separator_comments(file_data.lines.all_lines, message_bag)
self.review_comment_code_similarity(file_data.lines.all_lines, message_bag)
+ self.review_multiple_responsibilities(file_data.lines.all_lines, message_bag)
if __name__ == "__main__":
@@ -97,24 +127,54 @@ def add_warning(self, reviewer, message, line_number):
self.warnings.append(line_number)
class MockLine:
- def __init__(self, comments):
+ def __init__(self, comments, is_only_comments=True):
self.comments = comments
self.line_number = 0
-
- reviewer = Reviewer()
- bag = MockBag()
- line1 = MockLine("this is something ..")
- line2 = MockLine("this is something ----")
- line3 = MockLine("this is something --#-~-")
- line4 = MockLine("// \\\\ ...")
- lines = []
- lines.append(line1)
- lines.append(line2)
- lines.append(line3)
- lines.append(line4)
-
- reviewer.review_separator_comments(lines, bag)
-
- assert len(bag.warnings) == 2, "Wrong number of separator comments found"
+ self._is_only_comments = is_only_comments
+ def is_only_comments(self):
+ return self._is_only_comments
+ def test_separator_comments():
+ reviewer = Reviewer()
+ bag = MockBag()
+
+ line1 = MockLine("this is something ..")
+ line2 = MockLine("this is something ----")
+ line3 = MockLine("this is something --#-~-")
+ line4 = MockLine("// \\\\ ...")
+ lines = []
+ lines.append(line1)
+ lines.append(line2)
+ lines.append(line3)
+ lines.append(line4)
+
+ reviewer.review_separator_comments(lines, bag)
+
+ assert len(bag.warnings) == 2, "Wrong number of separator comments found"
+
+ def test_get_all_comment_blocks():
+ line1 = MockLine("/**", True)
+ line2 = MockLine(" * Class super.great.Framework", True)
+ line3 = MockLine(" * This class is the main entry point of the framework", True)
+ line4 = MockLine(" * It provides the main utilities of the framework and also a way to load other classes", True)
+ line5 = MockLine(" */", True)
+ line6 = MockLine("super.great.Framework = function() {", False)
+ line7 = MockLine(" this.test = 2; // wow, we initialize stuff", False)
+ line8 = MockLine(" /**", True)
+ line9 = MockLine(" * Name of the instance", True)
+ line10 = MockLine(" * @type {String}", True)
+ line11 = MockLine(" */", True)
+ line12 = MockLine(" this.name = \"\"", False)
+
+ lines = [line1,line2,line3,line4,line5,line6,line7,line8,line9,line10,line11,line12]
+
+ blocks = Reviewer().get_all_comment_blocks(lines)
+ assert len(blocks) == 2, "Wrong number of comment blocks found in lines"
+
+ blocks = Reviewer().get_all_comment_blocks([])
+ assert len(blocks) == 0
+
+ test_separator_comments()
+ test_get_all_comment_blocks()
+
print "ALL TESTS OK " + __file__
View
@@ -40,7 +40,7 @@ def review_ifs_complexity(self, lines, message_bag):
message_bag.add_error(self, "Found an IF statement with more than " + str(Reviewer.ERROR_MAX_NB_OF_CONDITIONS_IN_IF) + " AND or OR! Wrap them in a function like isABC()", line.line_number)
elif len(conditions) > Reviewer.WARN_MAX_NB_OF_CONDITIONS_IN_IF:
message_bag.add_warning(self, "Found an IF statement with more than " + str(Reviewer.WARN_MAX_NB_OF_CONDITIONS_IN_IF) + " AND or OR! Could you extract this in a function like isABC()?", line.line_number)
-
+
def review(self, file_data, message_bag):
self.review_functions_complexity(file_data.functions, message_bag)
self.review_ifs_complexity(file_data.lines.all_lines, message_bag)
@@ -0,0 +1,20 @@
+def filter_empty_items_from_dict_list(list, object_property_to_check):
+ """Given a list of dictionaries, filter out the ones for which the
+ object_property_to_check key/value evaluates to False"""
+
+ return filter(lambda item: not not item[object_property_to_check], list)
+
+def filter_dups_from_list(the_list):
+ """Given a list of hashable items, remove the duplicates from that list"""
+
+ return list(set(the_list))
+
+if __name__ == "__main__":
+ assert filter_empty_items_from_dict_list([], "test") == []
+ assert filter_empty_items_from_dict_list([{"test": ""}, {"test": []}, {"test": "a"}], "test") == [{"test": "a"}]
+
+ assert filter_dups_from_list([]) == []
+ assert filter_dups_from_list([1,1,1,2,3,1,5]) == [1,2,3,5]
+ assert sorted(filter_dups_from_list(["a", 4, False, False, 4, "a", "b"])) == sorted(["a", 4, False, "b"])
+
+ print "ALL TESTS OK " + __file__
@@ -1,14 +1,12 @@
import re
import extractwords
-
-def get_unique_words(words):
- return list(set(words))
+import general
def is_code_and_comment_similar(code, comment):
- comment_words = get_unique_words(extractwords.get_all_words_from_line(comment))
+ comment_words = general.filter_dups_from_list(extractwords.get_all_words_from_line(comment))
comment_words_similar_to_code = []
- code_words = get_unique_words(extractwords.get_all_words_from_line(code))
+ code_words = general.filter_dups_from_list(extractwords.get_all_words_from_line(code))
code_words_similar_to_comment = []
if len(comment_words) == 0 or len(code_words) == 0:
View
@@ -11,10 +11,10 @@ def run_unit_tests(dir):
pass
def run_full_tests():
- for item in os.listdir("../testscripts"):
+ for item in os.listdir("scripts"):
if item[-3:] == ".js":
print "-- RUNNING CLEANJS ON " + item
- os.system("python ../cleanjs_cmdline.py ../testscripts/" + item + " ../testscripts/reports/" + item + "-report.html")
+ os.system("python ../cleanjs_cmdline.py scripts/" + item + " scripts/reports/" + item + "-report.html")
print ""
print "-- RUNNING ALL UNIT TETS"
@@ -0,0 +1,15 @@
+var my = {package: {}}
+
+/**
+ * @class my.package.MyClass
+ * This class is used to store the data model on the client, using local storage or session storage depending on the case.
+ * If the setting is set to LOCAL, then local storage is used, otherwise session storage is used
+ */
+my.package.MyClass = function(setting) {
+ this.setting = setting;
+};
+
+// Set the setting
+my.package.MyClass.prototype.setSetting = function(setting) {
+ this._setting = setting;
+};
File renamed without changes.
File renamed without changes.
@@ -0,0 +1,82 @@
+<!DOCTYPE html>
+ <html lang="en">
+ <head>
+ <meta charset="utf-8">
+ <title>cleanjs report for file scripts/CommentedClass.js</title>
+ <style type="text/css">
+ body {
+ margin: 0;
+ padding: 1em;
+ font-size: 12px;
+ font-family: verdana;
+ overflow-x: hidden;
+ width: 100%;
+ color: #444;
+ }
+ h1 {
+ margin: 0;
+ padding: 1em 1em 1em 25px;
+ }
+ .general {
+ padding: 0;
+ margin: 0 0 30px 30px;
+ list-style-type: none;
+ width: 500px;
+ color: #222;
+ }
+ .lines {
+ padding: 0;
+ margin: 0;
+ list-style-type: none;
+ width: 10000px;
+ }
+ .lines .line {
+ overflow: hidden;
+ padding: 0;
+ margin: 0;
+ list-style-type: none;
+ }
+ .lines .line .gutter {
+ float: left;
+ text-align: right;
+ width: 25px;
+ padding: 4px 5px 4px 0;
+ color: #aaa;
+ font-size: 11px;
+ }
+ .lines .nomessage .gutter {
+ color: #ddd;
+ }
+ .lines .line .messages {
+ float: left;
+ padding: 0;
+ margin: 0;
+ list-style-type: none;
+ width: 500px;
+ }
+ .lines .line .messages li, .general li {
+ padding: 4px 1em;
+ line-height: 13px;
+ }
+ .lines .line .code {
+ float :left;
+ margin: 0;
+ padding: 0 1em;
+ font-family: Menlo, Monaco, Consolas, "Lucida Console", monospace;
+ font-size: 11px;
+ }
+ .error, .info, .warning {
+ border-left: 4px solid #3e93bf;
+ background: #eee;
+ }
+ .warning {
+ border-color: #f5931f;
+ }
+ .error {
+ font-weight: bold;
+ border-color: #d83e0f;
+ }
+ </style>
+ </head>
+ <body>
+ <h1>scripts/CommentedClass.js | grade B</h1><ul class='general'><li class='info'>File is 15 lines long</li><li class='info'>6 lines of comments</li><li class='info'>7 lines of code</li><li class='info'>2 empty lines</li><li class='info'>There are 2 functions in the file</li><li class='info'>Longest function is 1 lines long, and shortest one is 1 (average is 1)</li></ul><ul class='lines'><li class='line nomessage'><span class='gutter'>1</span><ul class='messages'><li>&nbsp;</li></ul><pre class='code'>var my = {package: {}}</pre></li><li class='line nomessage'><span class='gutter'>2</span><ul class='messages'><li>&nbsp;</li></ul><pre class='code'></pre></li><li class='line'><span class='gutter'>3</span><ul class='messages'><li class='warning'>It seems this comment tries to explain a piece of code that has several responsibilities</li></ul><pre class='code'>/**</pre></li><li class='line nomessage'><span class='gutter'>4</span><ul class='messages'><li>&nbsp;</li></ul><pre class='code'> * @class my.package.MyClass</pre></li><li class='line'><span class='gutter'>5</span><ul class='messages'><li class='error'>Line is more than 120 character long (122). This is hard to read.</li></ul><pre class='code'> * This class is used to store the data model on the client, using local storage or session storage depending on the case.</pre></li><li class='line nomessage'><span class='gutter'>6</span><ul class='messages'><li>&nbsp;</li></ul><pre class='code'> * If the setting is set to LOCAL, then local storage is used, otherwise session storage is used</pre></li><li class='line nomessage'><span class='gutter'>7</span><ul class='messages'><li>&nbsp;</li></ul><pre class='code'> */</pre></li><li class='line nomessage'><span class='gutter'>8</span><ul class='messages'><li>&nbsp;</li></ul><pre class='code'>my.package.MyClass = function(setting) {</pre></li><li class='line nomessage'><span class='gutter'>9</span><ul class='messages'><li>&nbsp;</li></ul><pre class='code'> this.setting = setting;</pre></li><li class='line nomessage'><span class='gutter'>10</span><ul class='messages'><li>&nbsp;</li></ul><pre class='code'>};</pre></li><li class='line nomessage'><span class='gutter'>11</span><ul class='messages'><li>&nbsp;</li></ul><pre class='code'></pre></li><li class='line nomessage'><span class='gutter'>12</span><ul class='messages'><li>&nbsp;</li></ul><pre class='code'>// Set the setting</pre></li><li class='line nomessage'><span class='gutter'>13</span><ul class='messages'><li>&nbsp;</li></ul><pre class='code'>my.package.MyClass.prototype.setSetting = function(setting) {</pre></li><li class='line nomessage'><span class='gutter'>14</span><ul class='messages'><li>&nbsp;</li></ul><pre class='code'> this._setting = setting;</pre></li><li class='line nomessage'><span class='gutter'>15</span><ul class='messages'><li>&nbsp;</li></ul><pre class='code'>};</pre></li></ul></body></html>
Oops, something went wrong.

0 comments on commit f114587

Please sign in to comment.