Skip to end of metadata
Go to start of metadata

The fools with the tools make the rules.
– anonymous

 

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 jira or any other method that records the reasons.

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 branching and merging. Also the release process is mostly linear. (Short-living topic branches are perfectly OK).

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 over explicit integration steps.

gitflow

MidPoint development is not using gitflow. There is no develop branch. The master branch is the branch when all the development happens.

Continuity

Keep the system working. As much as you can. You should not commit a code that breaks the build and go home. 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 few hours. 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 compilable 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 3-4 minutes if you skip the tests. Also execute the tests at least in the component that you have changed. Have a look at Bamboo automated build or Travis CI 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 trunk 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.

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 for all source files (Java, XML, properties, ...)
  • 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 needs to be maintained as much as the code, but they cannot be tested. Therefore there is danger that comments will get outdated quite quickly.
    • Comment what the code is expected to do, not what it does. 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 @throws section - 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 @author section. 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 will see 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. Simple delete it. 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.

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.

TODO

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 copy&paste the code. Create an utility method and place the common method there. Or think about the inheritance or composition. Maybe the need for copy&paste 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. IModel<ObjectWrapper<UserType>> is not user,  but userWrapperModel. 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 IModel<String>, not just IModel. 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 JIRA (and put the JIRA 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. 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 serialVersionUID to 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 the 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 exactly 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 immediatelly see what component is causing a problem in exception stack straces. 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 wiki page describing design of the component. The wiki page should describe ideas and motivations, contain diagrams and figures. The goal of the wiki page is to explain why the component is created like this (motivation) and provide a basic overview of how it works. The wiki 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 wiki 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 wiki 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: http://www.nlight.eu/documents/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.

TODO

  • Do not use code reformat function on the whole file. 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.

See Also

  • No labels