Add possibility to check only public coverage #10

stieglma · 2018-01-18T13:00:46Z

No description provided.

manoelcampos

I'd like to perform some changes to the code and request other ones.
To make it easier, could you please give me temporary writing permission to your fork?

You can access this link and add me as a collaborator.

stieglma · 2018-01-25T21:16:51Z

what do you want to change? The second pull request I issued relies already on some parts of these changes here. (This is also the version I currently use, I checked the numbers and they all seem to be correct there)

stieglma · 2018-01-25T21:18:25Z

if it's not too much I'd prefer when you put comments to the locations you want to have changed (and also the way you want to have them change) I'll do it until sometime saturday then

manoelcampos · 2018-01-26T12:47:27Z

I haven't reviewed all the changes yet. Up to now, I performed some small changes I describe below, which are related to format and documentation only:

Small documentation fixes and reorganization to conform to the general format used in the project.
For instance, the documentation for a param should be placed after its name, not in the next line.
Optimized imports: when there are more than 3 imports from the same package, an import package.* is used to avoid a long list of imports that just makes the code less clean. This approach has no impact on compiled class size, since Java compiler includes just the used classes into the bytecode. This way, the use of * is being preferred and it's automatically performed by IntelliJ.
Reformated some long lambda expressions to break line after each method call. This is a good practice to make lambdas clearer.

There is a major issue which will require several structural changes. I wonder if you could fix it as I describe below (after merging the changes I've already done).

You included the computeOnlyForPublicModifier attribute into the CoverageDoclet class. This attribute is being passed around several classes. Even where there is access to a CoverageDoclet object, such as inside the AbstractDataExporter class, the attribute is passed as a parameter to other classes. That is the case at line 61 of the AbstractDataExporter. The JavaDocsStats requires a RootDoc object which is got from the doclet. In the same way, the value for the computeOnlyForPublic parameter is being got from doclet. That suggests you could just pass the doclet as a single parameter and get what you want from it inside the JavaDocsStats. The same applies for all DocStats classes.
Multiple methods from classes such as ClassDocStats received a new computeOnlyForPublic. When you pass the same parameter around the methods of a class, that suggests you should create an attribute and remove such parameter from the methods. For such classes, applying the approach proposed above fix this issue.

There may be other small issues. Tell me the way you prefer to approach our changes.

stieglma · 2018-01-27T11:12:45Z

I added some rework of the docmentation and formatting stuff. As I work with eclipse it may not comply 100% to the settings you have in IntelliJ. As this is a common problem I propose to use the google-java-formatter instead. It has quite reasonable formatting defaults and will produce the same formatting for all IDEs.

To the second point regarding passing the CoverageDoclet as a parameter around: This is something I wouldn't do as it has many more methods that should not ever be called inside all the javadoc computation classes.
I was writing it this way, in order not to change the whole code at this point. But the way to go would be to create a separate configuration container class (lets call it Configuration ;) ) which contains the rootDoc and the options which are set or not. This way it would also become much easier to add new options (similar to passing the doclet, but this has just some other purpose and shouldn't be used for that case)

manoelcampos · 2018-02-04T23:01:59Z

Sorry for the late reply. I think that creating a Configuration class is an excellent option. I appreciate if you could follow this path. However, since the Doclet is already being passed around, a new Configuration configuration attribute could be defined inside the Doclet class. This way, the configuration could be got directly from the Doclet, not requiring to add an extra parameter to existing methods.

I'm going to include some comments on the code you pushed.

manoelcampos · 2018-02-04T23:14:22Z

src/main/java/com/manoelcampos/javadoc/coverage/CoverageDoclet.java

    /**
     * Checks if a given parameter is a valid custom parameter accepted by this doclet.
     * @param paramName the name of the parameter to check
     * @return true if it's a valid custom parameter, false otherwise
     */
    private static boolean isCustomParameter(final String paramName) {
-        return isParameter(paramName, OUTPUT_NAME_OPTION);
+        return isParameter(paramName, OUTPUT_NAME_OPTION) || isParameter(paramName, COVERAGE_ONLY_FOR_PUBLIC_OPTION);


I think that it should be created a new class to define a command line parameter. The class would include the short and long param names and a description. Then, a list of custom parameters could be defined, so that it can be iterated here to avoid changing the method every time a new parameter is included.

that was part of the configuration rework

manoelcampos · 2018-02-04T23:16:28Z

src/main/java/com/manoelcampos/javadoc/coverage/CoverageDoclet.java

            }
        }

-        return new String[]{};
+        return null;


To avoid NulPointerException, I prefer returning an empty String instead of null. We can use String.isEmpty to check if there is a value for a given parameter.

isn't a problem with the new configuration handling any more

manoelcampos · 2018-02-04T23:20:09Z

src/main/java/com/manoelcampos/javadoc/coverage/stats/ClassMembersDocStats.java

+                .count();
+    }
+
+    private Predicate<? super Doc> filterPublicIfNecessary() {


You don't need to make the method return a Predicate. That just makes the method body more complex by requiring it to return a lambda expression. You can just change the method signature to the same signature of the test method into the Predicate functional interface: private boolean filterPublicIfNecessary(Doc m).

Then, where you have filterPublicIfNecessary() you just change to a method reference this::filterPublicIfNecessary

stieglma · 2018-02-10T13:48:30Z

Configuration handling rework is now done. Took a while, but I didn't have much time the last weeks

stieglma · 2018-02-10T13:54:36Z

I also added lombok as a dependency. I greatly reduces boilerplate code that would have to be written otherwise. If you don't like to have this dependency we should at least think about using guava for less verbose precondition checks.

manoelcampos · 2018-02-15T23:42:30Z

pom.xml

+        <dependency>
+            <groupId>org.projectlombok</groupId>
+            <artifactId>lombok</artifactId>
+            <version>1.16.20</version>


I really don't like lombok because it uses some JDK internals which may be removed. It also requires IDE plugins for some features. At this time, I prefer to keep dependencies as minimal as possible.

manoelcampos · 2018-02-15T23:55:15Z

src/main/java/com/manoelcampos/javadoc/coverage/CoverageDoclet.java

@@ -75,51 +62,12 @@ public static boolean start(final RootDoc rootDoc) {
     */
    public CoverageDoclet(final RootDoc rootDoc) {
        this.rootDoc = rootDoc;
-        this.exporter = new HtmlExporter(this);
-    }
+        Configuration config = new Configuration(rootDoc.options());


The idea in my previous message was to create config as an attribute. This way, you don't need to pass an additional parameter for some methods which already receive the doclet.
I think the code will be clearer if you pass the RootDoc instead of an matrix of String for the Configuration constructor. Inside the constructor you can get the options from the rootDoc.
It's not obvious where this [][] String has to be got from. On the other hand, it's straighforward to understand where to get a rootDoc object from. This way, we move the complexity into the Configuration class.

manoelcampos · 2018-02-16T00:03:28Z

src/main/java/com/manoelcampos/javadoc/coverage/CoverageDoclet.java

-    private static boolean isCustomParameter(final String paramName) {
-        return isParameter(paramName, OUTPUT_NAME_OPTION);
-    }
+        JavaDocsStats stats = new JavaDocsStats(rootDoc, config);


Changing the Configuration constructor as suggested above makes unnecessary to add one more parameter to the JavaDocsStats constructor. You can pass only the config parameter and get the rootDoc from it.
For a cleaner and more functional code, the fewer parameters the better.

The stats object was being created inside the AbstractDataExporter class and you moved it out here. The object is then passed to the HtmlExporter constructor below, also increasing its number of parameters. This moves the complexity out of the AbstractDataExporter classes. Encapsulating the stats inside the AbstractDataExporter avoids duplicating its instantiation for every exporter class and also hides the creation of an object used just internally by such exporter classes.

manoelcampos · 2018-02-16T00:16:43Z

src/main/java/com/manoelcampos/javadoc/coverage/CoverageDoclet.java

-
-        return Standard.validOptions(options, errorReporter);
+        // this needs to be the last part as it already accesses some stuff from the doclet
+        this.exporter = new HtmlExporter(config, stats);


Check out the comment above.

manoelcampos · 2018-02-16T00:20:34Z

src/main/java/com/manoelcampos/javadoc/coverage/CoverageDoclet.java

    }

    /**
-     * Gets the values associated to a given command line option.
+     * Checks that all given option are valid


"Checks that all given options are valid"

manoelcampos · 2018-02-16T00:33:35Z

src/main/java/com/manoelcampos/javadoc/coverage/configuration/StringOption.java

+    private final Optional<String> defaultValue;
+
+    public StringOption(@NonNull String shortName, @NonNull String longName, @NonNull String description,
+            @NonNull Optional<String> defaultValue) {


It's expected to make methods to return an Optional, but it's a bad practice to declare parameters and fields as Optional. Since the parameter list is too long, I suggest you remove the defaultValue param and use a simple Builder pattern by adding a setter to the defaultValue as below:

StringOption setDefaultValue(String defaultValue){ this. defaultValue = defaultValue; return this; }

This way, if you need to set the default value, you can make a chained call like
StringOption option = new StringOption("-op", "-onlyPublic", "Only public members").setDefaultValue("true");

I think it's even worse to have mutable options, either we need to create complete builder pattern for it, or leave it in the constructor. For working around the optional in the constructor we could create two constructors, one without default value parameter, and one with, (where the default value has to be nonnull)

To avoid to much work for you, an overloaded constructor is good and simpler.

manoelcampos · 2018-02-16T01:05:37Z

src/main/java/com/manoelcampos/javadoc/coverage/configuration/Configuration.java

+            return opt.get().getLength();
+        }
+        return Standard.optionLength(option);
+    }


Using isPresent is a bad practice also. You could replace the lines starting at the if by:

return opt.map(Option::getLength).orElseGet(() -> Standard.optionLength(option))

manoelcampos · 2018-02-16T01:14:44Z

src/main/java/com/manoelcampos/javadoc/coverage/configuration/StringOption.java

@@ -0,0 +1,34 @@
+package com.manoelcampos.javadoc.coverage.configuration;


I don't think this StringOption class is needed. Since the Option class is generic, you could make it concrete and instantiate it. The number of values an option requires can be defined by the length attribute. This way, there is no need to add an abstract method parseValue. The Option class can implement this method by checking if the number of given parameters is equal to length.

Creating a sub-class for each type of option eliminates all the advantages of using a single generic class.

actually it isn't necessary currently, but it will be necessary as soon as we have e.g. NumberOptions that need to parse the value in some or another way.

The parsing process is currently just converting the String value to a specific type. Since there isn't an easy way to convert a String to a generic type (without recurring to reflection), I think the best approach would be to return the value as String and leave the conversion process for the caller. This is the approach used by Apache Commons CLI. The number of required values can be checked automatically by using the length attribute. Despite you consider that the option can have more than one value, your implementations (such as the StringOption) always return just one value.

The implementation below doesn't care about value conversion and automatically check if the required number of values was passed. It also has a method to get just the first value as well as an array of values (removing the element 0 which represents the option name). It handles default value too.

public class Option{ private int length; private String defaultValue; public String getFirstValue(final String[] values){ return getValues(values)[0]; } /** * Gets the values of the option, excluding the option name (the 0th element). * @return an array containing the values (without the 0th element), if the values' length isn't as expected; * or an array containing the default value if the length isn't as expected and there is a default value. * @throws IllegalStateException when the length of values isn't as expected and there isn't a default value */ public String[] getValues(final String[] values){ if(defaultValue == null && values.length != length){ throw new IllegalStateException("Wrong option line passed, string options do only have 2 values"); } return values.length == length ? Arrays.copyOfRange(values, 1, values.length) : new String[]{defaultValue}; } /* Trying the implementation. */ public static void main(String[] args) { System.out.println(); Option o = new Option(); o.length = 3; String[] values = new String[]{"-o", "1", "2"}; System.out.println("First Value: " + o.getFirstValue(values)); System.out.println("Values: " + Arrays.toString(o.getValues(values))); try{ values = new String[]{"-o", "1"}; //Error because the length of values isn't equal to 3 System.out.println("Value: " + o.getFirstValue(values)); } catch(Exception e){ System.out.println("\nError: " + e.getMessage() + "\n"); } o.length = 2; o.defaultValue = "default value"; values = new String[]{"-o"}; System.out.println("First Value: " + o.getFirstValue(values)); System.out.println("Values: " + Arrays.toString(o.getValues(values))); } }

I've just updated the implementation.

I think the value parsing is one of the most important features. Why would we allow the caller to parse the values as he likes if it can be done already in the Option itself? I added an option for minimal coverage targets as an example. Sure we could just return all the string values to the caller of getValues but then the caller has to ensure every condition again and again everytime he needs it. By doing all this stuff in the option we don't have these problem and also provide a cleaner interface.

The parsing is being performed by the implementation I provided, based on your code, making sure to:

check if the number of values is as required;

use the default value if a value is not present;

remove the parameter name from the list of values.

This is performed both for the getValue and the getValues methods. If you provide just the getValue, there is no way to get the additional values when the required number of values is greater than 1. Therefore, it doesn't make sense to consider that an option can have multiple values if the class allows access only to the first value.

The caller won't parse the values. This is done by the provided methods, which ensure the conditions enumerated above are met. They just aren't converting the value to a specific type. Since the conversion from String to a specific type is a one-liner for the caller (such as Integer.valueOf(String value)), I don't see any problem with it.

Creating a subclass simply to convert values from String is overkill. If you really want to convert the values inside the class, I see two alternatives: using reflection or functional programming.

A generic Option class using reflection is available here. The code is ugly, has to deal with lots of exceptions and there is the reflection overhead yet. However, it works for any class representing primitive values and will fail for any other type of object.

A generic, functional Option class is available here. It enables the caller to provide a Function that converts the values from String to the generic type and can work for any type you want. The only downside is that the caller is required to provide a conversion Function.

And if you want to provide default implementations for converting String to classes representing primitive types, there is this example here. It has an ugly if chain, which doesn't follow the Open/Closed Principle, but works for any primitive type.

manoelcampos · 2018-02-16T01:22:31Z

src/main/java/com/manoelcampos/javadoc/coverage/stats/ClassDocStats.java

@@ -46,41 +45,46 @@
    private List<MethodDocStats> methodsStats;
    private List<MethodDocStats> constructorsStats;

-    public ClassDocStats(final ClassDoc doc) {
+    public ClassDocStats(final ClassDoc doc, Configuration config) {
        this.doc = doc;


The config parameter should be stored in a config attribute. This way, you avoid passing the config around several methods of the class.

manoelcampos · 2018-02-16T01:24:35Z

src/main/java/com/manoelcampos/javadoc/coverage/stats/ClassMembersDocStats.java

+    }
+
+    private boolean filterPublicIfNecessary(Doc m) {
+        boolean computeOnlyForPublic = config.computePublicCoverageOnly();


declare as final

manoelcampos · 2018-02-16T01:30:16Z

src/main/java/com/manoelcampos/javadoc/coverage/configuration/Option.java

+@ToString
+@Getter
+@AllArgsConstructor
+abstract class Option<T> {


I was thinking that it would be better to use Apache Commons Cli to parse command line arguments. But nevermind if it's to much work for you.

I looked into it briefly before writing my own option class. What we need are generic default values, and I didn't see this capability when looking at the usage examples there

Thomas Stieglmaier added 2 commits January 18, 2018 08:36

first draft for public only changes

c117572

remove unused enum

6393b85

manoelcampos reviewed Jan 25, 2018

View reviewed changes

fixing some formatting issues

7ebf9bd

manoelcampos requested changes Feb 4, 2018

View reviewed changes

add configuration container

9556f6a

make variable local

0d0c7d2

manoelcampos requested changes Feb 16, 2018

View reviewed changes

manoelcampos reviewed Feb 16, 2018

View reviewed changes

adding an option for minimal coverage limits

68fea80

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add possibility to check only public coverage #10

Add possibility to check only public coverage #10

stieglma commented Jan 18, 2018

manoelcampos left a comment

stieglma commented Jan 25, 2018

stieglma commented Jan 25, 2018

manoelcampos commented Jan 26, 2018 •

edited

Loading

stieglma commented Jan 27, 2018

manoelcampos commented Feb 4, 2018

manoelcampos Feb 4, 2018

stieglma Feb 10, 2018

manoelcampos Feb 4, 2018

stieglma Feb 10, 2018

manoelcampos Feb 4, 2018

stieglma Feb 10, 2018

stieglma commented Feb 10, 2018

stieglma commented Feb 10, 2018

manoelcampos Feb 15, 2018 •

edited

Loading

manoelcampos Feb 15, 2018

manoelcampos Feb 16, 2018

manoelcampos Feb 16, 2018

manoelcampos Feb 16, 2018

manoelcampos Feb 16, 2018

stieglma Feb 16, 2018

manoelcampos Feb 16, 2018

manoelcampos Feb 16, 2018

manoelcampos Feb 16, 2018

stieglma Feb 16, 2018

manoelcampos Feb 16, 2018 •

edited

Loading

manoelcampos Feb 16, 2018

stieglma Feb 17, 2018

manoelcampos Feb 17, 2018 •

edited

Loading

manoelcampos Feb 16, 2018

manoelcampos Feb 16, 2018

manoelcampos Feb 16, 2018

stieglma Feb 16, 2018

		@@ -0,0 +1,34 @@
		package com.manoelcampos.javadoc.coverage.configuration;

Add possibility to check only public coverage #10

Are you sure you want to change the base?

Add possibility to check only public coverage #10

Conversation

stieglma commented Jan 18, 2018

manoelcampos left a comment

Choose a reason for hiding this comment

stieglma commented Jan 25, 2018

stieglma commented Jan 25, 2018

manoelcampos commented Jan 26, 2018 • edited Loading

stieglma commented Jan 27, 2018

manoelcampos commented Feb 4, 2018

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

stieglma commented Feb 10, 2018

stieglma commented Feb 10, 2018

manoelcampos Feb 15, 2018 • edited Loading

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

manoelcampos Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manoelcampos Feb 17, 2018 • edited Loading

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

manoelcampos commented Jan 26, 2018 •

edited

Loading

manoelcampos Feb 15, 2018 •

edited

Loading

manoelcampos Feb 16, 2018 •

edited

Loading

manoelcampos Feb 17, 2018 •

edited

Loading