TotalCross / totalcross

TotalCross is a Software Development Kit that helps cross platform application development. Currently supported platforms are: Windows, Wince, Android, iOS, Linux and Linux ARM for embedded systems.
https://www.totalcross.com
GNU Lesser General Public License v2.1
220 stars 40 forks source link

4D method in Convert class #131

Open jeffque opened 4 years ago

jeffque commented 4 years ago

4D method in Convert class

AFAIK, 4D methods has been deprecated and should have been removed. However, investigating GitLab's TotalCross/totalcross#583, I found a 4D method in Convert.java here and here

Describe the bug

Code style suggests that 4D methods and classes should be abolished

Additional context

The Convert.toString4D(long, int) has an almost identical code to Convert.toString(long, int). The exception been thattoString(long, int)adds a validation in it's third condition,&&useNative, that doesn't exists intoString4D(long, int)`.

newLauncherInstance() has some misterious comments about "applets" not initializing correctly, so it was necessary as some kind of backup tool. Also, besides judging if should create or not a new Launcher instance, it also calls to instance method Launcher.fillSettings() too. I dunno why there is a reason to call this method always, why it isn't a new Launcher() instruction, nor why it is wrote out from native code (newLauncherInstance4D() is an empty method).

flsobral commented 4 years ago

The Convert.toString4D(long, int) has an almost identical code to Convert.toString(long, int). The exception been thattoString(long, int)adds a validation in it's third condition,&&useNative, that doesn't exists intoString4D(long, int)`.

You're right, the 4D method and the useNative flag can both be removed. 👍

newLauncherInstance() has some misterious comments about "applets" not initializing correctly, so it was necessary as some kind of backup tool. Also, besides judging if should create or not a new Launcher instance, it also calls to instance method Launcher.fillSettings() too. I dunno why there is a reason to call this method always, why it isn't a new Launcher() instruction, nor why it is wrote out from native code (newLauncherInstance4D() is an empty method).

Launcher is only available when running on the JDK. I guess we could make an empty native method to remove the 4D, but that would be lazy. I guess I left it that way to annoy me into a better solution in the future, possibly rewriting the Launcher and/or Convert. Creating an empty native method feels like sweeping it under the rug for me, I'd rather keep it that way.

jeffque commented 4 years ago

I guess I left it that way to annoy me into a better solution in the future

Well, I think that I called the future Fábio now 🤭

jeffque commented 4 years ago

Maybe instead of an empty native a simple check if it is in a Java env should work as intended.

jeffque commented 4 years ago

To avoid the TotalCross/totalcross#583 issue, the Convert.toString(long) code could be replaced with a a call to Long.toString(long), as the Conert.toString(int) code is a call to Integer.toStirng(int)