Thanks for your contribution. Codewise looks good to me so far.
I think it would make sense to change the cite generation to encode the unique thing when combining, but then the parse ref Names needs to be adjusted as well and many other cases as well.

I think it would make sense to change the cite generation to encode the unique thing when combining, but then the parse ref Names needs to be adjusted as well and many other cases as well.

I started a new branch to reorganize OOBibBase.java, at https://github.com/antalk2/jabref/tree/improve-reversibility

So far it should contain no changes to user-visible functionality,
mainly reorganization of the code. Parsing of ref marks is now only in parseRefMarkName
and isJabRefReferenceMarkName

On the other hand, reorganizing means lots of changes,
many of which may seem or turn out to be superfluous,
since I am editing while trying to understand what is going on.

I am worried, that with so many changes I end up with
something that will not be merged, due to the amount of changes.

Please let me know what to expect and what to watch out for.

Probably the best would be to merge this one and you keep working on your branch. But try to focus on one issue change per commit, so we can understand what you changed, commit by commit. You can also label your PR as WIP and draft, growing as you work on it, and we check regularly in and have a look at your changes.
You can even comment on your own code in the files changed tab, so you can discuss your changes in that PR with us.

Don't worry, working on JabRef together should be fun. We don't bite. 😄

Probably the best would be to merge this one and you keep working on your branch. But try to focus on one issue change per commit, so we can understand what you changed, commit by commit. You can also label your PR as WIP and draft, growing as you work on it, and we check regularly in and have a look at your changes.
You can even comment on your own code in the files changed tab, so you can discuss your changes in that PR with us.

Don't worry, working on JabRef together should be fun. We don't bite. smile

@antalk2 what do you think about merging this one first, and then having the further refactoring an a second PR?

@antalk2 what do you think about merging this one first, and then having the further refactoring an a second PR?

Yes. I think this is better as a separate merge.
Do I need to do something for that?

I am doing the refactoring in a separate branch.

Ok, I've reopened this PR then. If you could address the small comments by @calixtus and me, then we can merge. Thanks!

Ok, I've reopened this PR then. If you could address the small comments by @calixtus and me, then we can merge. Thanks!

I believe I did that. Let me know if I left out something.

Style checks (reviewdog) often ask me to make the code less readable.
Shall I ignore it, or follow its suggestions anyway?

The style check is important to have consistency across the repository and to prevent merge conflicts of unrelated changes e.g. whitespaces or line breaks. You can import JabRef's code style in intellij and do a kind of
https://devdocs.jabref.org/getting-into-the-code/guidelines-for-setting-up-a-local-workspace#configure-your-ide

The style check is important to have consistency across the repository and to prevent merge conflicts of unrelated changes e.g. whitespaces or line breaks.

This sounds like "follow its suggestions anyway". OK, I will try that.

Well, we configured reviewdog to automate the suggestions, otherwise we would make. What suggestions in particular do you think makes the code less readable?

Well, we configured review dog to automate the suggestions,
otherwise we would make. What suggestions in particular do you think
makes the code less readable?

1. No whitespace in sin(x) sounds OK, but

   longVariableName.longMethodname(another.long(something))

is harder to read than

   longVariableName.longMethodname( another.long(something) )

Style checker documentation says:

Abstract class for checking the padding of parentheses. That is
whether a space is required after a left parenthesis and before a
right parenthesis, or such spaces are forbidden.

Apparently they support both required and forbidden (and implicitly, noCheck)

2. Avoid nested blocks

Rationale:
Nested blocks are often leftovers from the debugging process, they
confuse the reader.

Inner blocks are useful in restricting scope of local variables,
assuring the reader they are not referred to later.
Current style checks disallow this.

   void f(x) {
       ...
       String[][] bibtexkeys;
       {
           Some localvar ... // used to calculate initial value for bibtexkeys
           ...
           bibtexkeys = ...
       }
       ... more code here: does it use localvar?
   }

Style checker seems to prefer this:

   void f(x) {
       ...
       String[][] bibtexkeys;
       Some localvar ... // used to calculate initial value for bibtexkeys
           ...
       bibtexkeys = ...
       ... more code here: does it use localvar?
   }

3. Line up, for example assignments, variable declatarions (style checker insists on single space)

assert another.length == field.length;
assert anEvenLongerName == field.length;
this.field = field;
this.another = another;
this.anEvenLongerName = anEvenLongerName;

or

assert another.length   == field.length;
assert anEvenLongerName == field.length;
this.field            = field;
this.another          = another;
this.anEvenLongerName = anEvenLongerName;

I know, when this.anEvenLongerNameTheSecond arrives, we either loose
the line up, or change unrelated lines, leading to potential merge
problems. Google is more permissive here than
the style checker in JabRef.

4. Where should the brace go?

   Easy

   void f(x) {  //<- brace here, ok
      body here
   }
   

   With a longer head brace on next line could improve visual separation of head and body.

   void f(x,
          y,
          z)
         throws XException,
                YException,
                ZException {  //<- brace here according to style
      body here
   }
   

   void f(x,
          y,
          z)
         throws XException,
                YException,
                ZException
   {
      body here
   }
   

Thanks for your answer. I think we (@JabRef/developers) maybe talk about these considerations in the next devcall.

Well, we configured review dog to automate the suggestions,
otherwise we would make. What suggestions in particular do you think
makes the code less readable?

1. No whitespace in sin(x) sounds OK, but

   longVariableName.longMethodname(another.long(something))

is harder to read than

   longVariableName.longMethodname( another.long(something) )

Style checker documentation says:

Abstract class for checking the padding of parentheses. That is
whether a space is required after a left parenthesis and before a
right parenthesis, or such spaces are forbidden.

Apparently they support both required and forbidden (and implicitly, noCheck)

1. Avoid nested blocks

Rationale:
Nested blocks are often leftovers from the debugging process, they
confuse the reader.

Inner blocks are useful in restricting scope of local variables,
assuring the reader they are not referred to later.
Current style checks disallow this.

   void f(x) {
       ...
       String[][] bibtexkeys;
       {
           Some localvar ... // used to calculate initial value for bibtexkeys
           ...
           bibtexkeys = ...
       }
       ... more code here: does it use localvar?
   }

Style checker seems to prefer this:

   void f(x) {
       ...
       String[][] bibtexkeys;
       Some localvar ... // used to calculate initial value for bibtexkeys
           ...
       bibtexkeys = ...
       ... more code here: does it use localvar?
   }

1. Line up, for example assignments, variable declatarions (style checker insists on single space)

assert another.length == field.length;
assert anEvenLongerName == field.length;
this.field = field;
this.another = another;
this.anEvenLongerName = anEvenLongerName;

or

assert another.length   == field.length;
assert anEvenLongerName == field.length;
this.field            = field;
this.another          = another;
this.anEvenLongerName = anEvenLongerName;

I know, when this.anEvenLongerNameTheSecond arrives, we either loose
the line up, or change unrelated lines, leading to potential merge
problems. Google is more permissive here than
the style checker in JabRef.

1. Where should the brace go?
   Easy

   void f(x) {  //<- brace here, ok
      body here
   }
   

   With a longer head brace on next line could improve visual separation of head and body.

   void f(x,
          y,
          z)
         throws XException,
                YException,
                ZException {  //<- brace here according to style
      body here
   }
   

   void f(x,
          y,
          z)
         throws XException,
                YException,
                ZException
   {
      body here
   }
   

I'm sorry but I disagree with all of your suggestions and prefer the current JabRef code style. @JabRef/developers

1. it is better to keep params short, so you could also extract the long param to a var.
2. totally agree with the given rationale why inner braces should not be used. your case to restrict use of vars should be a rare exception and you're better of to extract that into a function then
3. might look nice but leads to a lot of refactorings when you add a new assignment. we wanted to kep the number of "unrelated" line changes in diffs low to make it easier to review PRs
4. of course only a preference, but we stick with the standard Java code style here to add the brace in the same line. might be too restrictive in oyur example, but also here we tried to keep the number of rules low and simple without exceptions.

I'm sorry but I disagree with all of your suggestions and prefer the current JabRef code style. @JabRef/developers

It is OK, I do not expect or suggest to change JabRef code style, just shared my preferences.

1. I accept the rules, even if I do not always think they are optimal.
2. I am not a maintainer (here or elsewhere), thus probably underestimate the probability and burden of merging conflicts. I am aware of this, so I do not intend to shape your code style to my preferences.

You can import JabRef's code style in intellij and ...

Thank you for this suggestion. Installing IDEA helped a lot to find a style
accepted by the checker.

Thanks again for your contribution, I tested your feature with both an author year and the numerical style and it works.

Checks are good, two approvals, @Siedlerchr confirmed its working. Thanks for your great contribution!