1 task #346

nrsxx · 2017-03-01T22:46:25Z

Second attempt:)

ignorer · 2017-03-04T08:51:13Z

Какой знакомый код. А Елена Смирнова точно дала своё согласие на его заимствование?

DanAnastasyev · 2017-03-12T17:28:42Z

...work-g596-narsia/src/main/java/ru/mipt/java2016/homework/g596/narsia/task1/MyCalculator.java

-    public double calculate(String Expression) throws ParsingException {
-        if (!isExpressionCorrect(Expression)) {
-            throw new ParsingException("Invalid expression");
+    private String whatIsIt(Character Symbol) {


"Ochen` ploxo", ochen'
Неужели нельзя это не так по-индуски сделать?

ignorer

Вынужден сообщить, что читаемость у этого кода далеко не на самом высоком уровне. Прочитав два десятка калькуляторов, я не могу понять, как работает твой, хотя идеологически он вряд ли отличается сильно. Проблема на мой взгляд в слишком больших switch блоках, злоупотреблении ими и отсутствии хоть каких-то комментариев к происходящему.

Некоторые замечания, которые я оставил к твоему коду, относятся не только к той строчке, к которой привязаны, поэтому будь добр, пройдись по всему коду и исправь ошибки в аналогичных местах.

ignorer · 2017-03-14T10:12:06Z

...work-g596-narsia/src/main/java/ru/mipt/java2016/homework/g596/narsia/task1/MyCalculator.java

-    public double calculate(String Expression) throws ParsingException {
-        if (!isExpressionCorrect(Expression)) {
-            throw new ParsingException("Invalid expression");
+    private String whatIsIt(Character symbol) {


Любой человек, который знает Java, понимает разницу между базовыми типами и их обёртками, а также про боксинг и анбоксинг, про который семинарист рассказывал вам ещё в начале семестра. Настоятельно рекомендую почитать материалы семинаров, документацию по java или stackoverflow.com и ознакомиться с тем, что такое primitive wrapper class, boxing и unboxing, а после этого со знанием дела объяснить, зачем тебе здесь именно Character.

ignorer · 2017-03-14T10:20:49Z

...work-g596-narsia/src/main/java/ru/mipt/java2016/homework/g596/narsia/task1/MyCalculator.java

-    public double calculate(String Expression) throws ParsingException {
-        if (!isExpressionCorrect(Expression)) {
-            throw new ParsingException("Invalid expression");
+    private String whatIsIt(Character symbol) {


Ты можешь представить, какой оверхед создаёт эта функция? Если Java ничего не оптимизирует, а будет под каждый вызов твоей волшебной функции whatIsIT() создавать новую строку, этот калькулятор будет съедать памяти раз в 50 больше нужного.
Теперь вопрос: а почему, собственно, возвращается строка? enum SymbolType в списанном решении был таким хорошим, а тут у тебя аж строки возвращаются. Мне кажется, что ответ - чтобы код не был похож на предыдущее решение, но это не точно.

ignorer · 2017-03-14T10:27:46Z

...work-g596-narsia/src/main/java/ru/mipt/java2016/homework/g596/narsia/task1/MyCalculator.java

+            case '6':
+            case '7':
+            case '8':
+            case '9':


Итак, у нас есть задача - нужно определить, является ли символ цифрой. У этой, не побоюсь этого слова, задачи есть несколько решений, из которых ты выбрал самое беспонтовое. Предлагаю ознакомиться с документацией к классу Character (хотя бы пролистать список методов) и сказать, что нам предлагает сам язык в этом плане.

ignorer · 2017-03-14T10:31:58Z

...work-g596-narsia/src/main/java/ru/mipt/java2016/homework/g596/narsia/task1/MyCalculator.java

+            }
+            case ' ':
+            case '\t':
+            case '\n':


Аналогично тому, что я писал выше про числа - для этого уже сделана стандартная функция, и тебе предлагается её отыскать.

ignorer · 2017-03-14T10:33:48Z

...work-g596-narsia/src/main/java/ru/mipt/java2016/homework/g596/narsia/task1/MyCalculator.java

-    private enum SymbolType {
-        OPENING_BRACKET, CLOSING_BRACKET, NUMBER, OPERATOR, NOTHING
+
+    private Integer getCode(Character first, Character second) {


А какой именно код возвращает эта функция? Лично мне сходу вообще непонятна семантика этой функции, в которой к тому же ещё и вложенные switch блоки, а значения кодов - тайна, покрытая мраком и нигде не задокументированная.

ignorer · 2017-03-14T10:42:19Z

...work-g596-narsia/src/main/java/ru/mipt/java2016/homework/g596/narsia/task1/MyCalculator.java

-                    if (previousSymbol == SymbolType.OPENING_BRACKET ||
-                            previousSymbol == SymbolType.OPERATOR) {
-                        return false; //открывающая-закрывающая, оператор-закрывающая
+        for (int cnt = 0; cnt < expression.length(); ++cnt) {


Название cnt по меньшей мере чрезвычайно странное. Как минимум оно трактуется неоднозначно - у тебя оно ассоциируется со словом count, у меня со словом cunt (или наоборот), а двузначность в коде - плохо. И вообще count - это количество или подсчёт, но никак не индекс (счётчик здесь тоже не подходит, ты же ничего не считаешь, а только итерируешься по строке).
И привыкай не использовать в коде сокращения имён (кроме общепринятых). Они хороши только когда пишешь код на листочке без автодополнения.

ignorer · 2017-03-14T11:00:05Z

...work-g596-narsia/src/main/java/ru/mipt/java2016/homework/g596/narsia/task1/MyCalculator.java

+    private StringBuilder removeSpaces(String expression) {
+        StringBuilder result = new StringBuilder(expression.length());
+        for (int cnt = 0; cnt < expression.length(); ++cnt) {
+            if (!whatIsIt(expression.charAt(cnt)).equals("Space")) {


Вот это можно вообще без whatIsIt() сделать также одной строчкой.

ignorer · 2017-03-14T11:01:59Z

...work-g596-narsia/src/main/java/ru/mipt/java2016/homework/g596/narsia/task1/MyCalculator.java

+        expression.append('~');
+        Character curSymbol;
+        Character prevSymbol = '~';
+        Stack<Character> Texas = new Stack<>();


В java code conventions явно указывается, что переменные называются с маленькой буквы.

ignorer · 2017-03-14T11:02:48Z

...work-g596-narsia/src/main/java/ru/mipt/java2016/homework/g596/narsia/task1/MyCalculator.java

-            if (currChar == ' ' || currChar == '\n' || currChar == '\t') {
-                continue;
+        Integer cnt = 0;
+        while (true) {


Как интересно ты в исходном решении заменил for на while.

ignorer · 2017-03-14T11:05:13Z

...work-g596-narsia/src/main/java/ru/mipt/java2016/homework/g596/narsia/task1/MyCalculator.java

+    @Override
+    public double calculate(String expression) throws ParsingException {
+        isAlmostValid(expression);
+        StringBuilder withoutSpaces = removeSpaces(expression);


А зачем тебе именно StringBuilder возвращать из функции? Ты же можешь вернуть return stringBuilder.toString(), и тогда на выходе будет строка, и тип переменной withoutSpaces будет вполне себе понятным - String. А то пока это выглядит так, как будто ты планируешь дальше строить эту строку.

DanAnastasyev · 2017-03-15T18:44:55Z

...work-g596-narsia/src/main/java/ru/mipt/java2016/homework/g596/narsia/task1/MyCalculator.java

+import ru.mipt.java2016.homework.base.task1.Calculator;
+import ru.mipt.java2016.homework.base.task1.ParsingException;
+
+import java.util.*;


Считается плохим тоном импортировать через *. Перечисли всё, что ты импортируешь отдельно

DanAnastasyev · 2017-03-15T18:49:15Z

...work-g596-narsia/src/main/java/ru/mipt/java2016/homework/g596/narsia/task1/MyCalculator.java

+import java.util.*;
+
+// подробное описание работы кода
+// можно найти в файле description


Пиши комментарии прямо в коде

DanAnastasyev · 2017-03-15T18:51:14Z

...work-g596-narsia/src/main/java/ru/mipt/java2016/homework/g596/narsia/task1/MyCalculator.java

+        if (symbol.equals('~')) {
+            return SymbolType.FIRST;
+        }
+        return SymbolType.INVALID;


Во многих языках программирования есть такая штука, как assert. Его используют, чтобы поймать некорректные состояния. Этот твой SymbolType.INVALID - как раз такое состояние. Кидай отсюда исключение

DanAnastasyev · 2017-03-15T18:52:30Z

...work-g596-narsia/src/main/java/ru/mipt/java2016/homework/g596/narsia/task1/MyCalculator.java

+                            OPENING_BRACKET, CLOSING_BRACKET, FIRST, INVALID }
+
+
+    private SymbolType whatIsIt(Character symbol) {


Не слишком-то похоже на понятное название
Почему принимает Character?

DanAnastasyev · 2017-03-15T18:56:19Z

...work-g596-narsia/src/main/java/ru/mipt/java2016/homework/g596/narsia/task1/MyCalculator.java

+    }
+
+
+    private int getCode(char first, char second) {


Как говорили Великие:

А какой именно код возвращает эта функция? Лично мне сходу вообще непонятна семантика этой функции, в которой к тому же ещё и вложенные switch блоки, а значения кодов - тайна, покрытая мраком и нигде не задокументированная.

Почему не сделать enum вместо цифр? С ним назначение функции слегка прояснится. Заодно можно будет переименовать getCode во что-нибудь более определенное

DanAnastasyev · 2017-03-15T18:57:35Z

...work-g596-narsia/src/main/java/ru/mipt/java2016/homework/g596/narsia/task1/MyCalculator.java

+        Character curSymbol;
+        Character prevSymbol = '~';
+        Stack<Character> texas = new Stack<>();
+        StringBuilder california = new StringBuilder(expression.length());


То, что в статье, которую ты скинул, есть слова Техас и Калифорния, ещё не значит, что их нужно использовать, как названия переменных. Придумай что-нибудь более говорящее

1 task

d449e5f

Second attempt

605a5a4

DanAnastasyev reviewed Mar 12, 2017

View reviewed changes

String -> StringBuilder

ed20414

ignorer requested changes Mar 14, 2017

View reviewed changes

nrsxx added 3 commits March 15, 2017 06:03

Fixes in total + description file added

251fdf2

Codestyle

820b723

Codestyle last time

1f82f19

DanAnastasyev reviewed Mar 15, 2017

View reviewed changes

nrsxx added 2 commits March 16, 2017 21:23

Fixes

a152601

Codestyle fixes

6069427

ignorer approved these changes Mar 21, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1 task #346

1 task #346

nrsxx commented Mar 1, 2017 •

edited

Loading

ignorer commented Mar 4, 2017

DanAnastasyev Mar 12, 2017

ignorer left a comment

ignorer Mar 14, 2017

ignorer Mar 14, 2017

ignorer Mar 14, 2017

ignorer Mar 14, 2017

ignorer Mar 14, 2017

ignorer Mar 14, 2017

ignorer Mar 14, 2017

ignorer Mar 14, 2017

ignorer Mar 14, 2017

ignorer Mar 14, 2017

DanAnastasyev Mar 15, 2017

DanAnastasyev Mar 15, 2017

DanAnastasyev Mar 15, 2017

DanAnastasyev Mar 15, 2017

DanAnastasyev Mar 15, 2017

DanAnastasyev Mar 15, 2017

		OPENING_BRACKET, CLOSING_BRACKET, FIRST, INVALID }


		private SymbolType whatIsIt(Character symbol) {

1 task #346

Are you sure you want to change the base?

1 task #346

Conversation

nrsxx commented Mar 1, 2017 • edited Loading

ignorer commented Mar 4, 2017

Choose a reason for hiding this comment

ignorer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrsxx commented Mar 1, 2017 •

edited

Loading