#44 AQM (AlpineQuest Map) file format loader for MOBAC (patch review)

[ph-t]

Hi r_x,

I have written some piece of code to read AQM file format from MOBAC.
I would be happy to share this code and contribute to MOBAC.
This feature couble be usefull to convert AQM to other offline GPS file format or simply to resize large AQM file (10-20 Go).

Could you please tell me whether the quality if good enough to be commit on SVN repo? Logging should be done and memory usage is clearly not optimized... Any feedback or comment appreciated?

Kind regards
Pierre

PS : please note that apache commons-collections4-4.4.jar dependency is required

[r_x]

Ok, your code looks nice and it is well structured, however I noticed some potential problems:

1. Code formatting: Please always use the Eclipse auto-formatting before saving files. Especially the indentation was a mixture of spaces and tabs. Eclipse can do this automatical for you. (Source -> Format or simple Crl+Shift+F)
2. At some points within your code you use

    try {
            ...
    } catch (Exception e) {
            e.printStackTrace();
    }

Please do not do this. Never capture an Exception without handling it. Narow it down to the exact Exception you expect and then handle it (throw e.g. a new RunTimeException or log it or something else but never use e.printStackTrace() as it does not use the logging system.

1. Ho large is a typical AQM map? Because I notices that you fully load the AQM file to memory. Also this limits the file size to 2GB (max array size). Therefore at the moment your implementation requires up to 4GB of memory as all tiles are stored in-memory. (2GB for the file + 2GG for all the tiles). This is very memory inefficient. I would prefer an on-demand loading.

2. When using streams please use try-with-resource blocks whenever possible: https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html

3. Please make sure that all files have the proper MOBAC copyright block at the beginning of the file.

4. When initializing classes with geric parameter you only have to set the parameter on the left side:

List<String> ret = new ArrayList<String>(); // String not necessary on right side
List<String> ret = new ArrayList<>();

For new code I prefer the short variant.

[ph-t]

Hi,

Thank you for all your feedback. I will try to improve all what you mention.
It will take time. Hope to get back to you soon with a better
implementation.

Thank you. Have a good week-end.

Pierre

De : r_x [mailto:r_x@users.sourceforge.net]
Envoyé : vendredi 27 septembre 2019 09:10
À : [mobac:patches]
Objet : [mobac:patches] #44 AQM (AlpineQuest Map) file format loader for
MOBAC (patch review)

Ok, your code looks nice and it is well structured, however I noticed some
potential problems:

1. Code formatting: Please always use the Eclipse auto-formatting
   before saving files. Especially the indentation was a mixture of spaces and
   tabs. Eclipse can do this automatical for you. (Source -> Format or simple
   Crl+Shift+F)

2. At some points within your code you use

   try {
   ...
   } catch (Exception e) {
   e.printStackTrace();
   }

Please do not do this. Never capture an Exception without handling it. Narow
it down to the exact Exception you expect and then handle it (throw e.g. a
new RunTimeException or log it or something else but never use
e.printStackTrace() as it does not use the logging system.

1. Ho large is a typical AQM map? Because I notices that you fully load
   the AQM file to memory. Also this limits the file size to 2GB (max array
   size). Therefore at the moment your implementation requires up to 4GB of
   memory as all tiles are stored in-memory. (2GB for the file + 2GG for all
   the tiles). This is very memory inefficient. I would prefer an on-demand
   loading.

2. When using streams please use try-with-resource blocks whenever
   possible:
   https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClos
   e.html

3. Please make sure that all files have the proper MOBAC copyright
   block at the beginning of the file.

4. When initializing classes with geric parameter you only have to set
   the parameter on the left side:

List<string> ret = new ArrayList<string>(); // String not necessary on right
side
List<string> ret = new ArrayList<>();</string></string></string>

For new code I prefer the short variant.

---

[patches:#44] https://sourceforge.net/p/mobac/patches/44/ AQM
(AlpineQuest Map) file format loader for MOBAC (patch review)

Status: open
Group: Enhancement
Created: Thu Sep 26, 2019 07:29 PM UTC by ph-t
Last Updated: Thu Sep 26, 2019 07:29 PM UTC
Owner: r_x
Attachments:

• MOBAC.AQM.loader.0.1.patch
  https://sourceforge.net/p/mobac/patches/44/attachment/MOBAC.AQM.loader.0.1. patch (47.0 kB; application/octet-stream)
• commons-collections4-4.4.jar
  https://sourceforge.net/p/mobac/patches/44/attachment/commons-collections4- 4.4.jar (751.9 kB; application/octet-stream)

Hi r_x,

I have written some piece of code to read AQM file format from MOBAC.
I would be happy to share this code and contribute to MOBAC.
This feature couble be usefull to convert AQM to other offline GPS file
format or simply to resize large AQM file (10-20 Go).

Could you please tell me whether the quality if good enough to be commit on
SVN repo? Logging should be done and memory usage is clearly not
optimized... Any feedback or comment appreciated?

Kind regards
Pierre

PS : please note that apache commons-collections4-4.4.jar dependency is
required

---

Sent from sourceforge.net because you indicated interest in
https://sourceforge.net/p/mobac/patches/44/

To unsubscribe from further messages, please visit
https://sourceforge.net/auth/subscriptions/

[ph-t]

Hi r_x,

Please find enclosed a new patch that includes most of your recommendations. This implementation supports large AQM file without running out of memory. I tested it on a 2Go AQM file and it works well and fast. Feel free to integrate this patch if the quality meet the expected requierments.

Best regards

Pierre

[r_x]

Hi Pierre,

thanks for the patch it now looks quite nice. I made some improvements to it and commited it into the MOBAC SVN. In my opinions there are still some minor edits necessary - please see below.

I had to made some changes as the extra methods for MapSource were not acceptable as this would break the map pack compatibility for older map packs. Instead I moved the two added methods to an special interface named MapSourceInitialDisplayPosition which is implemented by CustomLocalAqmMapSource.

I changed the code parts that require the apache commons collections library. Such a large library for just a variant of a map...
While modifying your code I encountered a strange part in AqmMap.java which should from my understanding always throw a NullPointerException if MOBAC ever uses this specific if branch. I marked the line with // TODO: ERROR. Please have a look at it.

Also some minor improvements should be done in the field of exception handling. Logging exceptions is nice, however this is usually invisible to the user. Better throw an Exception instead in case the error is not recoverable or ignorable. Or show a error dialog.

Robert

[r_x]

No response by the patch author. The code was buggy like hell, so I had to fix it myself :(