Converting a number into Hungarian text

I'd like to convert some integer number from a cell between 0 and 1,000,000,0000 into Hungarian text. I've found already a solution, a VBA function.

How can be improved this solution?

```
Function num2txthu(cellvalue As Double)
'------------------------------------------------------------------------------
'VBA-function, ami Excel-ben használható.
'Egy cella tartalmát (0 és 1 milliárd közötti egész számot) konvertálja szöveggé.
'
'Virág Imre megoldása alapján, köszönettel! Kissé módosítva, tesztelve.
'(http://www.adatkerteszet.hu/2014/08/osszeg-betuvel-szamok-atirasa-szovegge-keplettel/)
'
'hulloalma@gmail.com
'
'Példák
' A1 A2 =num2txthu(A1)
' ------------ ----------------------------------------------------------------
' 1 egy
' 10 tíz
' 19 tizenkilenc
' 20 húsz
' 25 huszonöt
' 1 000 ezer
' 1 999 ezerkilencszázkilencvenkilenc
' 2 001 kétezer-egy
' 3 016 háromezer-tizenhat
' 47 563 negyvenhétezer-ötszázhatvanhárom
' 100 000 százezer
' 1 100 000 egymillió-egyszázezer
' 7 001 530 hétmillió-egyezer-ötszázharminc
' 7 491 530 hétmillió-négyszázkilencvenegyezer-ötszázharminc
' 7 490 530 hétmillió-négyszázkilencvenezer-ötszázharminc
' 10 000 000 tízmillió
' 999 999 999 kilencszázkilencvenkilencmillió-kilencszázkilencvenkilencezer-kilencszázkilencvenkilenc
'1 000 000 000 egymilliárd
' ------------ ----------------------------------------------------------------
' A1 A2 =CONCATENATE("",UPPER(MID(num2txthu(A1),1,1)),MID(num2txthu(A1),2,200),"")
' ------------ ----------------------------------------------------------------
' 4672 Négyezer-hatszázhetvenkettõ
' 4911 Négyezer-kilencszáztizenegy
'------------------------------------------------------------------------------

Dim v_arr(100)
v_number = cellvalue
v_length = Len(v_number)
v_number_name = ""
v_group_name = ""
v_num2txthu = ""

'It will then reevaluate whenever the

We'll start with variables. Variables not defined -

v_number = cellvalue
v_length = Len(v_number)
v_number_name = ""
v_group_name = ""
v_num2txthu = ""
pos

Variables defined poorly

Dim v_arr(100)

All variables should be defined and given a type. Now for variable names - I have no idea what they are supposed to be. v_arr is an array of the letter "v" (sorry, I'm not being snarky, I promise)? Good variables names are great for readers of code. Let me take a guess at these

Dim v_arr(100) -         Dim numbersForConversion() as Long
v_number = cellvalue -   Dim numberToConvert as Double
v_length = Len(v_number) Dim  numberToConvertLength as string
v_number_name = ""       Dim numberName as string
v_group_name = ""        Dim groupName as string
v_num2txthu = ""         Dim numberAsText as string
pos                      Dim positionInNumber as long

Why is v_arr(100? If you can only work with numbers under 1,000,000,000 - your array only needs to be 10. Better yet Redim numbersForConversion(numberToConvertLength).

Now the code will be a lot easier to read. Well, I can't read Hungarian, so I assume it will be easier to read. If not, pick your name in whatever language you'd like.

Function Modulo(a, b) could be Public Function(byVal numberValue as long, byVal divisor as long)

On the same note Function num2txthu(cellvalue as double) is breaking a lot of conventions. You shouldn't have a digit in a function name - it's just bad practice. There's also no reason, as mentioned above, to name it num2txthu when you could easily name it Public Function ConvertNumberToHungarian(byVal targetNumber as Double.

Code organization - none of the code is indented from the left - you should always start your code indented so labels can be seen easily - which you do have labels. I honestly can't tell where the function ends and the next one begins, are there more than two functions? There's an Exit Function all the way to the left that confuses me.

Comments - I can't really comment because I can't read what they say, but they look to me like they are just saying what's happening instead of why something is happening. Always put comments where you deviate from the obvious way of doing something and it would be difficult to unwrap without spending a lot of time on it.

Speaking of labels, they have pretty poor names too - l_group_100s, l_group_10s:, l_group_1s:, l_group_names:. They could be more descriptive like HundredsPosition, TensPosition or whatever the Hungarian words are for those numbers.

I'm going to try to tackle the logic, but it may take a while.

I really like the use of the Fix() function - I've never actually seen it. Same goes for Val() - but I don't see the reason to use it for an actual number:


The Microsoft Excel VAL function accepts a string as input and returns
the numbers found in that string.

Oh, you're using Mid to get a string and returning it as a number. That seems like a pretty roundabout way of doing that, but I can't think of a better way.

I'd shift the check for 0 to above the check for integer, because 0 will pass the check for integer.

Does this function account for negative numbers? Because the first check - If v_number > 1000000000 Then would pass for -2,000,000,000 and I think the length might get wonky and might overflow or type mismatch in the array.

Might want to check if ABS(v_number) > 1000000000 and then figure out how to use negative numbers. You'd probably only have to change one case if you converted it to positive and made it negative at the end.

I get that Function Modulo is a refactoring of the code, but all it does is v_number mod #### so why break it out? I mean v_number mod ## is shorter than Modulo(v_number, ##) Maybe someone else can help me understand that.

The use of GoSub ... Return is interesting as well. It might actually make more sense to move the labels to their own functions and just use the Public Function(byval # as long) as string to send the digits to get the text. By the looks of it those labels are already refactored to include several types of cases, so breaking them out would make sense. Plus you wouldn't need to carry pos all the way through and back every time.

StackExchange Code Review Q#119168, answer score: 2