souramoo / unapkm

APKM file decryptor
Apache License 2.0
174 stars 19 forks source link

Request: apply some adjustments and suggestions from the IDE #3

Closed AndroidDeveloperLB closed 4 years ago

AndroidDeveloperLB commented 4 years ago
  1. This :

            byte magic = getBytes(i, 1)[0];
            if (magic > 0xff) {
                throw new Exception("wrong version magic oops");
            }

    A byte can't be larger... The condition is useless.

  2. This:

            while (bytesRead != -1) {
                bytesRead = i.read(cipherChunk);
                if (bytesRead == -1) break;

No reason to check twice. The condition is enough. You can use while(true) instead.

  1. This:
                for (int j = 0; j < cipherChunk.length; j++) {
                    cipherChunk[j] = 0;
                }

You can use this instead:

Arrays.fill(cipherChunk, (byte) 0);
  1. This:
            if (fos != null) {
                fos.close();
            }

It's already not null. Also, it's recommended to close it in the finally-block, or better: using "try(...)" on it, to auto-close it, as if it's in the finally-block.

  1. This:
    public UnApkm(String filein, String fileout) {

On Java, functions are supposed to start with lower-case letter. Meaning:

    public unApkm(String filein, String fileout) {
  1. You can use Kotlin instead :)
souramoo commented 4 years ago
  1. Removed in the latest commit
  2. Refactored in the latest commit, the condition in that bit is checked before actually reading any data which is why it was there but have switched it to while ( (bytesRead = i.read(...)) != -1) instead
  3. Fixed!
  4. Fixed also
  5. Ah but it's a constructor and classes start with a capital letter ;)
  6. Pls no
AndroidDeveloperLB commented 4 years ago
  1. OK
  2. Nice
  3. Good
  4. Great
  5. That's not a recommended thing to do. CTORs should be very minimal, not handling files, let alone having to deal with tons of calculations and take a long time to be created. There are various alternatives to this. Examples: a. Singleton, with a function to extract. Or let all functions be static, as you don't expect anyone to extend from it. b. A normal class, which has those parameters but requires a call to an additional function to do the extraction. c. I'm sure there are more solutions but those are the most common that I know.

But in any case, please don't put this whole code in a CTOR...

  1. Haha ok.
souramoo commented 4 years ago
  1. Moved into a static function :) https://github.com/souramoo/unapkm/commit/7b59cc3bcc12dffabb923acb870982d666b5fc1a
AndroidDeveloperLB commented 4 years ago

Thank you.

The CTOR is useless now though. You can make it private to tell whoever that use it : "this is not intended to be used this way" .

And you can apply the suggestion of renaming to IOException ignored and remove the casting of long opsLimit = (long) byteToInt(getBytes(i, 8));

I also don't see that you use getHex function. What was it used for? To investigate how it works?

souramoo commented 4 years ago

Fixed to all three of the above points!

Yep, I was using it mostly to debug the outputs of the crypto byte arrays when putting the whole thing together, have left it in just in case it's useful for anyone else (and me later if I need to debug it again).

AndroidDeveloperLB commented 4 years ago

I see. Now it has those stuff though:

        FileOutputStream fos = null;

Not used in decryptFile.

        byte[] data = new byte[(int) num];

"num" is already an int.

copyInputStreamToFile - I think you could use try-with, like here: https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html Meaning maybe like this:

        try (OutputStream out = new FileOutputStream(file)) {
          // no need to close it. It auto-closes in "finally" for you, even if "finally" isn't written by you
        } catch (Exception e) {
            e.printStackTrace();
        }
souramoo commented 4 years ago

Thanks, have fixed the above!

AndroidDeveloperLB commented 4 years ago

Thank you. If you wish, you can also apply this:

image

And it's a common thing to put constants at the top, at least on Java. On Kotlin sadly it's usually at the bottom :(

souramoo commented 4 years ago

Have done! :)

AndroidDeveloperLB commented 4 years ago

Seems all the nice warnings are gone. Only thing that's left is this one, as you don't really need to initialize it.

int bytesRead = 0;

You could, however, put "final" for each variable, whenever you can. If you used Kotlin, the IDE would have helped you on this. Back in the days of Eclipse, it had such a suggestion for Java, and even did it for you each time you've formatted the code (optional setting).