nitsmell : Add code smell detection #2445

Delja · 2017-05-15T15:34:35Z

Adding code smell detection :

Long class
Long method parameter list
Long size method
Feature envy

Adding a visitor to analyse the contents of the methods

privat · 2017-05-15T15:44:49Z

ok to test

lbajolet

Although there's a base that could be merged, there is still some work to do to arrive at that point, could you fix all the issues in your code and re-submit? Thanks!

lbajolet · 2017-05-24T00:39:32Z

src/metrics/codesmells_metrics.nit

+module codesmells_metrics
+
+# We usualy need specific phases
+# NOTE: `frontend` is sufficent in most case (it is often too much)


This comment feels out of place, is it a copy/paste from an example phase?

lbajolet · 2017-05-24T00:40:05Z

src/metrics/codesmells_metrics.nit

+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+module codesmells_metrics


The module header should be detached from the license, else nitdoc might consider it as part of the documentation

lbajolet · 2017-05-24T00:40:25Z

src/metrics/codesmells_metrics.nit

+class Antipatterns
+	super BadConceptions
+	# create all Antipatterns
+	init do


This init could be removed if it is empty

lbajolet · 2017-05-24T00:41:37Z

src/metrics/codesmells_metrics.nit

+	end
+
+	#Collection
+	fun collect(classe : AClassdef)do 


A common way to avoid synatx errors when using class is to use clazz instead, but if it bothers no one, classe is also OK, albeit a bit too french

For AClassdef, aclassdef and n_classdef (the latter for historical reasons) are the most common variable names used in the Nit compiler.

lbajolet · 2017-05-24T00:42:18Z

src/metrics/codesmells_metrics.nit

+
+class BadConceptions
+	#Code smell list
+	var badConceptionElement = new Array[BadConception]


Generally when declaring attributes or variables, the preferred Nit style is snake_case instead of camelCase

lbajolet · 2017-05-24T00:52:57Z

src/metrics/codesmells_metrics.nit

+	var badConceptionElement = new Array[BadConception]
+
+	# Print all element conception
+	fun printAll do


I feel the print concerns should be handled by the nitmetrics command-line client instead of the library.
If it is the case, at some point we may add a dependance on console to add some command-line eye-candy, which would make no sense in the library itself

lbajolet · 2017-05-24T00:55:01Z

src/metrics/codesmells_metrics.nit

+		var visits = callvisiteMethodAnalyse(aclasse)
+
+		for visit in visits do
+			if visit.lineDetail.length > 30 then


Hard-coded magic number is hard-coded, could you move it to an attribute should we wish to change it at some point?

lbajolet · 2017-05-24T00:56:28Z

src/metrics/method_analyse_metrics.nit

+import mclasses_metrics
+import semantize
+
+fun callvisiteMethodAnalyse(aclasse : AClassdef) : Array[MethodAnalyse] do


fun analyze_methods? Or analyse_methods would be OK too, I'm not angry with en_GB.

lbajolet · 2017-05-24T00:58:00Z

src/metrics/method_analyse_metrics.nit

+			end
+		end
+	end
+end


Same newline issue here

lbajolet · 2017-05-24T00:58:36Z

src/nitsmells.nit

+
+# The body of the specific work.
+# The main entry point is provided by `test_phase`,
+# This function is then automatically (unless errors where found).


This seems out of place too, copy/paste issue?
There's a typo here as well

Morriar

A lot of changes required. But aside from the code the real problem is the theory behind it.

In my opinion, you really need more tests on this. At least to find the edge cases (like the one shown in test_prog.

Morriar · 2017-06-01T14:00:24Z

src/metrics/codesmells_metrics.nit

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# Detect the code smells and antipatterns in the code.


If this is the documentation of the module it should be attached to the module declaration. Remove the blank line between them.

Morriar · 2017-06-01T14:01:04Z

src/metrics/codesmells_metrics.nit

+
+
+redef class ToolContext
+	var codesmells_metrics_phase: Phase = new CodeSmellsMetricsPhase(self, null)


I'm not convinced that the static type is really required here.

Morriar · 2017-06-01T14:01:54Z

src/metrics/codesmells_metrics.nit

+
+
+	redef fun process_mainmodule(mainmodule, given_mmodules)
+	do


Most of your dos are inlined with the method declaration, keep it consistent.

Morriar · 2017-06-01T14:02:31Z

src/metrics/codesmells_metrics.nit

+	redef fun process_mainmodule(mainmodule, given_mmodules)
+	do
+		print toolcontext.format_h1("--- Code Smells Metrics ---")
+		var mclazzs = mainmodule.flatten_mclass_hierarchy


Did you mean mclasses ?

Morriar · 2017-06-01T14:03:37Z

src/metrics/codesmells_metrics.nit

+		print toolcontext.format_h1("--- Code Smells Metrics ---")
+		var mclazzs = mainmodule.flatten_mclass_hierarchy
+		var model_builder = toolcontext.modelbuilder
+ 		var model_view = model_builder.model.private_view


This is a really good question. Do we take private/generated things into account when running mertrics?

Morriar · 2017-06-01T15:23:33Z

tests/sav/nitsmells_args1.res

+		-quit
+Long method:  Average 1 lines
+	Affected method:
+		-total_strengh has 2 lines


Well, the example itself shows how reliable this metric is... :/

I'm not convinced that a 2-lines method can really be viewed as large. Even if the average is 1 line.

Morriar · 2017-06-01T15:24:32Z

tests/sav/nitsmells_args1.res

+*** CODE SMELLS METRICS ***
+--- Code Smells Metrics ---
+-------------------
+Class: Character


You should display the class full name (full_name) or/and the class location.

Morriar · 2017-06-01T15:26:29Z

tests/sav/nitsmells_args1.res

+Class: Character
+Feature envy :  true
+	Affected method:
+		-quit


Feature envy: A method accesses the data of another object more than its own data.

Here the quit code:

fun quit do career = null end

I really don't understand the result.

Morriar · 2017-06-01T15:28:00Z

tests/sav/nitsmells_args1.res

+		-total_endurance has 2 lines
+		-total_intelligence has 2 lines
+-------------------
+Class: Character


If it appears twice for the same program, you are not talking about Class but Classdef

Morriar · 2017-06-01T15:32:09Z

tests/sav/nitsmells_args1.res

+Class: Character
+Feature envy :  true
+	Affected method:
+		-hit_points


Again:

redef fun hit_points do return health

Here health is introduced by Character but in another mclassdef.

Morriar · 2017-06-05T17:38:45Z

src/metrics/codesmells_metrics.nit

+	fun print_result is abstract
+end
+
+


Morriar · 2017-06-05T17:38:55Z

src/metrics/codesmells_metrics.nit

+
+	var number_method = 0
+
+	redef fun name do


Morriar · 2017-06-05T17:39:21Z

src/metrics/codesmells_metrics.nit

+
+	redef fun name do
+		return "LONGPL"
+	end


Morriar · 2017-06-05T17:39:33Z

src/metrics/codesmells_metrics.nit

+		return result
+	end
+
+


Morriar · 2017-06-05T17:39:44Z

src/metrics/codesmells_metrics.nit

+	end
+end
+
+


Morriar · 2017-06-05T17:47:40Z

src/metrics/codesmells_metrics.nit

+		number_attribut = mclassdef.collect_intro_and_redef_mattributes(model_builder.model.private_view).length
+		# get the number of methods and subtract the get and set of attibutes (numberAtribut*2)
+		number_method = mclassdef.collect_intro_and_redef_methods(model_builder.model.private_view).length - (number_attribut*2)
+		if number_method.to_f > phase.average_number_of_method and number_attribut.to_f > phase.average_number_of_attribute then


return "the big ass condition"

Morriar · 2017-06-05T17:49:24Z

src/metrics/codesmells_metrics.nit

+			bad_methods.add(meth)
+			result = true
+		end
+		return result


return bad_methods.not_empty

Morriar · 2017-06-05T17:49:46Z

src/metrics/codesmells_metrics.nit

+			result = true
+			bad_methods.add(mmethoddef)
+		end
+		return result


return bad_methods.not_empty

Morriar · 2017-06-05T17:51:18Z

src/metrics/codesmells_metrics.nit

+		return "Long method"
+	end
+
+	redef fun collect(mclassdef, model_builder): Bool do


Factorize this method

Morriar · 2017-06-05T17:51:30Z

src/metrics/codesmells_metrics.nit

+		var result = false
+		var mmethoddefs = call_analyze_methods(mclassdef,model_builder)
+		for mmethoddef in mmethoddefs do
+			if mmethoddef.line_number <= phase.average_number_of_lines.to_i then continue


Redefine this part from the factorization

Morriar · 2017-06-05T18:06:23Z

tests/sav/nitsmells_args2.res

@@ -0,0 +1,2 @@
+Error: cannot find module `../TestNitsmells/LargeClass/`.


Morriar · 2017-06-05T18:06:28Z

tests/sav/nitsmells_args3.res

@@ -0,0 +1,2 @@
+Error: cannot find module `../TestNitsmells/LongMethod/`.


Morriar · 2017-06-05T18:06:33Z

tests/sav/nitsmells_args4.res

@@ -0,0 +1,2 @@
+Error: cannot find module `../TestNitsmells/LongParameterList/`.


Morriar · 2017-06-05T18:18:13Z

src/metrics/codesmells_metrics.nit

+		return counter.avg + counter.std_dev
+	end
+
+	fun get_avg_LineNumber(model_builder: ModelBuilder): Float do


privat · 2017-06-06T19:36:38Z

tests are OK (modulo Java & race conditions).
A final(?) review is requested.

Delja · 2017-06-06T19:55:03Z

I just saw one last change. I need to replace the mclassdef.mpropdefs by mclassdef.collect ...

Long class Long method parameter list Long size method Feature envy Adding a visitor to analyse the contents of the methods Signed-off-by: Florian Deljarry <deljarry.florian@gmail.com>

Changes applied

Morriar · 2017-06-09T17:04:38Z

Good job, ok to merge!

Adding code smell detection : - Long class - Long method parameter list - Long size method - Feature envy Adding a visitor to analyse the contents of the methods Pull-Request: #2445 Reviewed-by: Jean Privat <jean@pryen.org> Reviewed-by: Lucas Bajolet <r4pass@hotmail.com> Reviewed-by: Jean-Christophe Beaupré <jcbrinfo.public@gmail.com>

privat added the need_review label May 15, 2017

privat requested a review from Morriar May 15, 2017 15:44

lbajolet previously requested changes May 24, 2017

View reviewed changes

Delja force-pushed the master branch 2 times, most recently from 5a4540f to 517a880 Compare May 31, 2017 18:38

Morriar requested changes Jun 1, 2017

View reviewed changes

Delja force-pushed the master branch from 517a880 to 1d93573 Compare June 5, 2017 17:36

Morriar requested changes Jun 5, 2017

View reviewed changes

Delja force-pushed the master branch 6 times, most recently from bd529e7 to 52b86ca Compare June 6, 2017 13:18

privat added tests_are_ok need_review and removed need_review labels Jun 6, 2017

Adding code smell detection :

3686625

Long class Long method parameter list Long size method Feature envy Adding a visitor to analyse the contents of the methods Signed-off-by: Florian Deljarry <deljarry.florian@gmail.com>

Delja force-pushed the master branch from 52b86ca to 3686625 Compare June 6, 2017 20:16

Morriar approved these changes Jun 9, 2017

View reviewed changes

privat merged commit 3686625 into nitlang:master Jun 12, 2017

privat removed the need_review label Jun 12, 2017



		redef class ToolContext
		var codesmells_metrics_phase: Phase = new CodeSmellsMetricsPhase(self, null)

		@@ -0,0 +1,2 @@
		Error: cannot find module `../TestNitsmells/LargeClass/`.

nitsmell : Add code smell detection #2445

nitsmell : Add code smell detection #2445

Conversation

Delja commented May 15, 2017

privat commented May 15, 2017

lbajolet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Morriar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

privat commented Jun 6, 2017 • edited Loading

Delja commented Jun 6, 2017

Morriar commented Jun 9, 2017

privat commented Jun 6, 2017 •

edited

Loading