Need help to review my code

[tangara goh]

Hi guys,

I would like to have someone review my code.

The objective of the code is to encode a source String with a character that is indicated and the source String will be used as the reference table.  So, A will be 0, B will be 1.

I am now using a hardcoded 1 as an index to generate the rotatedArray - the target array for me to populate the output : BGDKKN VNQKC

static final String source = "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789()*+,-./";
	//static final String target1 = "/ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789()*+,-./";//B is the indicated character
	static final String target = "+,-./ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789()*";
 
	public String obfuscate(String s) {				
		char[] sourceChar = s.toCharArray();//Hello World 
		char[] target = rotatedArray(source);
		System.out.println("rotated array now is " + new String(target));
		int pos = positionToUse(s);
		System.out.println("target " + new String(target));//"/ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789()*+,-." for index = 1
		char[] result = new char[s.length()];
		String offsetChar = null;
		for (int i = 0; i < s.length(); i++) {
			if (s.substring(s.length() - 1) != "") {
				char c = s.charAt(i);
				if (c == ' ') {					
					c = ' ';// Preserve spaces
					result[i]= ' ';
				} else {
					int index = source.indexOf(c);
					char rtn = source.charAt(pos);
					offsetChar = Character.toString(rtn);	//this will return the position of B from the source			
					result[i] = target[index]; // and here will return GDKKN VNQKC 
					}
			}
		}
		return offsetChar + (new String(result).toString());
 
	}
 
 
	public static int positionToUse(String s) { //hardcoded
		char[] sourceChar = source.toCharArray();
		char offsetChar;
		int offset = 5;
		boolean found = false;
		for (int i = 0; i < sourceChar.length; i++) {
 
			if (offset == sourceChar[i])
				found = true;
		}
		return offset;// ? offset : (int) (Math.random() * source.length());
	}
	
	// a method that returns the transformed array based on the character picked from source and the index position in the reference table
	public static char[] rotatedArray(String source) {
		char[] sourceChar = source.toCharArray();
		int offset = 1;
		char[]rotatedArr = new char[sourceChar.length];
		for (int i = 0; i < rotatedArr.length; i++) {
			int index = (i + offset)%rotatedArr.length;
			rotatedArr[index] = sourceChar[i];
		}
		return rotatedArr;
	}
	
	
 
	public static int RandomPositionToUse(String s) {		
		char[] sourceChar = s.toCharArray();
		boolean found = false;
		int offset = new Random().nextInt(s.length());
		for (int i = 0; i < sourceChar.length; i++) {
 
			if (offset == sourceChar[i])
				found = true;
		}
		return offset;// ? offset : (int) (Math.random() * source.length());
	}
 
	public static void main(String[] args) {
		System.out.println(new EncodingWithModulus().obfuscate("HELLO WORLD")); // becomes BGDKKN VNQKC
		
	}
}

Thanks a million.

[Campbell Ritchie]

Please tell us the algorithm you are using. It looks like a Caesar cipher with negative changes..
Why have you made so many things static? Aren't you using object orientation? Why is line 72 calling static methods on an object reference?
Your lines are too long and the // comments make them longer still. Some of those comments should probably be changed to documentation comments.
Why have you not made the fields private? Come to think of it, you should most probably have made most of those methods private, too.
What is positionToUse() supposed to do? the comment is by no means clear, and the method appears to have local variables and loops which don't actually do anything.
What is offset, and why might it equal a source character in line 65? Don't start the name of that method with a capital R. It also seems to contain code repeated from another method, which should be refactored. Or maybe deleted.
Don't present code for review with parts commented out. I found something commented out that I don't like: arithmetic with Math.random(). Use a Random object instead.
What do you mean about had‑coding values? You appear to have presented us with incomplete code.

[tangara goh]

tangara goh wrote:Hi guys,

I would like to have someone review my code.

The objective of the code is to encode a source String with a character that is indicated and the source String will be used as the reference table.  So, A will be 0, B will be 1, C will be 2 and D will be 3 till end of the reference table which is String source.

I am now using a hardcoded 1 as an index to generate the rotatedArray - the target array for me to populate the output : BGDKKN VNQKC

static final String source = "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789()*+,-./";//B will be the indicated character 
	//static final String target1 = "/ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789()*+,-./"
	static final String target = "+,-./ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789()*";
 
	public String obfuscate(String s) {				
		char[] sourceChar = s.toCharArray();//Hello World 
		char[] target = rotatedArray(source);
		System.out.println("rotated array now is " + new String(target));
		int pos = positionToUse(s);
		System.out.println("target " + new String(target));//"/ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789()*+,-." for index = 1
		char[] result = new char[s.length()];
		String offsetChar = null;
		for (int i = 0; i < s.length(); i++) {
			if (s.substring(s.length() - 1) != "") {
				char c = s.charAt(i);
				if (c == ' ') {					
					c = ' ';// Preserve spaces
					result[i]= ' ';
				} else {
					int index = source.indexOf(c);
					char rtn = source.charAt(pos);
					offsetChar = Character.toString(rtn);	//this will return the position of B from the source			
					result[i] = target[index]; // and here will return GDKKN VNQKC 
					}
			}
		}
		return offsetChar + (new String(result).toString());
 
	}
 
 
	public static int positionToUse(String s) { //hardcoded
		char[] sourceChar = source.toCharArray();
		char offsetChar;
		int offset = 1;
		boolean found = false;
		for (int i = 0; i < sourceChar.length; i++) {
 
			if (offset == sourceChar[i])
				found = true;
		}
		return offset; 
	}
	
	// a method that returns the transformed array based on the character picked from source and the index position in the reference table
	public static char[] rotatedArray(String source) {
		char[] sourceChar = source.toCharArray();
		int offset = 1;
		char[]rotatedArr = new char[sourceChar.length];
		for (int i = 0; i < rotatedArr.length; i++) {
			int index = (i + offset)%rotatedArr.length;
			rotatedArr[index] = sourceChar[i];
		}
		return rotatedArr;
	}
	
	
 
	public static int RandomPositionToUse(String s) {		
		char[] sourceChar = s.toCharArray();
		boolean found = false;
		int offset = new Random().nextInt(s.length());
		for (int i = 0; i < sourceChar.length; i++) {
 
			if (offset == sourceChar[i])
				found = true;
		}
		return offset;// ? offset : (int) (Math.random() * source.length());
	}
 
	public static void main(String[] args) {
		System.out.println(new EncodingWithModulus().obfuscate("HELLO WORLD")); // becomes BGDKKN VNQKC
		
	}
}

Thanks a million.

I can't do an edit from my original post so here is more information.  

@Campbell,

Should I shift the static String to the method since the String source will be referenced upon to make it into a new Array except that the encoded String will still carrying the first indicated character from String source like for this case it is B, but it can be F or anything from String source.

I am not sure if the positionToUse is necessary but I am not sure at this point of time know what is the best way to extract out the position of the indicated character.

[tangara goh]

Campbell Ritchie wrote:Please tell us the algorithm you are using. It looks like a Caesar cipher with negative changes..
Why have you made so many things static? Aren't you using object orientation? Why is line 72 calling static methods on an object reference?
Your lines are too long and the // comments make them longer still. Some of those comments should probably be changed to documentation comments.
Why have you not made the fields private? Come to think of it, you should most probably have made most of those methods private, too.
What is positionToUse() supposed to do? the comment is by no means clear, and the method appears to have local variables and loops which don't actually do anything.
What is offset, and why might it equal a source character in line 65? Don't start the name of that method with a capital R. It also seems to contain code repeated from another method, which should be refactored. Or maybe deleted.
Don't present code for review with parts commented out. I found something commented out that I don't like: arithmetic with Math.random(). Use a Random object instead.
What do you mean about had‑coding values? You appear to have presented us with incomplete code.

Hello Campbell,

I have tried to edit my original but was not able to so I edit it from quotating my original post.
Hope that it is clearer to you now what this code is suppose to do.

[Stephan van Hulst]

tangara goh wrote:I have tried to edit my original but was not able to

Correct. We do not want people to edit their posts after they have been replied to.

Think about how difficult it would be to read the topic if the replies no longer matched the original post.

When you want to share new  code, make a new post that contains your changes. Please don't use the "Quote" feature either: your post will be harder to read.

[Campbell Ritchie]

tangara goh wrote:. . . Thanks a million.

That's a pleasure

. . . @Campbell,

Should I shift the static String to the method . . .

I think probably no. I think you have lots of code you are not using and lots of algorithms you are not using. It looks like code that had just growed (Topsy: in Uncle Tom's Cabin
by Harriet Beecher Stowe). I would go back to the original design. It should be possible to create a simple formula for a Caesar cipher, and simply apply that to each letter.

I am not sure if the positionToUse is necessary . . .

What does it do? Where are you using it? I don't think you need a position at all. You want an input and an output. Decide whether you are doing it in a functional or object‑oriented way. Decide what to do about spaces. Decide the arithmetic to convert A to Z for the cipher. I think you will end up with the code occupying about 15% of the space it does at present.

[tangara goh]

Campbell Ritchie wrote:What does it do? Where are you using it? I don't think you need a position at all. You want an input and an output. Decide whether you are doing it in a functional or object‑oriented way. Decide what to do about spaces. Decide the arithmetic to convert A to Z for the cipher. I think you will end up with the code occupying about 15% of the space it does at present.

Hi Campbell, this is not a cipher if my understanding is correction and neither is this a encryption algorithm.  The assignment is in PDF and I am not sure if I can attach it here.

It is an interview assignment and really after frantically studying DSA I still have to google to know how to start.

But, I started at the wrong foot - without using the rotatedArray the k rotation famous question and then I changed it after my mistake.

However, I am really a noob in DSA so I don't have friends to ask I decided to ask her, to improve my coding skill.

Is there anything I can improve upon ? The assignment stated that they want OOP design....

What is a functional design ? I am not clear about it? Is the code I have posted here OOP or functional ?

[Paul Clapham]

tangara goh wrote:The assignment stated that they want OOP design....

What is a functional design ? I am not clear about it? Is the code I have posted here OOP or functional ?

It isn't OOP because you haven't used any objects (except in the most trivial way). It isn't functional because you haven't used any functional programming features.

Although for this assignment I would have used code similar to what you posted. I suppose it would be possible to design some objects to do this character-mashing work but I wouldn't ask people I was considering hiring to do that. It might just encourage them to do it when working at my organization.

[Tim Holloway]

This is indeed a substitution cipher, where encoding is done character-by-character using a simple mapping function. There are more complex forms, but this is pretty much the basis for them all.

It's quite insecure, but it suffices when your enemies are barely literate to begin with.

[tangara goh]

Paul Clapham wrote:

tangara goh wrote:The assignment stated that they want OOP design....

What is a functional design ? I am not clear about it? Is the code I have posted here OOP or functional ?

It isn't OOP because you haven't used any objects (except in the most trivial way). It isn't functional because you haven't used any functional programming features.

Although for this assignment I would have used code similar to what you posted. I suppose it would be possible to design some objects to do this character-mashing work but I wouldn't ask people I was considering hiring to do that. It might just encourage them to do it when working at my organization.

Well, you have piqued my interest.  I really would love to know how you would have written the code in OOP fashion and also the functional programming way.  Could you show me PLEASE ? Really, I did not go thru any degree in computer science and this entire year I suffered with no job but keep learning from free resources on line and my neck is so painful without ceasing due to the strain on solving those difficult DSA questions and I hardly get take home assignments, if i get it, done it and working one, I got ghosted.

[Tim Holloway]

Paul's response reminds me of the "small-child-with-hammer" syndrome. Not every problem is a nail.

Unfortunately, HR is not known for its ability to recognize technical competence, so they put together exercises that involve a hammer, a package of screws, a plank and some Jell-O™.

I latched onto OOP almost as soon as it came out, and even I don't use OOP to solve all my software development needs. Got a project right now that isn't OOP-worthy, in fact.

Now in a case of a simple cryptographic encoder, I'd make the encoder itself an object, have an "encode" method that takes plain-text in and spits encoded text out, and maybe allows you to inject custom translation tables. This has merit because it can easily be expanded (possibly subclassed) to handle extensions of the basic functio such as rotating ciphers, where you use one translation table on the first character, another on the second, and so forth.

[Campbell Ritchie]

final class CaesarCipher
{
   private final String input;
   private final String output;
   private final int difference;

   public CaesarCipher(String input, int difference)
   {
      this.input = input;
      output = this.convert(input, difference);
   }

   private String convert(String input, int difference)
   {
      // ...
      return output;
   }

   @Override
   public String toString()
   {
      return String.format(
           "CaesarCipher %s by %d=%s",
            input, difference output);
   }

   @Override
   public boolean equals(Object ob)
   {
      return ob == this ||
             ob instanceof CaesarCipher &&
             input.equals(ob.input) &&
             output.equals(ob.output) &&
             difference == ob.difference;
   }

   @Override
   public int hashCode()
   {
      return (0x1f * input.hashCode() +
             output.hashCode()) * 0x1f + difference;
   }

   public String input()
   {
      return input;
   }

   public String output()
   {
      return output;
   }
}

Now work out how you can use other classes representing [edit: complete the sentence] different algorithms. Work out which checks I have written and which not written and why.

[Campbell Ritchie]

A few minutes ago, I wrote:. . . Now work out . . .

Also find the mistake I inserted late in the process and correct it.

[Carey Brown]

   public boolean equals(Object ob)
   {
      return ob == this ||
             ob instanceof CaesarCipher cc &&
             input.equals(cc.input) &&
             output.equals(cc.output) &&
             difference == cc.difference;
   }

[tangara goh]

Campbell Ritchie wrote:Now work out how you can use other classes representing [edit: complete the sentence] different algorithms. Work out which checks I have written and which not written and why.

I see. I forgot all about POJO since I have not been really touching OOP for more than 1 year.
Now, the thing is this code seems more like a utility class, is there a need to do it the POJO way with the usual hashCode in object method ?
Why don't Java built in this part, since it will save time, since this seems like a compulsory things to do ?  

[Campbell Ritchie]

No, that isn't a utility class. Explain why you might want to write a utility class, and how you would do it. Find my mistake. Check the parts I omitted or didn't omit, as I said yesterday. Carey Brown has contacted me privately to tell me I made another mistake. At least the compiler will find those two mistakes.

tangara goh wrote:. . . Why don't Java built in this part . . .

There is a construct that allows you to create objects of a class with all those parts added automatically.
If you are sure nobody will ever going to put your objects into a collection or a Map, then nobody will notice you have omitted equals() and hashCode(). It is very likely that somebody will want to print the object, so you should expect many complaints if you don't override toString().

[Tim Holloway]

ACK! Campbell, WHAT HAVE YOU DONE???  

You have processor logic IN YOUR CONSTRUCTOR!

Also a lot of rendundant code like equals() and toString() which I hope were simply auto-generated by your IDE, as any practical need for them is very unlikely.

[Lou Hamers]

Tim Holloway wrote:ACK! Campbell, WHAT HAVE YOU DONE???  

Surely he was testing everyone! You won!!!

[Campbell Ritchie]

Tim Holloway wrote:ACK! Campbell, WHAT HAVE YOU DONE???   . . .

Knocked something together in a couple of minutes over a drink.
If you make the fields final, you have to initialise them before the constructor completes.

Also a lot of rendundant code . . . .

I have already explained equals(). You always need toString(). The short way of creating such a class would have included auto‑generated methods like that. I am keeping quiet about what the short technique is.
As an alternative, you can write the whole thing in a functional style.

[Tim Holloway]

Campbell Ritchie wrote:

Tim Holloway wrote:ACK! Campbell, WHAT HAVE YOU DONE???   . . .

Knocked something together in a couple of minutes over a drink.
If you make the fields final, you have to initialise them before the constructor completes.

Also a lot of rendundant code . . . .

I have already explained equals(). You always need toString(). The short way of creating such a class would have included auto‑generated methods like that. I am keeping quiet about what the short technique is.
As an alternative, you can write the whole thing in a functional style.

I'd say more like after the fourth or fifth drink. First of all, it's a gratuitous one-off class where the solution is nailed to the problem. You need a separate instance for every encryption you do. I have no idea why toString() is mandatory. I've written innumerable classes without soString(). It looks here like you've back-doored in a separate solution (a specific hard-coded formatting of class properties) to the primary solution (encoding). The need for equals() makes even less sense.

Practically speaking it looks more like a procedural-programming attempt to code in an object-based system (note, I consider C++ object-oriented because it still allows non-OOP. Java I prefer to call objectbased because objects are not an option. But I bow to general usage).

[Campbell Ritchie]

It takes more than that to get me under the table. The true object‑oriented approach would want one object per message. You think that isn't desirable, so, let's try  a functional approach. I think that will look better.

class CaesarCypher
{
   private CaesarCypher()
   {
   }

   private static final int LETTERS_IN_ALPHABET = 26;

   public static String encrypt(String input, int difference)
   {
      return something;
   }

   public static String decrypt(String input, int difference)
   {
      return something;
   }
}

No equals(), nor hashCode(), nor toString() because we have an uninstantiable class. You might want a private static helper method to do the actual arithmetic.

Last week, I wrote:Work out which checks I have written and which not written and why.

I didn't write any. I should probably have written a test with a regex to confirm that the input only contains the twenty‑six English letters and spaces and maybe punctuation. I should probably have written a test whether the difference was in the range (int)'A'...(int)'Z' or similar. I could have written a test for whether the input is null, but thought I would suffer an exception as soon as I tried to manipulate that String, so it would be found out very soon.
I would have liked to see an algorithm, which I suspect can be done with the + − and % operators and a few ifs. I don't think those arrays are necessary.

[Tim Holloway]

Campbell Ritchie wrote:It takes more than that to get me under the table. The true object‑oriented approach would want one object per message.

Tangarah didn't indicate that he was expected to produce a container object for tests before and after encryption. Unless I missed something (again) in the evolution of this thread, what he really needed was an OOP-based mechanism to convert a plaintext to a cyphertext. To make an immutable object out of that would be the same as making an immutable object out of a Fahrenheit-to-Celsius temperature converter.  It's making a heavyweight item that cannot be freely used in general code where a simple converted value is all that's required.

I might have a separate encrypter object for each encoding table, but never for each individual string. What if you were supposed to encode thousands of strings per minute?

The static example is completely different to the previous instantiating example, I still haven't seen the requirement for the ancillary methods and I'm just getting more and more confused here.

[Chris Smithtopher]

I'm still quite a noob in general, but it's great to see 2 distinct viewpoints and approaches on the same issue. It kind of gives me insight on the types  of discussions I can expect to have in the professional world.

[Lou Hamers]

It is good to see that, that's one of the fun aspects of dev. Despite the fact that we keep trying to make it into a science, it's really a hybrid art/science.