Search and fix minor errors and typos

Development
1

For several days I have been reading the OpenMRS Core code and I notice a lot of minor errors, typos and non-compliance with conventions. That’s why I decided since I am browsing classes, I can correct these small mistakes.

The main task, where I grouped several groups of things to improve in the subtasks. Each of them can be assessed separately if it is worth doing or whether it should be left unchanged.

Here I search for places where casting is made without checking that the correct type variable is being cast. This can result in an exception that we do not expect and do not catch explicitly in the code.

Here I search for minor errors in javadocs, typos in the parameters, outdated links etc.

Raw use of parameterized Collection? Is this java 1.4?

Someone did the todo task but forgot to delete the todo.

Here the retVal variable is created and assigned the value true if a string is found. At the same time, the method continues to search, even though it has already found. Correcting this will give us several processor cycles. :wink:

Here you can reduce the number of lines of code by replacing iterative iteration through collections with the use of the Collection.removeIf method.

Half of the interfaces have “public” access modifiers, half are not. I wonder if this should not be improved so that all did not have it. In addition, it is worth noting that all methods in interfaces are public, so this modifier is skipped by the compiler.

Using a parameterized constructor instead of the addAll method can improve the readability of the code.

Following https://wiki.openmrs.org/display/docs/Java+Conventions: “Use primitives where possible Rather than Wrapper classes”.

These are not very complicated tasks, so if there is someone who can change the status of at least some of these issues to “ready for work”, it would let me start.

Discussion of individual subtasks is recommended if someone considers the subtask to be unnecessary.

4 Likes
2

cc @dkayiwa

3

Thanks @sacull for the great contribution to improve our code base. :slight_smile:

For each of the sub tickets, do you mind giving links to the lines of code that you are referring to?

4

Great idea, forgive me for not thinking about it. I updated the descriptions for all subtasks by adding links to the code.

5

I have made a couple of them ready for work. Since you seem to be interested in tickets that improve code quality, have you looked at the sonar reports? https://wiki.openmrs.org/display/docs/Sonar

1 Like
6

My caution and fixing minor bugs is because I prefer not to change larger pieces of code without understanding how it works. In time, I will understand him more.

I haven’t had the opportunity to use Sonar yet, but I can read about it in my free time. Thanks.

7

I like your reasoning! :smile:

And by the way, did you get a chance to look at our community priority tickets listed here? https://wiki.openmrs.org/display/docs/Getting+Started+as+a+Developer

8

Yes, I’ve seen them before, but first I want to know the source code better. Someday I will probably go back there. :wink:

1 Like
9

I took a closer look at SonarQube and noticed a lot of different messages there about where to fix the code. A large proportion of these places have been in the code for several years, perhaps it’s time to reduce their number. A few days ago, I prepared a few suggestions for improving the code, I present them below.

https://issues.openmrs.org/browse/TRUNK-5931

A rare case, but why rely on chance.

https://issues.openmrs.org/browse/TRUNK-5932

In this case, we can gain some memory, not much, but still.

https://issues.openmrs.org/browse/TRUNK-5933

Building if-else structures when we return a boolean can sometimes take up unnecessary lines of code, often it can be simplified while keeping the code readable.

https://issues.openmrs.org/browse/TRUNK-5934

When we see

if (something) {
    if (somethingElse) {
        doSomething()
    }
}

only one question arises: why?

https://issues.openmrs.org/browse/TRUNK-5935

I am not sure what it results from, but I know that the SonarQube rules were made by people who know the language much better than I do, so I think that it is also worth improving.

https://issues.openmrs.org/browse/TRUNK-5936

https://issues.openmrs.org/browse/TRUNK-5937

Naming Convention. Let’s not mislead the code readers.

https://issues.openmrs.org/browse/TRUNK-5938

These are just typos that don’t affect your code, but they don’t do much to reflect the code itself. Let’s fix it. :wink:

https://issues.openmrs.org/browse/TRUNK-5939

The variable was used once, then someone else changed the code and left the variable? For what? I think he just forgot about her.

https://issues.openmrs.org/browse/TRUNK-5940

Do you need to add anything else to this description?

https://issues.openmrs.org/browse/TRUNK-5941

In these places, I don’t think they are needed.

https://issues.openmrs.org/browse/TRUNK-5942

Since the two sources agree, all that’s left for me to do is fix it. :wink:

There are many tasks, but most of them do not require much work. They fix things that most often do not affect the code being executed, but often correcting them will improve the quality of the code itself. If someone who can switch these tasks to work-ready finds a few minutes to look at my suggestions, I will be very grateful for that.

1 Like
10

great work @sacull

1 Like
11

@sacull this is so cool of you. You have doubled our list of available intro tickets. I have just made all of them ready for work. Please keep it up! :smile:

2 Likes
12

I searched for them with the intention of doing them myself, but as I read your comment, you’re right. These are good and simple issues that sometimes someone asks on talk. I’ll do two or three for my own satisfaction and the rest will stay for someone who wants to join the community but still lacks coding skills.

1 Like
13

You deserve to be fully satisfied, for you did a great job! Feel free to even add more. :slight_smile:

3 Likes
14

If you can find a few minutes of time, please check if any of the following tickets are not suitable for implementation. Maybe one of them will fulfill the conditions to become “intro”. :wink:

SonarQube describes nicely why the exception object should be immutable: “Exceptions are meant to represent the application’s state at the point at which an error occurred.”, thus modifying the object can cause problems in identifying the cause of the exception.

It is surprising that these places with bad order of modifiers survived so long in the code. :wink:

Good practice and in my opinion better readability indicate that this:

if (booleanSomething) {}

is better than this:

if (booleanSomething == true) {}

Why declare a variable only to return it on the next line and exit the method? Wouldn’t it be easier to return the result right away, without creating unnecessary variables?

Inheritance. One of the four characteristics of object-oriented programming. So why is it not used here?

Clean code also removes unused imports. Some of them have been in the code for several years. :stuck_out_tongue:

Dear reader of this post, I am asking for your opinion whether the tickets described above have a reason to exist. Maybe one of them should not be corrected?

1 Like
15

Dear creator of these tickets, my opinion is that they should have existed yesterday. I just made all of them ready for work. Thanks again for your awesomeness! :smile:

4 Likes
16

@dkayiwa I noticed the need for simple tickets so that those willing to help, but still learning, could learn about the tools used to develop OpenMRS, such as JIRA, SonarQube or GitHub.

When I was creating the previous ones, I did not think that there would be people who would like to use them. You could observe it yourself.

Now I know that simple tickets like this are very necessary to seek help from novice programmers.

Therefore, I thought that I would also create such simple tickets on other components of OpenMRS. Today it is openmrs-module-registrationapp.

Please see the descriptions of these subtasks to see if they are sufficient.

“Removing unused elements from the code” group.

These tickets are designed to clear the code from unused elements, imports, variables.

“Code style improvement” group

These tickets are intended to correct the style of the source code.

Group “Code efficiency improvement”

This group aims to improve code efficiency, save memory or processor cycles.

If tickets of this type are useful, they can also be created for other modules. :wink:

1 Like
17

This is awesome @sacull previous tickets gave me more hands on with tools used by the platform. thanks

2 Likes
18

I have made all of them ready for development. Thank you so much @sacull :slight_smile:

2 Likes
19

Today something small, openmrs-module-referenceapplication. :wink:

I found four things in this module. Three of them are related to code cleanup:

The fourth one with no @Override annotation. The lack of annotation does not, of course, prevent the code from compiling, but for the reader of this code, this annotation is a very important indication that this method is being overwritten or implemented.

@dkayiwa I want to do the same with coreapps and webservices.rest modules and finish it. A larger number of such tickets is not needed (the ones that already exist and the two modules that I still want to prepare). Besides, more tickets are more difficult to maintain. These tickets need to be kept up to date because the code changes all the time, lines of code go up and down, unnecessary elements come and go, etc. What do you think? Make also these two modules or the number of tickets is enough?

20

The good thing about these simple tickets is that a newbie can take on a good number of them. So, go ahead and do for those two modules as you propose. As for code changes leading to lines going up or down, just reference the code using a released tag. For instance, use: https://github.com/openmrs/openmrs-module-referenceapplication/blob/referenceapplication-2.10.0/api/src/main/java/org/openmrs/module/referenceapplication/ReferenceApplicationActivator.java#L64 instead of: https://github.com/openmrs/openmrs-module-referenceapplication/blob/master/api/src/main/java/org/openmrs/module/referenceapplication/ReferenceApplicationActivator.java#L64

Otherwise, thanks again for your awesome contribution! :slight_smile:

1 Like