Development Guidelines
Rules
The fools with the tools make the rules.
Following rules should be followed all the time. Everybody must follow the rules.
The first rule of midPoint development is: You do not talk about midPoint development rules.
MidPoint development has no strict unbreakable rules. Just guidelines. Every member midPoint development team is expected to use his brain all the time. We rely more on the judgment of midPoint developers than we rely on the ephemeral power of the rules.
Guidelines
Guidelines are just recommendations. Guidelines describe the method of doing something in the common case. They are kind of best practice. Guidelines should usually be followed, but they are not cast in stone. Everybody may choose not to follow guidelines if they know what they are doing. Guidelines may also be dangerous in some (rare) situations. Therefore everybody must choose not to follow a guideline in case that following a guideline would cause major problems in the project.
However, if you do not follow the guidelines please be prepared to explain why you have done what you have done. The best way how to deal with this is to document all exceptions from the guidelines and especially document why the guidelines were not followed. Use source comments, wiki, comments in the issue tracking system or any other method that records the reasons.
Mostly Linear Development
We keep the development as linear as possible. It means that vast majority of the development happens directly in the master branch. We usually try to avoid excessive branching and merging. Also the release process is mostly linear.
Short-living feature (topic) branches are perfectly OK. Those branches are even recommended for bigger features that risk breaking the code on master branch. However, try to merge the feature branches to master as soon as possible. Do not wait while your feature is done. The right time to merge is as soon as the risk of breaking master branch is low.
Practice:
Update you workspace frequently.
Commit frequently.
Keep the code synchronized.
Why we want this?
Branching means that the code in the branch is developed in isolation.
This complicates the subsequent merging as the main branch (master) also evolves in the meantime.
The merging usually requires a special integration step which takes extra effort and is quite unpredictable.
We prefer continuous integration of the code.
We do not want explicit integration steps.
|
No gitflow in my project, please.
MidPoint development is not using gitflow.
There is no |
Continuity
Keep the system working. As much as you can. You should not commit a code that breaks the build and then go home happily. If you have code that breaks the build, fix the system before you commit. No matter where the build fails (in what component) just go there and fix it. If you cannot fix it, then talk to the developer that can fix it before your commit and agree when it will be fixed. You can make commit that breaks the build only if you are sure that someone else will fix it in the next hour or so. And you are expected to stay online during these few hours and make sure that it is really fixed. If it is not, you are expected to roll back the changes and make the system buildable again. If you are going to make a major change that can break the system for few days, you are required to branch the code and merge it back when it will work again. The same principle also applies to broken tests, but with a less strictness in this case.
In short: The system should be buildable all the time and the tests should pass.
Practice:
Build the code before commit.
Building the complete code takes few minutes if you skip the tests.
Also execute the tests at least in the component that you have changed.
Have a look at Continuous Integration to make sure that the build is OK and that the tests pass.
Why we want this?
As the whole team shares the code breaking the code affects all team members.
Breaking the master branch interrupts the work and therefore it means a waste of time.
Even a short interruption may be considerable waste if it is multiplied by many developers.
No Code Ownership
We do not have my code and your code in midPoint. All code is midPoint code. Anybody may change any code at any time - assuming that he knows what he is doing (but also see Continuity guideline). However, midPoint code base is quite big and complex. It is beyond a capability of any single developer to understand all the code. Therefore if you are not confident that you know what you are doing please contact the original author (it should be identified in the source file header). Or contact the architect. Or just ask anybody.
Why we want this?
Because of continuity and efficiency.
You need to fix all the code that is affected by the change you have just done.
It does not matter whether the code that needs to be fixed is created by you or not.
It needs to be fixed.
Right now.
Sharing the code also spreads understanding through the team.
Test-Driven Bugfixing
If you encounter a bug write the test first. Then fix the bug. Do not be tempted to fix the bug right away. See Test-Driven Bugfixing page for details.
Practice:
Replicate the bug in a test (most likely an integration test).
See how the test fails (this is important).
Fix the bug.
See the test pass.
Why we want this?
Tests make sure that the code works.
But how we can make sure that the tests work? That they do what they are supposed to do? We can write tests for the tests but that is quite insane.
We rather write the test when the bug is still there and see that the test fails.
This is an indication that the test works well to detect the bug.
Ten fix the bug and see that the test passes.
This is an indication that the bug is gone.
Coding Guidelines
General Guidelines
-
Don’t forget to use license statement in each file. Add appropriate template in your IDE.
-
Use UTF-8 encoding for all source files (Java, XML, properties, …)
-
Indent by 4 spaces
-
Code readablity
-
Use good names for classes, methods, variables and everything else. Don’t be afraid to use even long, descriptive names. If you must use short names, make sure you explain it well in the documentation (javadoc).
-
Readable code is better than comments. Choose good class, method and variable names. The time invested in looking for a good name almost always pays off. But this is not an excuse for not using comments at all!
-
Use comments if necessary. But not too much. Comments need to be maintained as much as the code, but they cannot be tested. There is danger that comments will get outdated quite quickly.
-
Comment what the code is supposed to do, not what it does. Document the reasons why the code is there. Unless the code is very complex, what the code does is obvious from the source itself.
-
Use Javadoc for classes. If the class purpose is not entirely obvious please add at least one sentence that describes responsibility of the class: the reason why the class exists.
-
You are not required to write javadoc for all the methods. But writing javadoc for methods that form midPoint interfaces is (of course) encouraged.
-
If you document an interface, document all exceptions in javadoc
@throwssection - especially runtime exceptions. -
Delete any comments that are not up-to-date. Just delete them. Lying comments are worse than no comments.
-
-
If you make a substantial change to a source file, add yourself to the
@authorsection. So others will know who they should ask if they run into problems reading your code. -
Do not use public fields (except for constants). Use getter/setter methods.
-
Do not forget about appropriate logging. Logging is important. Do not use stdout or stderr. Just don’t. Use logging instead. If anyone sees any use of stdout or stderr he will delete that immediately.
-
Delete all commented-out code that is obviously older that a couple of days. Simply delete it. No mercy. Do not ask anyone. We do not want commented-out code. If you want to comment-out code and you have a good reason to do so then document your reasons in the comments. Otherwise, your code will disappear.
-
Do not use code reformat function on the whole file. Formatters often break a lot of things and make code unreadable. Do not use code formatters without thinking. It would be best not to use reformat at all. Set proper code template and let the IDE do its work. Re-indent the code manually if needed, or use formatter only on a small piece of code that you can easily check for readability problems.
Error Handling
Avoid using throws Exception, catch (Throwable t) and similar rough constructs.
Use the most specific subclass for thrown exceptions.
If you catch exception, either react to it or re-throw it.
Avoid using catch (FooException e) { // do nothing }} as much as you can.
No More Hacking!
MidPoint is no longer a young product. During the years midPoint matured and now it is reasonably stable system. However, we will not pretend that everything in midPoint is perfect and that we do not have any technological debt. As all practical systems there are things in midPoint that really need improvement. As midPoint has matured the developers also need to behave responsibly. Therefore this is the plan:
-
Do not make the situation worse. No more hacking and workarounds and no more let’s just try this… If you implement something try to do it properly:
-
Do not copypaste the code. Create a utility method and place the common method there. Or think about the inheritance or composition. Maybe the need for copypaste suggests that your class structure is wrong?
-
Try to reuse existing code. There are many classes with utility methods. Try to reuse code that is already present there. Or improve the code, make it more generic, more reusable. And then use it.
-
Choose good names for classes, method and especially variables. E.g.
IModelObjectWrapperUserTypeis notuser, butuserWrapperModel. It makes things much easier to read especially in very confusing code working with models and wrappers in the GUI. -
Use proper generics. It is always
IModelString, not justIModel. Never use raw types and do not use java annotations to simply turn off the warnings (unless you are fighting the Java type system and there is really no reasonable way around that). -
Clean up your code. If you are finished with a feature take few minutes to look over your new code. Have the classes, methods and variables good names? Are there any warnings in the code? Have you left any commented-out code? If you spend few days working on the feature spending one more hour to clean up your code will not do any harm. And it will make the life easier for everybody.
-
-
Continuously clean up the code:
-
If you see any code or structure that is obviously bad do not ignore it. This includes duplicated code, code that is obviously outdated, very convoluted or unreadable code, etc. Always do something about it. If it takes hour or two to fix it then do it immediately. Right now. If it takes longer then create an issue in OpenProject (and put the issue ID in the comment in the code so it will not get duplicated). But whatever happens do not ignore bad code.
-
When you see invocation of a deprecated method try to remove it. Anytime. All the time. Even if you are just working on something else and you see that some method is deprecated then fix the invocation immediately.
-
Eliminate the warnings. Warning are ugly pests. It is easy to eliminate most warnings. Just add
serialVersionUIDto an anonymous class, add proper generic type, etc. Always when you open any class and you see a warning try to eliminate it. It is usually perfectly safe. -
Always rename variables and private methods if they have wrong names. This is very quick and very safe refactoring. There is absolutely no excuse for not doing this.
-
-
When in doubt, ask! Do not be afraid to ask any question. We are well aware that our documentation is not perfect and our code is not always readable. So go ahead and ask! Anything. Anytime.
Code Structure Guidelines
Components
Components are midPoint building blocks.
The bricks that we use to construct the system.
Components are meant to be well encapsulated.
Components hide most of the details inside and expose the functionality using well-defined interfaces.
Component internal structures and methods should never be accessed directly from other components (regardless of whether these are marked as public) - except for components explicitly designed to be shared libraries.
See also Interfaces section below.
Each component has at least one primary maintainer in midPoint core team. Maintainer is not owner of the code. We do not follow a concept of code ownership. Maintainer just coordinates the development of the component. Maintainer should make sure that the all the component development efforts make sense, that the changes made by two developers do not conflict and that the component will be stable. The maintainer is not expected to do all the work by himself. Others are not prohibited to touch the component code. Quite the contrary. Everybody is more than welcome to fix bugs and make reasonable improvements anywhere in the system. However, if someone is doing a major change in the component, he should talk to the maintainer before doing that change - to make sure that the change will not clash with other activities, will not endanger deadlines and will not ruin a long-term plan for component development.
It is not possible for one person to understand all the details in the entire system. Therefore we need to split the knowledge a bit. The maintainer should understand exactly how the component works. He is expected to understand all the details, to be able to explain every line of source code in the component (or know where to look for an answer). Other team members will ask question in case they do not understand something about the component.
Each component should have a short name, e.g. foo.
This name should be unique in the whole project.
The name should be consistently used to identify the component.
Especially:
-
Component directories should use the component short name as prefix and should be placed inside appropriate subsystem directory. E.g.
model/foo-api, model/foo-impl. -
Java package name should contain the component name and appropriate subsystem name (if applicable). E.g.
com.evolveum.midpoint.model.foo. -
Artifact names produced by the component build (JARs, WARs, XML files) should contain component name.
Motivation: We want to recognize components at the first sight. We want to immediately see what component is causing a problem in exception stack traces. We want to be able to easily find out what components are in a service assembly by just looking at the file names. We want to navigate the source tree easily.
Each component should have a documentation page describing design of the component. The page should describe ideas and motivations, contain diagrams and figures. The goal of the page is to explain why the component is created like this (motivation) and provide a basic overview of how it works. The page should be used as an entry point for people new to midPoint and also as a way how to communicate component design to others in midPoint team. The pages should not dive too much into the details how the actual source code works. Use comments in the source code to describe that. The page should be short enough to maintain it efficiently. Outdated design page is misleading and it is often worse than no design page at all. Please keep the design page up to date.
The component should also be listed on Source Code Structure page.
Interfaces
Keep in mind that interfaces define the contract.
Interfaces are much more than just a code.
In short, all interfaces should have good description of all operations and data structures.
Use annotation elements in WSDL and XSD and javadoc in Java.
Use them a lot.
Full recommendation regarding interface definition can be found here: https://dracones.ideosystem.com/work/papers/2010-semancik-interface-definition.pdf
If the interface is meant to be public it should be listed in Interfaces page.
Dependency Guidelines
Keep number of dependencies reasonable. Do not add new dependency just because you need one simple function that you can easily create and maintain yourself.
Be careful about dependency versioning. If we use a dependency, we use single version of that dependency in all midPoint components. As our dependencies can have dependencies of their own, this may be quite tricky (a.k.a dependency hell). We are using dependency convergence plugin to check for some bad situations. But this plugin is not almighty. Therefore be careful.
If you add any new dependency, always notify midPoint architects. The dependency need to be checked for licensing compatibility. Also, NOTICE file may need to be updated. We also want to limit dependency creep. This is very important.
As a rule of the thumb, is it always a good idea to discuss all dependency changes with midPoint architects.