This week's book giveaway is in the OO, Patterns, UML and Refactoring forum.
We're giving away four copies of Five Lines of Code and have Christian Clausen on-line!
See this thread for details.
Win a copy of Five Lines of Code this week in the OO, Patterns, UML and Refactoring forum!
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
programming forums Java Mobile Certification Databases Caching Books Engineering Micro Controllers OS Languages Paradigms IDEs Build Tools Frameworks Application Servers Open Source This Site Careers Other all forums
this forum made possible by our volunteer staff, including ...
Marshals:
  • Campbell Ritchie
  • Bear Bibeault
  • Ron McLeod
  • Jeanne Boyarsky
  • Paul Clapham
Sheriffs:
  • Tim Cooke
  • Liutauras Vilda
  • Junilu Lacar
Saloon Keepers:
  • Tim Moores
  • Stephan van Hulst
  • Tim Holloway
  • fred rosenberger
  • salvin francis
Bartenders:
  • Piet Souris
  • Frits Walraven
  • Carey Brown

How can I clean up my code/make it more efficient?

 
Greenhorn
Posts: 3
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Just looking to touch up code and make it cleaner! Thanks in advance.

 
Marshal
Posts: 69756
277
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Welcome to the Ranch

You are asking two questions in the thread title, which are completely different. Only worry about “efficiency” if you have some sort of evidence that the program is running inefficiently. There are various simple thing syou can do, however. I think you shou‍ld use few print calls and make them larger. You can probably run a 200‑letter String together in 0.2μs and then take a few milliseconds to print it on screen. You can change lines 31‑35 to one line; familiarise yourself with the printf method (there are several other methods scattered around which behave very similarly) and you will find it very useful:-You have four %ns for new line and two %ss for String representation, so I think I have compressed those 5 lines correctly. Note how I broke the line into shorter lines. You will also note I removed the// print key comment, which adds nothing.
As for speeding up the code, I would suggest what else you shou‍ld do about performance just at the present: nothing. Much more important to make the code correct first. So I shall say little about formatting, though you shou‍ld make lots of formatting changes.
 
Ned Ma
Greenhorn
Posts: 3
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Campbell Ritchie wrote:Welcome to the Ranch

You are asking two questions in the thread title, which are completely different. Only worry about “efficiency” if you have some sort of evidence that the program is running inefficiently. There are various simple thing syou can do, however. I think you shou‍ld use few print calls and make them larger. You can probably run a 200‑letter String together in 0.2μs and then take a few milliseconds to print it on screen. You can change lines 31‑35 to one line; familiarise yourself with the printf method (there are several other methods scattered around which behave very similarly) and you will find it very useful:-You have four %ns for new line and two %ss for String representation, so I think I have compressed those 5 lines correctly. Note how I broke the line into shorter lines. You will also note I removed the// print key comment, which adds nothing.
As for speeding up the code, I would suggest what else you shou‍ld do about performance just at the present: nothing. Much more important to make the code correct first. So I shall say little about formatting, though you shou‍ld make lots of formatting changes.



thank you! I appreciate the kind words, the print method worked as well.
 
Campbell Ritchie
Marshal
Posts: 69756
277
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I am afraid the structure of the program is confused, and I can see no object‑orientation there. Why have you not get a Message class, with plain and encrypted fields? Remember that is nothing like how you would encrypt something in a security‑compromised environment (i.e. the whole world outside your own computer).
You have far too much code in the main method, whose ideal length is one statement.
Why does the Scanner field have public access? All fields except those used as global constants shou‍ld hve private access; if you need to use them elsewhere, you can grant access via methods. You can even copy the fields to prevent their changing state inside your object.
Why are you mixing Strings and StringBuilder? In line 73, you are using += on Strings, an inefficient operation; you would do better to use the StringBuilder there. I tend to use StringBuilders only as local variables; if you keep them well hidden inside your methods, no harm can come to them, but if you let them escape, other code can change them without your knowing about it.
I removed a comment which I thought wasn't needed, but I think you shou‍ld add some /* comments */ elsewhere to explain what is going on. I cannot tell how you are encrypting the text, but comments would help me know what you are trying to do, so I can see whether you have got the algorithm right.
I don't like Math#random, despite what the Java™ Tutorials say. You will find a Random object gives more reliable code; something like your line 143 is very error‑prone.
Another things about StringBuilders; their methods are fiddled so you can join them all together. You can compress lines 125‑133 intoThen return key.toString() and change the method's return type to String.
You have an inconsistency between methods; some return Strings and some are void. I think they shou‍ld probably all return Strings. Remember that letting anybody else get their hands on your key will breach all your security.
 
Ned Ma
Greenhorn
Posts: 3
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Campbell Ritchie wrote:I am afraid the structure of the program is confused, . . .



I am still learning Java and i appreciate all the input. This code is more for a personal project, and I was just trying to make it clutter free. The things you said are very good points.
 
Sheriff
Posts: 15733
264
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Why is the code for these three methods exactly the same? Why not just have one method?
 
Junilu Lacar
Sheriff
Posts: 15733
264
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
You wrote:
...
This code suffers from "comments are a code smell". Why don't you just rename the variable so that it's not so vague/nebulous as "temp"? I tell programmers I mentor that using variable names like "temp" when there is a better name that could be used is just being lazy. You're not putting that little bit of effort it takes to find the word that expresses the variables purpose or intent and the code's quality suffers because of that laziness.

Also, you don't have to declare the variable so far away from where it is actually used. You only use it in the for-loop, so declare it inside the for-loop. Keep the scope of program variables to a minimum.

Lastly, that formula on line 71 is verbose.  You can extract that to a method of its own, then you can write this instead:

The name firstChar now explains better what the significance of the value is, and the method call isVowel(firstChar) clearly tells what you're trying to check.
 
Junilu Lacar
Sheriff
Posts: 15733
264
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
You could also write that as

There are many ways to skin that (ch)at (bad joke)
 
Junilu Lacar
Sheriff
Posts: 15733
264
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Still staying on the subject of good names, most of the names you use are not good. "iString", "X1X2", "Y1", "zSkip", "temp" and most everything else are atrocious. How would you like it if you went in to work and someone said to you, "Hey, can you go to AZ and get me a Y1? And if there are any X2s just drop a few AZ3s into the X4 at QRCD. Got that?"  That's what reading your code is like.

Try to use terms from the problem domain. In the world of cryptography, the original unencrypted text is usually referred to as "plaintext" or "cleartext".  The encrypted text is called "ciphertext". Consider using these and other terms that someone with domain knowledge in cryptography can easily recognize.
 
Junilu Lacar
Sheriff
Posts: 15733
264
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Let's ignore the fact that you only have static methods in your code and that it's not object-oriented.  Let's focus on some of your design decisions instead.

You wrote this:

1. Why add a comment to spell out what you mean by the abbreviated name? Why not just spell out the name if that's really what you want to say? You're not saving much by using abbreviated names like that. Using abbreviations like that makes your code less expressive, more cryptic, and harder to read. You should be able to pronounce the names in your code and they should be whole words or as close to whole words as they can be. Just write this, without a comment:

Then, when you see that, you should think, "Well, that name is a noun. Methods should be verb phrases." Maybe this will be better:

2. Why return a StringBuilder from this method? Do you want someone outside of this method to modify the key that was just calculated/generated in this method? If not, then just return the final result as a String.

You can see that in just one line of code—well, three lines if you include the deleted comment and the return statement—there are already a few things that you can change to make the code a lot better.

The rest of your code has many of the same kind of issues.
 
Junilu Lacar
Sheriff
Posts: 15733
264
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

I see what you're doing with this code now. That's like driving downtown and back just to get to your neighbor's house down the street in the suburbs (and ironically, just like this analogy in getting to the point).

I already noted previously that your randomSpecial(), randomAZ(), and randomZ() do exactly the same thing. What you did and why you did it is not good.

First, you are hard coding values like (2, 5), (65, 90), and (33, 47). The significance of those numbers is not immediately apparent, even to someone who is familiar with char integer values. Second, you are jumping through way too many hoops just to get four random int values converted to chars.

Since a char is an integer type, you can use it just like you would an integer. Conversion from char to int happens transparently. Also, as someone probably already mentioned, prefer using a instance of java.util.Random instead of Math.random().

You can code this up more cleanly and elegantly with Java 8 lambdas and Random. I'll give you a chance to try your hand at it but here's the outline:

Instead of using int literals, you use char literals to make the nature and intent of the values apparent.
 
Junilu Lacar
Sheriff
Posts: 15733
264
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Well, I guess the declarations I gave you conflict with the fact that you're still using static methods.  The idea will be the same though if you use static methods.  That's another thing you should move away from. That's just not a good habit to develop because you'll have a harder time wrapping your head around object-orientation and OOP concepts later.
 
a wee bit from the empire
Building a Better World in your Backyard by Paul Wheaton and Shawn Klassen-Koop
https://coderanch.com/wiki/718759/books/Building-World-Backyard-Paul-Wheaton
    Bookmark Topic Watch Topic
  • New Topic