Originally Posted by
Gorman
Nicely done! I can see a lot of minor semantic/grammatical errors, but the program runs nicely. It is also good that you are looking to optimisation, just be aware that in this program it really isn't needed I also like how you didn't use OO, since it is not necessary.
Thanks. I meant more like finding the coefficients exponents more quickly, but looking over it there really isn't any way to improve it.
Originally Posted by Gorman
The first error I see is in naming conventions;
Java code:
String Exponent="1";
String Exponent2="1";
String Coefficient1="1";
String Coefficient2="1";
String finalTerm="1";
String Teste="";
See the inconsistency? It is even more obvious when looking at the entire system, there are a lot of different ones!
Luckily for you Java is a very well defined language, so it is not hard to find out what should be going on, from wikipedia;
Classes: Class names should be nouns in "CamelCase", with the first letter capitalised. Try to keep your class names simple and descriptive. Use whole words — avoid acronyms and abbreviations (unless the abbreviation is much more widely used than the long form, such as URL or HTML). (well you only had one class so this wasn't a big deal, you should name it something more like "TrinomialFactoriser" to make it obvious that that is what it does. "FactorTrinomial" is more of a verb.)
Methods: Methods should be verbs, in "CamelCase", with the first letter lower case. (You did this inconsistently)
Variables: Local variables, instance variables, and class variables are also written in "CamelCase", with the first letter lower case. Variable names should not start with underscore ('_') or dollar sign ('$') characters, even though both are allowed. Certain coding conventions state that underscores ('_') should be used to prefix all instance variables, for improved reading and program understanding.
Variable names should be short yet meaningful. The choice of a variable name should be mnemonic — that is, designed to indicate to the casual observer the intent of its use. One-character variable names should be avoided except for temporary "throwaway" variables. Common names for temporary variables are i, j, k, m, and n for integers; c, d, and e for characters.
Constants: Constants should be written in uppercase characters separated by underscores. Constant names may also contain digits if appropriate, but not as the first character. (You followed this naming convention!)
Somewhere I got the bad habit of naming String variables starting with a capital. They should probably be renamed test1, test2, etc.
I'll get around to renaming the class TrinomialFactorer.
I only put the method name Trinomial in caps because I call the JTextArea for input trinomial. This is probably poor style, and I'll rename trinomial to trinomialInput and Trinomial to trinomial.
Originally Posted by Gorman
Java code:
else if(actionCommand.equals("Clear")) {trinomial.setText("");}
else {trinomial.setText("Error");}
I would suggest adding some more space to make things easier to read.
Like this, at least;
Java code:
else if (actionCommand.equals("Clear")) { trinomial.setText(""); }
else { trinomial.setText("Error"); }
It will especially clean up things like this;
Java code:
cTerm=cTerm/q;
bottomBox=bottomBox/q;
IN TO
cTerm = cTerm/q;
bottomBox = bottomBox/q;
Will do, and will also add spacing before/after &&'s in if statements.
Originally Posted by Gorman
Java code:
cTerm=cTerm/q;
bottomBox=bottomBox/q;
IN TO
cTerm /= q;
bottomBox /= q;
In this example the formatting
cTerm/=q; is fine too
There are probably more examples, so keep your eyes open
I've never really felt happy doing this, but if it's convention I'll change it.
Originally Posted by Gorman
Java code:
)){
if (counter2==0){
if (Tes.equals("-")){Coefficient1=Equation.substring(a,i);}
else {Coefficient1=Equation.substring(a+1,i);}
counter2++;
break;
}
if (counter2==1){
if (Tes.equals("-")){Coefficient2=Equation.substring(a,i);}
else {Coefficient2=Equation.substring(a+1,i);}
break;
}
As you can see, you have the lines
if (Tes.equals("-")){Coefficient1=Equation.substring(a,i);}
else {Coefficient1=Equation.substring(a+1,i);}
repeated.
This is where your shorthand really comes in handy! In this case, the ? : operator. ? : performs a logical test, and then performs an action based on the result, in this case it will be obvious, so I will hold out on the explanation.
Java code:
)){
if (counter2<=1){
if (Tes.equals("-")){Coefficient1=Equation.substring(a,i);}
else {Coefficient1=Equation.substring(a+1,i);}
counter2 += (counter2 == 0) ? 1 : 0; // As you can see, the logic (counter2 == 0) is evaluated, and if it is true then add 1, otherwise leave unchanged!
break;
}
Always be on the lookout for ways to keep your code small and unrepetitive.
I was actually thinking of using inline conditionals, but that's simply not possible if I want to add compatibility for inherent coefficients of one, such as in x or -x.
I know the entire section is repetitive, but the only way I could think to fix it was to create a for loop and array of two strings, and it seemed like more effort than it was worth given the level of indentation I was already using.
Originally Posted by Gorman
Here is a great example;
Java code:
/*None of these variables are necessary; however, they make the code more explicit. This method uses the X box factoring sequence, which one needs to familiarize oneself with to understand.
This method just assumes that x is one, and the first term has an exponent of 2.
To make this more understandable: any variable with top or bottom is a common multiple outside the box.
Anything with upper or lower are actual numbers within the box*/
else {
int upperLeft=coefficient;
int upperRight=r;
int lowerLeft=t;
int lowerRight=cTerm;
for (int s=Math.abs(upperRight);s>=Math.abs(upperRight)-2*(Math.abs(upperRight));s--){
if (upperRight%s==0 && upperLeft%s==0){
leftTop=s;
break;
}
}
int topRight=upperRight/leftTop;
int leftBottom=lowerRight/topRight;
int topLeft=lowerLeft/leftBottom;
return (q+"("+topLeft+"x+"+topRight+")("+leftTop+"x+"+leftBottom+")");
}
}
The first thing that strikes me about this segment is that it REALLY should be it's own function. You have a block comment telling me about it, and it obviously works independently.
The second thing I notice is two } in line with each other. This should never happen.
The third thing I think is "he more or less says I won't understand it, yet he doesn't make it a function?". It is much easier to understand Factor than to understand all the code... Functions are a powerful abstraction method!
Trinomial is a VERY long function. It needs to be split up. In some places your nesting is 6 deep (now some of that can be reduced with shorthand, but really you need to do both shorthand and modulisation).
Agile techniques prescribe a maximum function length of 25 lines, which I think you more than double (maybe triple?). To be fair Agile is extreme, but I think that you can cut it down to <40 lines easily, which will vastly improve readability.
The last line " return (q+"("+topLeft+"x+"+topRight+")("+leftTop+"x+"+lef tBottom+")");" is another thing.
If you wanted to change the format you would have to edit that string. Rather you should make a seperate function called "ToString" or such, and have it format the string, while the other functions just call it.
Well, it does work independently, but it recycles 4 different strings from the other part. Also, the block of text applies to an earlier section, where it bruteforces two numbers here:
Java code:
topBox=cTerm*coefficient;
System.out.println(topBox+" "+bottomBox);
done:for (r=Math.abs(topBox);r>=Math.abs(topBox)-(2*Math.abs(topBox));r--){
for (t=Math.abs(topBox);t>=Math.abs(topBox)-(2*Math.abs(topBox));t--){
if (t*r==topBox){
if (r+t==bottomBox){
break done;
}
}
}
}
if (coefficient==1){
return (q+"(x+"+r+")(x+"+t+")");
}
I suppose I should move the block of text up before that, and I could make this and all later code two separate functions, but I just don't see the point in splitting up these two parts. It almost seems like a waste of memory. Are small functions just a way of making the code more understandable?
Originally Posted by Gorman
It is nice, but as you can see there is a lot of room for improvement.
I suggest reading about coupling and cohesion (two topics that I didn't even really touch on, but will be hugely useful).
If you want to learn some higher level design, you could also take a look at some Design Patterns.
I'll be updating it soon.
Reading up coupling and cohesion too, thanks for the feedback.