Code Review
add tag
निरंजन
Hello everyone,

I want to put a new snippet of code for code review in the community. It is regarding my package [`tipauni`](https://ctan.org/pkg/tipauni). [The git repository](https://git.gnu.org.ua/tipauni.git) of this package has [a branch](https://git.gnu.org.ua/tipauni.git/log/?h=exp-warnings) with the new code. The following steps on a terminal should take you to the latest .dtx, .ins and the generated PDF documentation. You can also use the l3build program for installing the package.

```bash
git clone git://git.gnu.org.ua/tipauni.git
cd tipauni/tipauni/
git checkout exp-warnings
l3build install # in case you want to install it in the local texmf 
```

In particular [this](https://git.gnu.org.ua/tipauni.git/commit/?h=exp-warnings&id=60a6768e5fa39227b6f0527b9bbff2ff8c3ce20b) commit will show every code-related addition.

Any kind of feedback is highly appreciated :)

As per the request of @samcarter, I am adding the _relevant_ bit of the code in a readable format in this question itself, but I would highly recommend using the generated .sty for testing as the new code is mostly about warnings which one gets during the compilation.

Also before posting the relevant code, I would like to give some background of the package itself. So historically for typesetting international phonetic alphabet (IPA) in LaTeX, package [`tipa`](https://ctan.org/pkg/tipa) was developed. It provided ASCII-based input method which was compiled with T3 encoding and in the output users could see the phonetic characters. Even after so many years of Unicode the linguistics community is yet to come out of that handy method of typing phonetic characters. So I wrote this package which bridges the gap between the Unicode encoding and the commands which encode the phonetic shapes. The package by default loads a phonetic font Charis SIL for printing the IPA characters properly, but [a recent bug-report](https://puszcza.gnu.org.ua/bugs/?539) complained about this and asked me to provide more options which would enable a user to not load any other font. The following snippet has the code for those options as well as a few warnings which are issued in some special circumstances. Package [`xkeyval`](https://ctan.org/pkg/xkeyval) is used to develop the package options.

```
\newif\ifcharissil
\newif\ifpreservefont
\newif\ifdocumentfont
\newif\iffontoptions
\newif\ifnontipaignore
\newcounter{tipauni@settings}
\setcounter{tipauni@settings}{3}
\documentfonttrue
\charissiltrue
\let\tipauni@font@options\@empty
\DeclareOptionX{preservefont}{%
  \documentfontfalse
  \charissilfalse
  \preservefonttrue
  \ifnum\value{tipauni@settings}<3%
    \setcounter{tipauni@settings}{3}%
  \else
  \fi
}
\DeclareOptionX{fontspecoptions}{%
  \edef\tipauni@font@options{#1}%
  \ifx\tipauni@font@options\@empty
  \else
    \fontoptionstrue
  \fi
}
\DeclareOptionX{resetfontspecoptions}{%
  \ifx\tipauni@font@options\undefined
  \else
    \let\tipauni@font@options\@empty
  \fi
  \fontoptionsfalse
}
\DeclareOptionX{documentfont}{%
  \edef\tipauni@temp{#1}%
  \ifx\tipauni@font\undefined
    \ifx\tipauni@temp\@empty
      \ifpreservefont
        \setcounter{tipauni@settings}{0}%
        \documentfontfalse
        \charissilfalse
      \else
        \setcounter{tipauni@settings}{1}%
        \documentfontfalse
        \charissilfalse
      \fi
    \else
      \ifnum\value{tipauni@settings}<3%
        \setcounter{tipauni@settings}{3}%
      \else
      \fi
      \edef\tipauni@font{#1}%
      \documentfonttrue
      \charissilfalse
    \fi
  \else
    \ifx\tipauni@temp\@empty
      \documentfonttrue
      \charissilfalse
      \setcounter{tipauni@settings}{2}%
    \else
      \documentfonttrue
      \charissilfalse
      \edef\tipauni@font{#1}%
    \fi
  \fi
}
\DeclareOptionX{recommendedfont}{%
  \documentfonttrue
  \def\tipauni@font{CharisSIL}%
  \setcounter{tipauni@settings}{3}%
}
\DeclareOptionX{incompatible}{\nontipaignoretrue}
\ProcessOptionsX\relax
\ifnum\value{tipauni@settings}=0%
  \PackageWarningNoLine{tipauni}{%
    1. The `preservefont' option is active.\MessageBreak
    2. The `documentfont' option has no value.\MessageBreak
    Your document will be compiled with the default\MessageBreak
    font-family of (Xe/Lua)LaTeX, i.e., Latin Modern\MessageBreak
    unless another font is set with tipauni-external\MessageBreak
    methods%
  }%
\else
  \ifnum\value{tipauni@settings}=1%
    \PackageWarningNoLine{tipauni}{%
      The `documentfont' option has no value. Your\MessageBreak
      document will be compiled with the default\MessageBreak
      font-family of (Xe/Lua)LaTeX, i.e., Latin Modern\MessageBreak
      unless another font is set with tipauni-external\MessageBreak
      methods%
    }%
  \fi
  \ifnum\value{tipauni@settings}=2%
    \PackageWarningNoLine{tipauni}{%
      A `documentfont' option has no value, but you have\MessageBreak
      loaded another font with some tipauni-option, so that\MessageBreak
      font will be used as the main font of this document\MessageBreak
      unless another font is set with tipauni-external\MessageBreak
      methods. The font being currently loaded is:\MessageBreak
      \tipauni@font
    }%
  \else
  \fi
  \ifnum\value{tipauni@settings}=3%
  \else
  \fi
\fi
\ifcharissil
  \def\tipauni@font{CharisSIL}%% https://ctan.org/pkg/charissil
\else
  \ifdocumentfont
  \else
    \iffontoptions
      \PackageWarningNoLine{tipauni}{%
        Either the `preservefont' option has made the\MessageBreak
        tipauni-font ineffective, or you have issued the\MessageBreak
        `documentfont' option with an empty value. An active\MessageBreak
        font loaded with package fontspec is necessary for\MessageBreak
        `fontspecoptions' to take effect. Please use the\MessageBreak
        `documentfont' with a value to make `fontpecoptions'\MessageBreak
        effective. Currently the ignored  fontpsec option(s)\MessageBreak
        is (are) as follows:\MessageBreak
        \tipauni@font@options
      }%
    \fi
  \fi
\fi
\ifdocumentfont
  \expandafter\setmainfont\expandafter[\expandafter{\tipauni@font@options}]{\tipauni@font}
\else
\fi
```
Top Answer
Skillmon
Note: This review is not based on the code in above's answer, but on  commit 28c7fdc.

----

In this review I'll first address your code directly and give some nagging on the code as it is, not on primarily on its general behaviour. After that I'll present my thoughts on the behaviour.

----

# Nitpicky code review on file `tipauni.dtx`

## Initialisation of the variables (line 523ff):

```
\newif\ifcharissil
\newif\ifpreservefont
\newif\ifdocumentfont
\newif\iffontoptions
\newif\ifnontipaignore
```

Those are internal macros and should be named accordingly, e.g., `\newif\iftipauni@charissil` etc.

```
\newcounter{tipauni@settings}
\setcounter{tipauni@settings}{3}
```

Do you really need a counter for this? You could as well define a macro to save the state (still with numeric values), or use a count register directly (LaTeX's `counter` mechanism is a bit overkill for storing an internal state, as it includes printing the counter, and potential reset hooks for other counters, etc.; though the latter is not invoked by `\setcounter`, only by `\(ref)stepcounter`).

Suggested code:

```
\newcount\tipauni@settingsstate
\tipauni@settingsstate=3
```

(the missing `%` after 3 to suppress the line ending from adding a space is deliberate, this way the space terminates the numeric expression and you don't accidentally expand things in the next line that shouldn't be expanded just yet, if you prefer you could also use `\tipauni@settings=3\relax` to make this more obvious)

## Definition of the `preservefont` option (line 554ff):

```
\DeclareOptionX{preservefont}{% 
  \documentfontfalse
  \charissilfalse
  \preservefonttrue
  \ifnum\value{tipauni@settings}<3%
```
Is there any possibility of `tipauni@settings` getting bigger than 3? If not, just omit the test and use `\setcounter{tipauni@settings}{3}`, gives clearer code, KISS.
```
    \setcounter{tipauni@settings}{3}%
  \else
```
Empty `\else` branch, you could drop the `\else`.
```
  \fi
}
```

Suggested alternative implementation:

```
\DeclareOptionX{preservefont}{% 
  \documentfontfalse
  \charissilfalse
  \preservefonttrue
  \setcounter{tipauni@settings}{3}%
}
```

## Definition of the `resetfontspecoptions` option (line 578ff):

```
\DeclareOptionX{resetfontspecoptions}{%
  \ifx\tipauni@font@options\undefined
```
That condition can never be met, line 546 did `\let\tipauni@font@option\@empty`, and no option undefines it, so this `\ifx` test can be omitted.
```
  \else
    \let\tipauni@font@options\@empty
  \fi
  \fontoptionsfalse
}
```

Suggested alternative implementation:

```
\DeclareOptionX{resetfontspecoptions}{%
  \let\tipauni@font@options\@empty
  \fontoptionsfalse
}
```

## Definition of the `documentfont` option (line 590ff):

```
\DeclareOptionX{documentfont}{%
  \edef\tipauni@temp{#1}%
  \ifx\tipauni@font\undefined
```
You could use `\ifdefined` instead (*Attention* this would invert the logic, so true and false branch need to be swapped).
```
    \ifx\tipauni@temp\@empty
      \ifpreservefont
        \setcounter{tipauni@settings}{0}%
        \documentfontfalse
        \charissilfalse
      \else
        \setcounter{tipauni@settings}{1}%
        \documentfontfalse
        \charissilfalse
      \fi
```
No code doubling! If something happens in both cases don't put it inside both if-branches, instead just move it outside of it.
```
    \else
      \ifnum\value{tipauni@settings}<3%
        \setcounter{tipauni@settings}{3}%
      \else
      \fi
      \edef\tipauni@font{#1}%
```
Unnecessary `\edef`, just `\let\tipauni@font\tipauni@temp`.
```
      \documentfonttrue
      \charissilfalse
    \fi
  \else
    \ifx\tipauni@temp\@empty
      \documentfonttrue
      \charissilfalse
      \setcounter{tipauni@settings}{2}%
```
Isn't this running into an open knife? You know that no font has been provided yet (`\tipauni@font` is still undefined), and the given option doesn't hold a valid font (`\tipauni@temp` being empty). This should throw an error, not only a warning (the warning is thrown further down the road, in line 713ff; and this is the only case setting `tipauni@settings` to 2)!
```
    \else
      \documentfonttrue
      \charissilfalse
      \edef\tipauni@font{#1}
```
Why doesn't this `\setcounter{tipauni@settings}{3}`? This should be a valid case, shouldn't it?
```
    \fi
```
Again code doubling and `\edef`! (and `\charissilfalse` is used in every case!)
```
  \fi
}
```

Suggested alternative implementation (this contains a behaviour change, namely it would set `tipauni@settings` to 3 if the given argument wasn't empty regardless of the previous state of ``\tipauni@font`):

```
\DeclareOptionX{documentfont}{%
  \charissilfalse
  \documentfonttrue
  \edef\tipauni@temp{#1}%
  \ifx\tipauni@temp\@empty
    \ifdefined\tipauni@font
      \setcounter{tipauni@settings}{2}%
    \else
      \documentfontfalse
      \ifpreservefont
        \setcounter{tipauni@settings}{0}%
      \else
        \setcounter{tipauni@settings}{1}%
      \fi
    \fi
  \else
    \let\tipauni@font\tipauni@temp
    \setcounter{tipauni@settings}{3}
  \fi
```

## Definition of the `recommendedfont` option (line 667ff)

```
\DeclareOptionX{recommendedfont}{%
  \documentfonttrue
  \def\tipauni@font{CharisSIL}%
  \setcounter{tipauni@settings}{3}%
}
```

Why not just `\charissiltrue` and `\documentfonttrue`?

## Error parsing (line 684ff):

```
\ifnum\value{tipauni@settings}=0%
```
Do you know the `\ifcase` primitive?
```
  \PackageWarningNoLine{tipauni}{%
    1. The `preservefont' option is active.\MessageBreak
    2. The `documentfont' option has no value.\MessageBreak
    Your document will be compiled with the default\MessageBreak
    font-family of (Xe/Lua)LaTeX, i.e., Latin Modern\MessageBreak
    unless another font is set with tipauni-external\MessageBreak
    methods%
  }%
```
If the `preservefont` option is active this should be a no-issue, as it should mean "don't touch the font setup" which doesn't conflict with `documentfont` having no value, the real error is that someone used `documentfont` with an empty value, which should always be an error, shouldn't it (at least it is not documented that there is a special "empty value" syntax)?
```
\else
  \ifnum\value{tipauni@settings}=1%
    \PackageWarningNoLine{tipauni}{%
      The `documentfont' option has no value. Your\MessageBreak
      document will be compiled with the default\MessageBreak
      font-family of (Xe/Lua)LaTeX, i.e., Latin Modern\MessageBreak
      unless another font is set with tipauni-external\MessageBreak
      methods%
    }%
  \fi
  \ifnum\value{tipauni@settings}=2%
    \PackageWarningNoLine{tipauni}{%
      A `documentfont' option has no value, but you have\MessageBreak
      loaded another font with some tipauni-option, so that\MessageBreak
      font will be used as the main font of this document\MessageBreak
      unless another font is set with tipauni-external\MessageBreak
      methods. The font being currently loaded is:\MessageBreak
      \tipauni@font%
```
Can this branch be actually called with a non-empty `\tipauni@font`?
```
    }%
  \else
  \fi
  \ifnum\value{tipauni@settings}=3%
  \else
  \fi
```
Neither the true nor the false branch are used, so why bothering checking this case anyway?
```
\fi
```

Suggested code:

```
\ifcase\value{tipauni@settings}% case 0
  \PackageWarningNoLine{tipauni}{%
    1. The `preservefont' option is active.\MessageBreak
    2. The `documentfont' option has no value.\MessageBreak
    Your document will be compiled with the default\MessageBreak
    font-family of (Xe/Lua)LaTeX, i.e., Latin Modern\MessageBreak
    unless another font is set with tipauni-external\MessageBreak
    methods%
  }%
\or% case 1
  \PackageWarningNoLine{tipauni}{%
    The `documentfont' option has no value. Your\MessageBreak
    document will be compiled with the default\MessageBreak
    font-family of (Xe/Lua)LaTeX, i.e., Latin Modern\MessageBreak
    unless another font is set with tipauni-external\MessageBreak
    methods%
  }%
\or% case 2
  \PackageWarningNoLine{tipauni}{%
    A `documentfont' option has no value, but you have\MessageBreak
    loaded another font with some tipauni-option, so that\MessageBreak
    font will be used as the main font of this document\MessageBreak
    unless another font is set with tipauni-external\MessageBreak
    methods. The font being currently loaded is:\MessageBreak
    \tipauni@font%
  }%
\fi
```

Also change the "everything is fine" case to `-1` instead of `3`. Rationale: If you get additional error cases you can just add them to the above `\ifcase...\or...\fi` structure, `-1` is never reached by it (only half true, `\ifcase` supports an `\else` that handles every unhandled case).


-----

# General behaviour rambling

Why do you bother evaluating the settings on the fly to set `tipauni@settings`?  In the end you throw any warnings/errors only after the fact, so why not evaluate the options only by then? The current implementation makes your options much more complicated than they have to. Compare all of above with the following (admittedly not the exact same behaviour, but should cover the bare necessities and give you an idea on what I mean -- sorry for ):

```
\newcommand*\tipauni@font@options{}
\newif\iftipauni@preserve
\newif\ifnontipaignore
\DeclareOptionX{preservefont}{\tipauni@preservetrue}
\DeclareOptionX{fontspecoptions}{\edef\tipauni@font@options{#1}}
% I'd expect this option to do more than just `options={}`
\DeclareOptionX{resetfontspecoptions}
  {%
    % in this implementation the undefined variable caries the information of
    % your `\charissiltrue`
    \let\tipauni@font\tipauni@undefined
    \let\tipauni@font@options\@empty
  }
\DeclareOptionX{documentfont}{\edef\tipauni@font{#1}}
\DeclareOptionX{recommendedfont}{\def\tipauni@font{CharisSIL}}
\DeclareOptionX{incompatible}{\nontipaignoretrue}
\ProcessOptionsX\relax
\iftipauni@preserve
  \ifx\tipauni@font@options\@empty
  \else
    \PackageWarning{tipauni}
      {%
        You specfied font options while `preservefont` is active.\MessageBreak
        Either completely set your font options outside of tipauni\MessageBreak
        or leave out the `preservefont` option.%
      }%
  \fi
\else
  \ifx\tipauni@font\@empty
    \PackageError{tipauni}{Empty documentfont option}
      {%
        If you want tipauni to not set any font related options use the
        preservefont option%
      }%
  \else
    \ifdefined\tipauni@font
    \else
      \def\tipauni@font{CharisSIL}
    \fi
    \expandafter\RequirePackage\expandafter[\expandafter{\tipauni@font@options}]
      {\tipauni@font}%
  \fi
\fi
```

You don't win anything from evaluating everything on the spot if you don't act on it. Either check whether `documentfont`'s value is empty and if so throw an error immediately, or don't only check this when you actually need to access this. Since you only need to access this once, but the possibility exists that it is set multiple times, I'd opt for lazy evaluation instead of an eager approach.

Enter question or answer id or url (and optionally further answer ids/urls from the same question) from

Separate each id/url with a space. No need to list your own answers; they will be imported automatically.