MayneKeen / Rubiks_Cube

0 stars 0 forks source link

Замечания #1

Open maximpetrov opened 6 years ago

maximpetrov commented 6 years ago
  1. Вместо строк для хранения значений цветов лучше использовать enum.
  2. Хранить временные переменные в полях класса - плохая идея (whiteCount и т.д.).
  3. Зачем Вам switch по times в colourCounter, если можно просто вычесть times?
  4. Можно было бы избежать и switch по colour, если вместо 6 переменных сделать Map.
  5. Здесь и везде switch Вы используете неправильно: если не писать break в конце каждого случая, то все последующие также выполнятся. Кстати, фигурные скобки внутри case лишние.
  6. randomize(), даже если правильно воспользоваться switch, принципиально неверно пытается собрать кубик: у кубика элементы, соседние через ребро (находящиеся на одном маленьком кубике), не могут быть одного цвета, а Вы специально это делаете.
  7. randomColour тоже очень странно выглядит. Во-первых, зачем Вы генерируете случайное число от 0 до 99, а потом проверяете его принадлежность к 1 из 6 примерно равных диапазонов? Почему нельзя сразу сгенерировать число от 0 до 5? Во-вторых, рекурсию лучше заменить циклом, иначе можно получить переполнение стека вызовов, если долго не будет получаться нужное число. А вообще, можно сделать так, чтобы случайный цвет выбирался только из доступных.
  8. Какой из углов для каждой грани вы считаете за (0,0)? Судя по всему, для разных поворотов Вы считаете по-разному.
  9. Не хватает поворотов вокруг третьей оси.
  10. Тесты должны использовать assert... методы, а не кидать исключения.
MayneKeen commented 6 years ago

(8 замечание) На каждую грань смотрю как бы сверху, как мы смотрим на лицевую. За (0,0) берется левый верхний угол, просто в таком случае не все грани связаны общими индексами с другими (к примеру, задняя, нижняя), потому в методах могут идти разные индексы.

maximpetrov commented 6 years ago

В том то и дело, что Вы в методах поворота везде меняете [0][0] с [0][0] и т.д, а так можно делать только для поворотов вокруг одной оси. Вокруг другой оси индексы уже будут другие. Вообще Ваше "смотрим сверху как на лицевую" как раз сильно зависит от того, как мы поворачивали кубик, чтобы смотреть на неё так и только вокруг этой оси поворота всё и будет работать.

Пример, чтобы убедиться, что у Вас не работает. Сделаем 2 раза turnRight1(), тогда [0][0] с передней грани перейдёт за заднюю, тоже на [0][0]. После этого тоже 2 раза turnUp1() из этот задний [0][0] вернётся на своё место [0][0] на передней грани. Это по Вашему коду. А если Вы кубик в руки возьмёте или в голове представите, то ничего такого не случится, [0][0] так и останется на задней грани.

MayneKeen commented 6 years ago

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

MayneKeen commented 6 years ago

я уже почти все замечания исправил кроме этого

maximpetrov commented 6 years ago

Расставлять индексы в грани можно как угодно, это не имеет значения. Главное аккуратно прописать все повороты, не нарушая принятого порядка нумерации.

maximpetrov commented 6 years ago

Если я правильно понял, то теперь в randomize() Вы пытаетесь сгенерировать для каждой стороны свой цвет (правда во второй половине почему-то е используете результат из temp). Это, конечно, спасает Вас от ситуации, когда Вы пытались один и тот же цвет присвоить смежным граням, но это далеко не единственное условие для того, чтобы сгенерировать правильный кубик. Например, все цвета попарно являются цветами противоположных граней и не могут быть на смежных гранях. Кроме того, даже если Вы возьмёте реальный кубик Рубика, и переставите два соседних кубика (не перестановками, а разобрав его), то его будет не собрать. Поэтому гораздо проще, чем учитывать все условия, просто взять собранный кубик и случайное число раз повернуть его случайные слои в случайные стороны.

MayneKeen commented 6 years ago

Спасибо, сделал как Вы сказали.

MayneKeen commented 6 years ago

А есть ли смысл еще исправлять этот класс или лучше начать делать следующее задание?

maximpetrov commented 6 years ago

Да, естественно имеет смысл доделывать это класс, сейчас в нём как минимум неправильно работают повороты. Оценка, выставленная за это задание на аттестации, является промежуточной по состоянию на тот момент и она может (и должна) быть улучшена.

MayneKeen commented 6 years ago

Понял. В таком случае хочу Вас попросить помочь мне с исправлением методов поворотов Я вроде как расписывал все на бумаге, но все равно получилось как-то не оч Вы могли бы что-нибудь посоветовать?

maximpetrov commented 6 years ago
  1. Раз у Вас уже почти реализованы повороты, аккуратно нарисуйте ещё раз на бумаге как всё должно быть, не забыв зафиксировать положение [0][0] на каждой грани.
  2. Можно сделать по-другому: научиться делать повороты только одного слоя и повороты кубика целиком (какие-то грани просто переставятся, какие-то придётся ещё и транспонировать). После этого можно все повороты слоёв выразить через поворот кубика таким образом, чтобы нужный слой встал на место слоя, для которого реализован поворот, собственно поворот этого слоя и поворот кубика обратно.
MayneKeen commented 6 years ago

У меня сломался гит и он почему-то не хочет создавать,а тем более и загружать коммиты для этого проекта. Я даже вручную через программу git bash пробовал, ничего не выходит. Поэтому пока что начал (а точнее к текущему моменту почти закончил) вторую задачу

maximpetrov commented 6 years ago

Попробуйте обновить IDEA и git.

MayneKeen commented 6 years ago

Не помогло.. Можете, пожалуйста, помочь с вторым заданием? Не очень понимаю, как должны выглядеть тесты для консольной утилиты

MayneKeen commented 6 years ago

Напишите, пожалуйста, что мне ещё нужно поправить?

maximpetrov commented 6 years ago
  1. Методы transposeLeft и transposeRight работают некорректно: записав, например в (1,0) содержимое ячейки (0,1) Вы теряете содержимое ячейки (1,0) и в (2,1) будет записано содержимое ячейки (0,1), а не (1,0). Также я не вижу причин делать эти методы публичными. Кроме того, то, что Вы пытаетесь сделать, это всё-таки поворот, а не транспонирование.
  2. В методе randomize a генерируется в пределах от 0 до 5, а в switch у Вас рассматриваются только случаи от 0 до 3.
  3. Здесь явно опечатка. https://github.com/MayneKeen/Rubiks_Cube/blob/f7fa474d465c6ada22474e377b77d0f3d957a31b/src/com/keenpro/Rubicks_cube.java#L148
  4. У Вас всё ещё есть ошибки в индексах в некоторых поворотах (turnUp и turnDown). Кроме того, во всех методах поворота целая грань поворачивается в противоположную сторону.
  5. Для методов поворота граней, перпендикулярных наблюдателю (turnPlaneLeft и turnPlaneRight), нет метода для поворота целого кубика сразу.
  6. Не самая хорошая идея возвращать приватные массивы без копирования в методах получения состояния грани: снаружи можно их изменить и нарушить целостность кубика. Лучше копировать их или вообще вместо метода, возвращающего грань целиком, сделать метод для запроса конкретной клетки.
  7. Тесты у Вас сейчас все закомментированы. Раскомментируйте их и приведите в порядок.