Fixing https://github.com/AppleCommander/AppleCommander/issues/18

[Lisias]

Fixing issue #18

"back-end" tested successfully with every readable disk image from Asimov.

"front-end" was tested and no errors detected until the moment with valid images.

Invalid images are silently ignored - intervention points while Warnings to the User can be issued are marked with "FIXME"

[a2geek]

Maybe the DiskCorruptException should be able to capture the corresponding return object? Maybe a second constructor with a corresponding method to get the object from the exception itself. Thinking that would provide a user interface (either the command-line or the GUI) the ability to over-ride the exception -- the user knows there is an error but can still get past it. (This also suggests that where the exception is thrown, the object needs to be captured. With "object" being a list of files or whatever causes this exception.)

[Lisias]

I thought of that.

But I didn't found it would worth the pain because the vast majority of the issues are related to copy protected disk images. I don't remember a single image where the problem was a bad directory structure (what is different to say there was any).

So I considered that perhaps this should be handled punctually by a specific rescue tool.

[Lisias]

Humm... However... Since that hypothetical rescue tool will need to know where the problem is, and since the code that raises the Corrupt Exception already knows what's happened...

Yes, you right. At least the Exception should carry on the information.

I will implement this for sure.

The overriding command, however, should not be implemented in my opinion. A rescue tool is a proper place to do that, I think. And that rescue tool can derive the current implementation and override the functionalities to do what it thinks it must do.

[a2geek]

I think it's best to assume the if will be multi-line. Would you please update this (and the few others) to have a structure like this? Basically add the { and } for the if. Thanks! :-)

if (...) {
    try {
        // code here
    } catch (...) {
        // exception handling here
    }
}

[Lisias]

It's a matter of taste and code compliance, so I will end up doing what you ask. But for while, let me try to explain why I do this way and, perhaps, make you agree with me. :-)

I develop in a lot of languages. Well, not that much - Java, C, C++, Python and Perl.

Yes. Perl. Don't ask. :-D

So I try to normalize the code "look and feel" to minimize the visual differences from similar code wrote in different languages. Looking into Python, where whitespaces have semantic value (#$#@$@%$), I end up with code with LOTS and LOTS of useless indentation, making a lot harder to understand exactly what was going on there.

So, consider:

def some_method(self) -> str:
    with open(blah) as x:
        with codec.something(x) as y:
            with my_own_filer(y) as z:
                try:
                    r = do_something(z)
                except Exception as e:
                    handle_it(e)
                    r = do_something_else()
                finally:
                    house_keeping()
    return r

And then consider this:

def some_method(self) -> str:
    with open(blah) as x, codec.something(x) as y, my_own_filer(y) as z: try:
        r = do_something(z)
    except Exception as e:
        handle_it(e)
        r = do_something_else()
    finally:
        house_keeping()
    return r

(note: this second formatting is illegal in Python - damned whitespaces with semantics - but I kept it nevertheless to demonstrate my point)

The core business of the code (do_something) is evident and clear on the second version. On the fully indented version, I waste some seconds looking for it.

Similar effect happens on C, C++ and Java, and I tend to follow this rule when it happens.

But, again, your project, your coding rules. If this is not enough to reconsider, advise and I will reformat the code.

[a2geek]

I think this might be an opportunity to use a BitSet! (Never had the chance before.) Basically, a 32MB hard-disk will have 65,536 blocks to track. Uncertain how a Map will perform, thinking a BitSet would be better in that case. I think the simple get(blockNumber) and set(blockNumber) will suffice for the needs here.

[Lisias]

I think we will get some (negligible) performance issues here. The HashMap uses a performatic hashing function to minimize the access time to the data. On Integers, the hashing function returns the very value of the Integer.

When using a TreeMap, the data are stored into a Binary Tree (not sure if balanced), what makes the thing very, very fast on large datasets. The Hash Map uses a more linear approach, what's faster for small datasets (faster to insert), but can be slower on using due collisions due the hashing function. Since there's no collisions with Integer, I think this approach is better than TreeMap on this case.

Since directories usually have just a few blocks, the size of the structure will not grow enough to make the linear approach perform badly.

More info here

So it appears to be a win-win for me.

The BitSet has a disadvantage for me: I need to know the size of my data subset. Floppies have a lot fewer blocks than Hard Disks, and GS/OS hard disks can have a lot more blocks that ProDOS ones.

So in order to use BitSet, I have to handle context. I have to know what I'm using at the moment, or then just assume the worst possible scenario - what will be hardly performatic for the most common use case: floppy disk images.

I'm not sure if BitSet will be more performatic than HashMaps, but HashMaps are free of context, and so less prone to errors on the long run.

However... Looking on the problem again, I realized I didn't thought enough on the problem, and just re-implemented a solution I was working on other code (my pet project that was blocked by the issue).

I just don't need a Map! A Set was enough! A HashSet, to tell you the true.

I will fix this for sure. Please refuse Pull Request #19 (or I can just "fix and reissue" the pull request?

[Lisias]

On the other hand... GIving some more thinking on the matter, and then realizing that BitSet can also be resized under demand, I must reconsider. There's a good chance that BitSet is the right tool to do the job!

You know what? I'll go DevOps style. I will make a bunch of tests to see what's better!

I already made the test bed to read all the Asimov disk images, didn't I? :-)

[Lisias]

Of course, someone else already did this research for us!! :-)

On the short run: small range of values are faster handled by HashSet. Very bigger ranges are better handled by BitSet, but the performance is due mainly using a more concise data structure with better chances of fitting into the CPU's cache (or at least is what I was told here).

[a2geek]

Similar to the if statement. (These things have caught me before!)

[Lisias]

These things are not problem to me because they are used in very specific situations. It's a fixed pattern on different languages I use, so ends up easing my life.

See my previous comment about the issue. I will reformat the code, if my argument is not valid for you.

[a2geek]

Hey @Lisias - I like! I did provide some commentary in that files changed tab.
(LOL, and I see there were comments added, which I didn't see at first. ;-) )

[Lisias]

Hi! I came back from the land of the computerless. I'm finishing the fixes as proposed.

[Lisias]

I think I finished the proposed changes - except by the proposal of a "1st aid tool" where disk errors are ignored. I think this should be handed in another issue, as this one aimed to solve the application's crash.

[a2geek]

Looks good! Pulled that image and watched my 32GB of memory get slowly eaten up by the unpatched version. I like the error and the display of the disk (which is empty, but it is technically corrupted!).