Using Modulo in my program

[John Vorona]

Hello,

I am writing a program which basically creates a 3 letter lock, say CAT for example, then the lock can be opened, closed, reset, and letters can be set individually. I am using only uppercase letters A - Z, (65 - 90 in unicode). The trick to this is to wrap the letters in a circle. Meaning after Z comes A, and before A comes Z. Just like a circular lock would have in real life. A lock can also be set with a tolerance, say the tolerance is 1, the lock combo of CAT can be opened with CAT, CBT, BAT, DAT etc.... the tolerance doesn't have a limit so it could be 5. Another example, a combination is ZAZ, and the tolerance is 5, this can be opened with ZAZ, AZA, ZAD, etc.. you get the point. I am having trouble with the wrapping of the code, I got the tolerance to work for most of the combos, but I am getting that this is the wrong combo when I try, the combo is ZAZ, the tolerance is 5, and ZAD will not open, which it should. Here is my method which determines whether the lock will open..

public void pull()
	{
		isOpen = (((Math.abs(letter1 - input1) % CHAR_RANGE) 
			     + (Math.abs(letter2 - input2) % CHAR_RANGE)
			     + (Math.abs(letter3 - input3) % CHAR_RANGE)) <= tolerance);
	}

where letterx is the letter which was used to set the lock. e.g. CAT, C = letter1, A = letter2, T = letter3. (The real lock combo)
where inputx is the letter which was used as an attempt by the user to open the lock. (The combo attempted to open the lock)
where CHAR_RANGE is 'Z' - 'A' which is 90 - 65 = 25.
and tolerance is the tolerance as explained in the paragraph above.

Anyways i have been over my head and tried nearly everything, I am still fairly new so if anyone can guide me in the right direction that would be great.

[Steve Luke]

I haven't run your code but it looks like you are doing too many things in one line. Try to break it all into smaller bits and log the results of each but so you know exactly what is happening.

[John Vorona]

I know that this can be done with a bunch of if else and even more nested if else statements, although editing that code will be chaos, I was told it could be done using modulus and I am trying to keep it simple.

[John Vorona]

Here is my code for that whole class

/*********************************************************************************
Implement a combination lock class.
A combination lock has a dial with 26 positions (A-Z).
Lock is created with a three letter combination.
To open lock, dial must be set three times.
If it is set to the correct combination, the lock will open.
To open the lock you must pull on it, it will open if combination was right.
While lock is open, turning the dial has no effect.
Say combination is CAT, XBCNSHCAT will open the lock. JAHSDJHASDBAT will not.
A lock can be created with a tolerance, meaning combination = CAT, tolerance = 1,
This lock can be opened with CAT, DAT, CAS, etc..
A lock can be created with a tolerance of 3, combination = XYZ, tolerance = 3,
This lock can be opened with XYZ, ZZZ, XYB, XYC, ABXZB, etc...
**********************************************************************************
Methods to create:
set - Sets the current dial the the given character.
pull - Attempts to open the lock, will open if combination is correct.
isOpen - if open, true. if closed, false.
close - closes the lock, resetting the dials.
Unicode range - {A:65 - Z:90}
Make A wrap to Z, A = 65, Z = 90,
so if an integer = 91, goes back to A.
**********************************************************************************/

public class Lock{
	
	private final int CHAR_A_VALUE = 'A';
	private final int CHAR_Z_VALUE = 'Z';
	private final int CHAR_RANGE = CHAR_Z_VALUE - CHAR_A_VALUE;
	private int letter1, letter2, letter3;
	private int tolerance;
	private int input1, input2, input3;
	private boolean isOpen;
	
	
	
	public Lock(char input1, char input2, char input3)
	{
		letter1 = input1;
		letter2 = input2;
		letter3 = input3;
		tolerance = 0;
		isOpen = false;
	}

	public Lock(char input1, char input2, char input3, int tolerance)
	{
		letter1 = input1;
		letter2 = input2;
		letter3 = input3;
		this.tolerance = tolerance;
		isOpen = false;
	}

	public void set(char letter)
	{
		input1 = input2;
		input2 = input3;
		input3 = letter;
	}

	public void pull()
	{
		isOpen = (((Math.abs(letter1 - input1) % CHAR_RANGE) 
			     + (Math.abs(letter2 - input2) % CHAR_RANGE)
			     + (Math.abs(letter3 - input3) % CHAR_RANGE)) <= tolerance);
	}

	public boolean isOpen()
	{
		return isOpen;
	}
	
	public void close()
	{
		letter1 = 'A';
		letter2 = 'A';
		letter3 = 'A';
		isOpen = false;
	}
}

[Steve Luke]

Nazar Buko wrote:I know that this can be done with a bunch of if else and even more nested if else statements, although editing that code will be chaos, I was told it could be done using modulus and I am trying to keep it simple.

Simple is good, for sure, but how simple is that? So "simple" you can't trouble shoot it, which really makes it harder to get right than it needs to be. The code in the first post has 7 operations: three subtractions (with an absolute value taken), three modulo operations, and a comparison. If I were writing this code I would break each operation onto its own line and output the result of each one, so I could see what was happening.

And in doing so, I might realize that 6 of those operations are actually the same 2 operations repeated 3 times with different data - a perfect opportunity to isolate the operations into a method and reduce the overall complexity, lines of code, and increase the maintainability of the code.

[John Vorona]

Here is my interpretation of a simplified equation.

public void pull()
	{
		int tolerance1 = Math.abs(letter1 - input1) % CHAR_RANGE;
		int tolerance2 = Math.abs(letter2 - input3) % CHAR_RANGE;
		int tolerance3 = Math.abs(letter3 - input3) % CHAR_RANGE;
		isOpen = (tolerance1 + tolerance2 + tolerance3) <= tolerance;
         }

I agree it is much simpler to see what's happening although the problem persists.

[Raymond Tong]

For the test case you have mentioned, with lock ZAZ, given ZAD should unlock.
To simplify it, with lock "Z", given "D" should unlock.

Your code to calculate the diff is outside tolerance of "5".
Hint: you mentioned about circular lock

[Steve Luke]

Nazar Buko wrote:Here is my interpretation of a simplified equation.
...
I agree it is much simpler to see what's happening although the problem persists.

So now, either println the result of each step or put break points at each line and run the code through a debugger. What do you see happening? Do you see why? When you see the problem come up with a couple simple ways to make it work - reorder things or whatever. Once you have a simple solution, then generalize it so it would work on all cases.

And to make testing easier, why not remove that one line that is repeated 3 times into its own method, and run tests on it with different values for the letter and input. That way you can build a really comprehensive test without lots of overhead.

    private int getCharacterOffset(char key, char input)
    {
        int tolerance = Math.abs(key - input) % CHAR_RANGE;
        System.out.printf("Key: %s  Input: %s  Result: %d%n", key, input, tolerance);
        return tolerance;
    }

    public static void main(String[] args)
    {
        Lock lock = new Lock('A', 'B', 'C');  // won't actually use this combo
        
        //Test start of alphabet
        char key = 'A';
        for (char input = 'A'; input <= 'Z'; input++)
        {
            lock.getCharacterOffset(key, input);
        }

        //Test end of alphabet
        key = 'Z';
        for (char input = 'A'; input <= 'Z'; input++)
        {
            lock.getCharacterOffset(key, input);
        }
    }

Check the output and see what you get for each combination. Once you get the expected tolerance, you can modify your pull() method to:

    public pull()
    {
        int offset1 = getCharacterOffset(letter1, input1);
        int offset2 = getCharacterOffset(letter2, input2);
        int offset3 = getCharacterOffset(letter3, input3);
        isOpen = (offset1 + offset2 + offset3) <= tolerance;
    }

[Robin Haden]

Hi, I have a similar assignment and I'm trying to figure out how to implement the 'wrap'. Just wondering, what is the significance of doing "(((Math.abs(letter1 - input1) % CHAR_RANGE)"?

[John Vorona]

Figured it out, my algorithm was wrong.

Don't use modulo, to wrap, find the midpoint of what interval you are trying to wrap and make 2 cases for it, if it is above that midpoint, subtract whatever number you have from your interval. in my case my interval was the alphabet, and i had to do 26 - x.