Skip to main content
Code Review

Return to Answer

added 22 characters in body
Source Link
Roland Illig
  • 21.8k
  • 2
  • 36
  • 83

Your main method should be much shorter. Ideally only one or two lines:

public static void main(String []args) {
 Scanner in = new Scanner(System.in);
 System.out.println(isWellformedExpression(in.nextLine()));
}

Nothing more. All the rest of your code should go into a separate method. This has several benefits:

  • That method is simpler since it doesn't have anything to do with input/output.
  • That method has a name describing what it does.
  • That method can be called in some testing code. You wouldn't want to set up a testing environment for the main method since providing System.in and System.out inis quite difficult.

The testing code could look like this:

static void testIsWellformedExpression() {
 assertTrue(isWellformedExpression("1"));
 assertTrue(isWellformedExpression("1+2"));
 assertTrue(isWellformedExpression("-5"));
 assertTrue(isWellformedExpression("((((((((1))))))))"));
 assertFalse(isWellformedExpression(""));
 assertFalse(isWellformedExpression("+"));
 assertFalse(isWellformedExpression("1+"));
 assertFalse(isWellformedExpression(")"));
}

You would have to define the methods assertTrue and assertFalse yourself, or better yet add JUnit (a testing framework) to your project.


The expression 0 == x.compareTo("+") looks strange, as if it came from another programming language (C) and from outer space (because of the Yoda style). In plain English you wouldn't say "if zero is the same as ...", but you would say "if ... is zero" instead. So should your code. You can just say x.equals("+"), which is more to the point.

I have no idea why you need 6 different lists. One should be enough.

The names you chose for these variables are terrible since they tell the reader nothing. They are not even in English, so most of this site's readers won't understand them at all. What is pila3? What is ww? Think of better names for them. What words would you use to describe their purpose to a human?

Instead of using a raw LinkedList, you should rather use a LinkedList of strings, which is written as LinkedList<String> in Java. Then you don't need these (String) type casts whenever you take something out of the list.

Your main method should be much shorter. Ideally only one or two lines:

public static void main(String []args) {
 Scanner in = new Scanner(System.in);
 System.out.println(isWellformedExpression(in.nextLine()));
}

Nothing more. All the rest of your code should go into a separate method. This has several benefits:

  • That method is simpler since it doesn't have anything to do with input/output.
  • That method has a name describing what it does.
  • That method can be called in some testing code. You wouldn't want to set up a testing environment for the main method since providing System.in and System.out in quite difficult.

The testing code could look like this:

static void testIsWellformedExpression() {
 assertTrue(isWellformedExpression("1"));
 assertTrue(isWellformedExpression("1+2"));
 assertTrue(isWellformedExpression("-5"));
 assertTrue(isWellformedExpression("((((((((1))))))))"));
 assertFalse(isWellformedExpression(""));
 assertFalse(isWellformedExpression("+"));
 assertFalse(isWellformedExpression("1+"));
 assertFalse(isWellformedExpression(")"));
}

You would have to define the methods assertTrue and assertFalse yourself, or better yet add JUnit to your project.


The expression 0 == x.compareTo("+") looks strange, as if it came from another programming language (C) and from outer space (because of the Yoda style). In plain English you wouldn't say "if zero is the same as ...", but you would say "if ... is zero" instead. So should your code. You can just say x.equals("+"), which is more to the point.

I have no idea why you need 6 different lists. One should be enough.

The names you chose for these variables are terrible since they tell the reader nothing. They are not even in English, so most of this site's readers won't understand them at all. What is pila3? What is ww? Think of better names for them. What words would you use to describe their purpose to a human?

Instead of using a raw LinkedList, you should rather use a LinkedList of strings, which is written as LinkedList<String> in Java. Then you don't need these (String) type casts whenever you take something out of the list.

Your main method should be much shorter. Ideally only one or two lines:

public static void main(String []args) {
 Scanner in = new Scanner(System.in);
 System.out.println(isWellformedExpression(in.nextLine()));
}

Nothing more. All the rest of your code should go into a separate method. This has several benefits:

  • That method is simpler since it doesn't have anything to do with input/output.
  • That method has a name describing what it does.
  • That method can be called in some testing code. You wouldn't want to set up a testing environment for the main method since providing System.in and System.out is quite difficult.

The testing code could look like this:

static void testIsWellformedExpression() {
 assertTrue(isWellformedExpression("1"));
 assertTrue(isWellformedExpression("1+2"));
 assertTrue(isWellformedExpression("-5"));
 assertTrue(isWellformedExpression("((((((((1))))))))"));
 assertFalse(isWellformedExpression(""));
 assertFalse(isWellformedExpression("+"));
 assertFalse(isWellformedExpression("1+"));
 assertFalse(isWellformedExpression(")"));
}

You would have to define the methods assertTrue and assertFalse yourself, or better yet add JUnit (a testing framework) to your project.


The expression 0 == x.compareTo("+") looks strange, as if it came from another programming language (C) and from outer space (because of the Yoda style). In plain English you wouldn't say "if zero is the same as ...", but you would say "if ... is zero" instead. So should your code. You can just say x.equals("+"), which is more to the point.

I have no idea why you need 6 different lists. One should be enough.

The names you chose for these variables are terrible since they tell the reader nothing. They are not even in English, so most of this site's readers won't understand them at all. What is pila3? What is ww? Think of better names for them. What words would you use to describe their purpose to a human?

Instead of using a raw LinkedList, you should rather use a LinkedList of strings, which is written as LinkedList<String> in Java. Then you don't need these (String) type casts whenever you take something out of the list.

Source Link
Roland Illig
  • 21.8k
  • 2
  • 36
  • 83

Your main method should be much shorter. Ideally only one or two lines:

public static void main(String []args) {
 Scanner in = new Scanner(System.in);
 System.out.println(isWellformedExpression(in.nextLine()));
}

Nothing more. All the rest of your code should go into a separate method. This has several benefits:

  • That method is simpler since it doesn't have anything to do with input/output.
  • That method has a name describing what it does.
  • That method can be called in some testing code. You wouldn't want to set up a testing environment for the main method since providing System.in and System.out in quite difficult.

The testing code could look like this:

static void testIsWellformedExpression() {
 assertTrue(isWellformedExpression("1"));
 assertTrue(isWellformedExpression("1+2"));
 assertTrue(isWellformedExpression("-5"));
 assertTrue(isWellformedExpression("((((((((1))))))))"));
 assertFalse(isWellformedExpression(""));
 assertFalse(isWellformedExpression("+"));
 assertFalse(isWellformedExpression("1+"));
 assertFalse(isWellformedExpression(")"));
}

You would have to define the methods assertTrue and assertFalse yourself, or better yet add JUnit to your project.


The expression 0 == x.compareTo("+") looks strange, as if it came from another programming language (C) and from outer space (because of the Yoda style). In plain English you wouldn't say "if zero is the same as ...", but you would say "if ... is zero" instead. So should your code. You can just say x.equals("+"), which is more to the point.

I have no idea why you need 6 different lists. One should be enough.

The names you chose for these variables are terrible since they tell the reader nothing. They are not even in English, so most of this site's readers won't understand them at all. What is pila3? What is ww? Think of better names for them. What words would you use to describe their purpose to a human?

Instead of using a raw LinkedList, you should rather use a LinkedList of strings, which is written as LinkedList<String> in Java. Then you don't need these (String) type casts whenever you take something out of the list.

lang-java

AltStyle によって変換されたページ (->オリジナル) /