Code review request: Base converter

Question

I do a lot of low level programming at work. As such I often found myself pulling up an online base converter to check my conversions. I relized that I wanted a way that I could do this from vim and thus I wrote this plugin.

Most notably it finds possible numbers as you type and gives you a completion list for the most common conversions. However, there are also command line commands to simply convert one base to another (Any base 2-16), and a mapping <A-f> to pull up common conversions for the number under the cursor.

I'm sure there are many things that could be optimized which is the reason I'm posting it here.

Up until now I was only using this for myself and so I haven't bothered with any of the <SID> stuff yet. I've never used them before so I need to research best practices etc. If any of you have any advice on anything regarding that I will happily take it.

I'm running vim 8.0.69 and usually use gVim.

And so without further ado, baseConverter.vim:

" @Tracked
" Base Conversion Plugin
" Author: Tumbler Terrall
" Last Edited: 02/03/2017 03:07 PM
" Version: 1.2

let g:vimBaseConversion = 1

command! -nargs=1 Hexcon call HexConverter('<args>')
" Automatically detect base and convert into 4 most common bases
command! -nargs=+ Base2Base echo BaseConversion(<f-args>)
" Convert any base to any other (2-16)
command! ASCII call PrintASCIIChart()
" Prints out a staic table for quick number to ASCII conversions

nnoremap <A-f> :call HexConverter(expand('<cword>'))<CR>
" Brings up the hex converter on number under cursor

if has("autocmd")
augroup BaseConversion
   au!
   autocmd CursorMovedI * call CheckConversions()
augroup END
endif

" CheckConversions ><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><>
"  brief: Takes word under cursor and determines if it's a number. If it is it
"         will pull up a conversion list, either in the completion list or in
"         a command line print out. (This function is called automatically
"         from a CursorMovedI event)
"    input   - void
"    returns - void
function! CheckConversions()
   let column = col('.')
   let wordBeforeCursor = matchstr(getline('.'), '\c\<[0-9a-fx]\+\.\=\x\+\%'.column.'c')
   let wordAfterCursor = matchstr(getline('.'), '\c\%'.(column-1).'c[0-9a-fx]\+\.\=\x\+')
   if (strlen(wordAfterCursor) == 0)
      " None of the word is after the cursor
      let base = FindBase(wordBeforeCursor)
      let rawNumber = StripLeader(wordBeforeCursor, base)
      let beginingColumn = col('.') - strlen(wordBeforeCursor)

      if (base != 0 && IsNumber(rawNumber, base) && rawNumber =~ '\S\{2,}')
         call ListConversions(base, rawNumber, beginingColumn, wordBeforeCursor)
      endif
   else
      " Some of the number resides after the cursor... completing not possible

      " This section used to pull up the conversions in a command line print
      " out as you typed, but it wasn't very useful and had some annoying bugs,
      " so I've removed it for now.
   endif
endfunction

" ListConversions <><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><>
"  brief: Calls up a completion popup with conversion suggestions.
"    input   - base: [int] base The base that the original number is in
"              rawNumber: [int] The number that needs to be converted
"              column: [int] The starting point of the number we're completing
"              origWord: [string] The text that we're completing against
"    returns - void
function! ListConversions(base, rawNumber, column, origWord)
   let label = '   ' . {2: 'BIN', 8: 'OCT', 10: 'DEC', 16: 'HEX'}[a:base]
   let decNumber = BaseConversion(a:rawNumber, a:base, 10)
   call complete(a:column, [
        \ {'word':a:origWord, 'menu':label},
        \ {'word':BaseConversion(a:rawNumber, a:base,  2, 1), 'menu': '   BIN'},
        \ {'word':BaseConversion(a:rawNumber, a:base,  8, 1), 'menu': '   OCT'},
        \ {'word':BaseConversion(a:rawNumber, a:base, 10, 1), 'menu': '   DEC'},
        \ {'word':BaseConversion(a:rawNumber, a:base, 16, 1), 'menu': '   HEX'},
        \ (decNumber > 31 && decNumber < 127)?
        \ {'word':nr2char(BaseConversion(a:rawNumber, a:base, 10, 1)), 'menu': ' ASCII'} : {}])
endfunction

" HexConverter ><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><>
"  brief: Brings up a command line print out of conversion suggestions.
"    input   - wordUnderCursor: [string] The number to try to convert
"              optional: [bool] If present won't shift window
"    returns - void
function! HexConverter(wordUnderCursor, ...)
   if !exists('t:inConvertMode')
      let t:inConvertMode = 0
   endif
   if !exists('t:lastCheck')
      let t:lastCheck = ''
   endif
   let base = FindBase(a:wordUnderCursor)
   let rawNumber = StripLeader(a:wordUnderCursor, base)
   if (strlen(a:wordUnderCursor) > 0)
      let N = strlen(BaseConversion(rawNumber, base, 2, 1))
   endif
   let shift = 0
   if (a:0 > 0)
      let winPercent = (winline()*100)/winheight(0)
      if (winPercent <= 10)
         let shift = 0
      elseif (winPercent < 36)
         let shift = 1
      elseif (winPercent < 65)
         let shift = 2
      elseif (winPercent < 89)
         let shift = 3
      else
         let shift = 4
      endif
   endif

   if (base != 0 && IsNumber(rawNumber, base) && a:wordUnderCursor =~ '\S\{2,}' && (a:0 || (t:lastCheck != a:wordUnderCursor)) && strlen(a:wordUnderCursor) > 0)
      if (!t:inConvertMode)
         set cmdheight=5
         if (shift)
            execute 'call feedkeys("\<C-o>'.shift.'\<C-y>")'
         endif
         let t:inConvertMode = 1
      endif
      exe "echo '  BIN: ' . printf('%".N."s', BaseConversion(rawNumber, base, 2, 1)) .'\n'." .
          \    "'  OCT: ' . printf('%".N."s', BaseConversion(rawNumber, base, 8, 1)) .'\n'." .
          \    "'  DEC: ' . printf('%".N."s', BaseConversion(rawNumber, base, 10, 1)).'\n'." .
          \    "'  HEX: ' . printf('%".N."s', BaseConversion(rawNumber, base, 16, 1))"
   else
      if (t:inConvertMode)
         set cmdheight=1
         if (shift)
            execute 'call feedkeys("\<C-o>'.shift.'\<C-e>")'
         endif
         let t:inConvertMode = 0
      endif
   endif
   if (t:lastCheck != a:wordUnderCursor)
      let t:lastCheck = a:wordUnderCursor
   else
      let t:lastCheck = ''
   endif
endfunction

" FindeBase ><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><>
"  brief: Does it's best to guess the base of the number.
"    input   - number: [string] The number from which to determine the base
"    returns - [int] The base number (2 for binary, 10 for decimal, etc...)
function! FindBase(number)
   if     (a:number[0:1] == '0b' && IsNumber(a:number[2:], 2))
      return 2
   elseif (a:number[0:1] =~ '0[0-7]' && IsNumber(a:number[2:], 8))
      return 8
   elseif (a:number[0:1] =~ '0x\c' && IsNumber(a:number[2:], 16))
      return 16
   elseif (IsNumber(a:number, 10))
      return 10
   elseif (IsNumber(a:number, 16))
      return 16
   else
      return 0
   endif
endfunction

" IsNumber ><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><>
"  brief: Looks at all the digits of a given number and determines if it is a
"            valid number for the given base.
"    input   - number: [string] The number to check
"              base: [int] The base to check for
"    returns - [bool] True if valid, false if invalid
function! IsNumber(number, base)
   let digitList = {2: '[01.]', 8: '[0-7.]', 10: '[0-9.]', 16: '[0-9a-f.]'}
   for char in split(a:number, '\zs')
      if char !~ digitList[a:base]
         return 0
      endif
   endfor
   return 1
endfunction

" BaseConversion ><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><>
"  brief: Convert any base [2-16] to any other base [2-16].
"    input   - num: [string] A string containing the number to convert
"              inbase: [int] The base of the number to convert
"              outbase: [int] The desired base
"              optional: [bool] Whether or not to prepend a base leader (i.e. 0x)
"    returns - void
function! BaseConversion(num, inBase, outBase, ...)
   let splitNum = split(a:num, '\.')
   let decNum = Base2Dec(splitNum[0], a:inBase)
   let decimalPrecision = 0
   if (len(splitNum) > 1)
      let decNum .= '.'.Frac2Dec(splitNum[1], a:inBase)
      let decimalPrecision = len(splitNum[1])
   endif
   if     (a:outBase == 2)
      return (a:0)?"0b".Decimal2Base(decNum, 2, decimalPrecision):Decimal2Base(decNum, 2, decimalPrecision)
   elseif (a:outBase == 8)
      return (a:0)?"0" .Decimal2Base(decNum, 8, decimalPrecision):Decimal2Base(decNum, 8, decimalPrecision)
   elseif (a:outBase == 10)
      return Decimal2Base(decNum, 10, decimalPrecision)
   elseif (a:outBase == 16)
      return (a:0)?"0x".Decimal2Base(decNum, 16, decimalPrecision):Decimal2Base(decNum, 16, decimalPrecision)
   else
      return Decimal2Base(decNum, a:outBase, decimalPrecision)
   endif
endfunction!

" Decimal2Base ><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><>
"  brief: Convert any decimal float to any base = [2-16]
"   input   - number: [string] String representation of decimal float to convert
"             base: [int] Desired base
"             decimalPrecision: [int] If number has a radix portion, how many
"                               places after the radix point before rounding
"   returns - [string] String representation of base [base] number
function! Decimal2Base(number, base, decimalPrecision)
   let splitNum = split(a:number, '\.')
   let result = 0
   if (len(splitNum) > 1)
      let precision = (a:decimalPrecision > 4) ? (a:decimalPrecision) : 4
      let result = Frac2Base('.'.splitNum[1], a:base, a:decimalPrecision)
      if (result == 1)
         let result = Dec2Base(splitNum[0]+1, a:base)
      else
         let result = Dec2Base(splitNum[0], a:base) . result
      endif
      return substitute(result, '\..*\zs0\+$', '', '')
   else
      return Dec2Base(splitNum[0], a:base)
   endif
endfunction

" Base2Dec ><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><>
"  brief: Convert any base [2-16] to decimal.
"    input   - number: [string] String representation of an int to convert
"              base: [int] Base of input number
"    returns - [int] Number converted to decimal
function! Base2Dec(number, base)
   let hexdig='0123456789abcdef'
   let result = 0
   let pos = 0
   let len = strlen(a:number)
   while pos < len
      let x = strpart(a:number, pos, 1)
      let d = match(hexdig, x.'\c')
      let result = result * a:base + d
      let pos = pos + 1
   endwhile
   return result
endfunction

" Dec2Base ><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><>
"  brief: Convert decimal integer to any base [2-16]
"    input   - number: [string] String representation of a decimal number to
"                      convert
"              base: [int] Desired base
"    returns - [int] Number converted to [base]
function! Dec2Base(number, base)
   let nr = str2float(a:number)
   let result = ''
   while (nr >= 1)
      let intresstr = printf("%f", nr / a:base)
      let intresstr = substitute(intresstr, '\.\d\+$', '', '')
      let intres = str2float(intresstr)
      let modres = float2nr(nr - intres * a:base)
      let result = printf("%x",modres) . result
      let nr = intres
   endwhile
   if (result == '')
      let result = '0'
   endif
   return toupper(result)
endfunction

" Frac2Base <><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><>
"  brief: Takes a decimal float less than one (a fraction) and converts it to a
"            different base [2-16]
"    input   - number: [string] String representation of a fraction (i.e. .25)
"              base: [int] Desired base
"              precision: [int] How many digits to compute before rounding
"                 (min 4)
"    returns - [string] String representation of fraction converted to new [base]
function! Frac2Base(number, base, precision)
   let hexdig='0123456789ABCDEF'
   let fraction = str2float(a:number)
   let loopCounter = 0
   let result = '.'
   let remainder = fraction
   while (remainder > 0 && ((loopCounter < a:precision) || (loopCounter < 4)))
      let multiplied = remainder * a:base
      let digit = float2nr(multiplied)
      let remainder = multiplied - digit
      let result .= hexdig[digit]
      let loopCounter += 1
   endwhile
   if (remainder != 0)
      let multiplied = remainder * a:base
      let digit = float2nr(multiplied)
      if (digit >= (a:base+1)/2)
         "Need to round
         let index = match(hexdig, result[loopCounter])+1
         let numOfTurnovers = 0
         let fullTurnover = 0
         while (index >= a:base)
            let result = result[:-2]
            let numOfTurnovers += 1
            if (numOfTurnovers == loopCounter)
               let fullTurnover = 1
               break
            endif
            let index = match(hexdig, result[loopCounter-numOfTurnovers])+1
         endwhile
         if (!fullTurnover)
            let result = result[:loopCounter-numOfTurnovers-1] . hexdig[index]
         else
            let result = 1
         endif
      endif
   endif
   return result
endfunction

" Frac2Dec ><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><>
"  brief: Takes a float less than one (a fraction) in any base [2-16] and
"            converts it to a decimal fraction.
"    input   - number: [string] String representation of fraction (i.e. .25)
"              base: [int] Base of the input number [2-16]
"    returns - [string] String representation of decimal fraction
function! Frac2Dec(number, base)
   if (a:base >= 2 && a:base <= 16)
      let hexdig='0123456789abcdef'
      let result = 0.0
      let value  = 0.0
      let pos = 0
      let len = strlen(a:number)
      while pos < len
         let digit = a:number[pos]
         let value = match(hexdig, digit.'\c')
         let result += (value / pow(a:base, (pos+1.0)))
         let pos += 1
      endwhile
      let stringResult = substitute(printf('%.16f', result)[2:], '0\+$', '', '')
      return stringResult
   else
      return 0
   endif
endfunction

" StripLeader <><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><>
"  brief: Takes a string representation of a number and removes any header that
"            isn't a valid digit. (i.e. 0x for hex)
"    input   - number: [string] String representation of the number to strip
"              base: [int] Base of input number
"    returns - [string] String representation of the number without the header
function! StripLeader(number, base)
   if (a:base == 2)
      return substitute(a:number, '^0b\c', '', '')
   elseif (a:base == 8)
      return substitute(a:number, '^0', '', '')
   elseif (a:base == 10)
      return a:number
   elseif (a:base == 16)
      return substitute(a:number, '^0x\c', '', '')
   else
      return 0
   endif
endfunction

" PrintASCIIChart <><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><>
"  brief: Prints a static chart of ASCII values so I can stop looking it up
"    input   - void
"    returns - void
function! PrintASCIIChart()
   echo "| DEC HEX OCT CHR | DEC HEX OCT CHR | DEC HEX OCT CHR | \n"
   let start = 32
   while (start <= 63)
      for i in [0, 1, 2]
         let number = start + (i * 32)
         echon "| "
         echon repeat(" ", 3-len(number))
         echon number
         echon "  "
         echohl PREPROC
         echon BaseConversion(number, 10, 16) . " "
         echohl NONE
         let oct = BaseConversion(number, 10, 8)
         echon repeat(" ", 3-len(oct))
         echon oct . "  "
         echohl PREPROC
         echon nr2char(number)
         echohl NONE
         echon "  "
      endfor
      echon "|\n"
      let start += 1
   endwhile
endfunction
"<< End of base Conversion plugin <><><><><><><><><><><><><><><><><><><><><><><>

Answer 1

I'll report areas of improvements as I find them on-the-fly. There is nothing much to say -- except I remember there was a plugin on this topic published a decade ago may be.

Plugin architecture

Guards

If this is meant to be a plugin, it's best to follow the naming convention for the anti-inclusion guard, i.e. it should be named g:loaded_{pluginname}. On the subject I've gone 2 step furthers : all my plugins have a s:k_version variable that stores the version number, and the following anti-inclusion guard:

" Avoid global reinclusion {{{1
let s:k_version = '1.2'
if &cp || (exists("g:loaded_baseconversion")
      \ && (g:loaded_baseconversion >= s:k_version)
      \ && !exists('g:force_reload_baseconversion'))
  finish
endif
let g:loaded_baseconversion = s:k_version
let s:cpo_save=&cpo
set cpo&vim
...code...
" }}}1
let &cpo=s:cpo_save
"=============================================================================
" vim600: set fdm=marker:

The advantage is that when I set g:force_reload_baseconversion to 1, it stays put, and I can happily work on the plugin code and reload it as many times as I wish. Actually I have a :Reload command hidden in my miscellaneous plugins that sets this force variable on.

Functions

It's best to avoid global functions that anybody can access. If they're meant to be exposed to other users, put them into an autoload plugin. It avoids invading the global namespace, and it also brings much better starting times.

Also, it's interesting to annotate functions with the abort clause. This way in case of errors, VimL interpreter won't try to continue to interpret every following Ex command in the function. This point is addressed in google "stylistic" document for VimL.

Licence

State explicitly the licence under which you wish to distribute your code. Right now, unless explicitly authorized nobody can redistribute your plugin. I'm not even sure we are authorized to use it -- I should ask to my colleague with an expertise on the subject. As it has been published on vi.SE, SE licensing policy applies on this very code, but not on the plugin once distributed on github or wherever else.

Internal variables

I see variables like t:lastCheck. Any plugin could decide to use a variable like this one for its own needs (don't hesitate to fix my English ^^'). As it has no prefix to say this variable belongs to Baseconversion plugin, we can't blame this other plugin. Beside, do we really need to save this variable on a tab basis? Couldn't it be a script-local, and thus global, variable?

Performances

The only places I see where there could be issues, is where there are loops and function static constants that could be extracted from function definitions (e.g. digitList could become s:digitList and be defined outside the function definition).

IsNumber

May be a negated pattern would be faster. The only problem I see is that those patterns are not trivial, and quite unmaintenable when not initiated to them.

But here, positive logic would be even more simple and efficient: return a:number =~ '^'.s:digitList[a:base].'*$'

BaseConversion

I suspect you could use a dictionary in order to avoid the switch and to duplicate code. I've no idea of the impact on performances.

let s:k_prefixes = { 2: '0b', 8: '0', 16: '0x' }
function! base_conversion#convert(num, inBase, outBase, ...) abort
   ...
   let prefix = get(s:k_prefixes, a:outBase, '')
   return prefix . base_conversion#Decimal2Base(decNum, 16, decimalPrecision)

Base2Dec

If this function has for precondition that input is a number in the expressed base, state it more clearly in the documentation of the function. I even have a series of lh#assert#*() functions on Design By contract in VimL topic, but unlike C, C++ and Python assertions they'll impact performances.

I wouldn't have used strpart, but simply a:number[pos] out of habit. However, I've no idea of its impact on performances.

misc ideas

When performances matters, I try to avoid match() and prefer stridx(). However, because of case sensitivity, it may not be that simple here.

Most loops are quite usual and hard to express otherwise in VimL. I'm not inspired to propose any possible improvement.

I would have tried to avoid pow() out of reflex from the other languages I'm working with. However, I cannot really guarantee that manually hardcoding exponentiation to 2^n would improve anything, nor anything significantly, once interpreted by Vim.

l:variables

Note that some may point out that local variables could be prefixed. google stylistic document even strongly encourage to prefix all local variables. I disagree completely on this topic. The problem is exactly the same with other languages: a same identifier may designate a global variable, a local variable, a parameter, a static variable (localized globals in C), an attribute, etc. While I, indeed, follow a different naming convention for globals, I don't follow any for local variables, except leaving them alone.

There are indeed a few cases where a difference may be observed: In order to store a Funref in a simple variable (and not in behind a dictionary key), the variable name shall start with a capital letter. In those situations, I did need to prefix my variable with l: to avoid confusion with global functions. Shall we always prefix all variables for consistency reasons with those rare case? Today, I don't buy it, l:variables make my eyes bleed. This is just my personal opinion: as far as I'm concerned, there is no right nor wrong way on this subject: l: fixes issues with funcref variables, but we loose the possibility to run :echo/:exe getline('.').