Code Review
निरंजन
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

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.