Remove repeat hyphen #474

bodom91 · 2016-08-28T13:19:17Z

This change is

php-coder · 2016-08-28T13:44:24Z

src/main/java/ru/mystamps/web/controller/CategoryController.java

@@ -65,6 +65,8 @@ protected void initBinder(WebDataBinder binder) {
 		StringTrimmerEditor editor = new StringTrimmerEditor(false);
 		binder.registerCustomEditor(String.class, "name", editor);
 		binder.registerCustomEditor(String.class, "nameRu", editor);
+		binder.registerCustomEditor(String.class, "name", new CustomCategoryNameEditor());
+		binder.registerCustomEditor(String.class, "nameRu", new CustomCategoryNameEditor());


Is it possible to use the same instance of the editor for both fields? In other words, is the class thread-safe?

php-coder · 2016-08-28T13:57:53Z

плюс не хватает интеграционных тестов

php-coder · 2016-08-28T13:59:19Z

Трэвис, помимо всего прочего, также жалуется на отсутствие хедера: https://github.com/php-coder/mystamps/wiki/check-license-header

php-coder · 2016-08-28T16:17:41Z

src/main/java/ru/mystamps/web/controller/CategoryController.java

@@ -62,9 +62,9 @@

 	@InitBinder("addCategoryForm")
 	protected void initBinder(WebDataBinder binder) {
-		StringTrimmerEditor editor = new StringTrimmerEditor(false);


Вот почему у тебя тесты падать начали -- ты зря удалил этот эдитор -- он для другого и нужен здесь.

Я добавил тримы непосредственно в реализацию кастомэдитора и все чеки прошли.
Возможно, я неверно трактовал эту проверку, но исходя из того что мы везде тримим пробелы, я решил это вынести.
И еще один момент, если мы оставим этот эдитор, то у нас не будет это все корректно работать , я пока не капнул почему, но моя догадка, что ЛУЧШЕ использовать только один эдитор.
Как ты считаешь ??

Ни разу не имел дел с более чем одним валидатором, поэтому придется тебе экспериментировать и проверять что/как можно, а что нельзя. Если смотреть с точки зрения принципа единой ответственности (SRP), то лучше держать проверки отдельно -- и реюзать проще и логика не "слипается".

Ну вот я и эксперементировал , и если делать два подряд - выполняется только последний......
Еще попробую покрутить, но пока такие результаты.

Note: Only one single registered custom editor per property path is supported. (c) http://docs.spring.io/spring/docs/current/javadoc-api/org/springframework/validation/DataBinder.html#registerCustomEditor-java.lang.Class-java.lang.String-java.beans.PropertyEditor-

Тогда надо либо попробовать какие-либо конвертеры использовать, либо использовать паттерн декоратор (и вкладывать один в другой).

If you want to re-implement StringTrimmerEditor and don't re-use this class then we don't need this flags because we're not using them, right?

Then, implement our own editor that do a trimming and also replace hyphens. Hm. Not ideal, but we have tests after all, so we can refactor this any time.

And also add the comment why we're not using StringTrimmerEditor.

Yes , i will not use StringTrimEditor class , but i need use boolean .
I need it because we want separate functionality CustomEditor.
If we have true - trim String , if we have false or empty value - not trim String.
Do you agree ?

You're going to add flag that always will have value true, right? As far I understand we'll use this converter for category and country names. For them we always need to trim the string, right?

And now I'm still don't understand why we need this flag.

Not exactly ))
Maybe i do , and you will see ... )

php-coder · 2016-08-29T16:19:51Z

src/main/resources/ru/mystamps/i18n/ValidationMessages.properties

@@ -24,6 +24,8 @@ value.too-short = Value is less than allowable minimum of {min} characters
 value.too-long = Value is greater than allowable maximum of {max} characters
 value.invalid-length = Value length must be equals to {max} characters
 value.hyphen = Value must not start or end with hyphen
+value.hyphen_repeat_en = Value must not contain repetition of hyphen
+value.hyphen_repeat_ru = Value must not contain repetition of hyphen


Why we need such duplication? Please, join them and remove language postfix.

And let's rename it to repeating_hyphen.

php-coder · 2016-08-29T20:43:53Z

66 message from 1 PR ..

It's a normal working process. Especially when we're discussed about how to achieve our goal.

php-coder · 2016-09-14T15:52:04Z

src/main/java/ru/mystamps/web/controller/editor/ReplaceRepeatingSpacesEditor.java

+		}
+
+		if (text.contains("  ")) {
+			text = text.replaceAll(REPEATING_SPACES_REGEXP, " ");


AFAIR replaceAll will compile the regexp on every method's invocation. Let's use Pattern and compile the regexp only once.

php-coder

We're closed to finishing this issue! :)

php-coder · 2016-09-15T10:40:17Z

src/main/java/ru/mystamps/web/validation/ValidationRules.java

@@ -42,6 +42,9 @@
 	public static final String CATEGORY_NAME_EN_REGEXP = "[- a-zA-Z]+";
 	public static final String CATEGORY_NAME_RU_REGEXP = "[- а-яёА-ЯЁ]+";
 	public static final String CATEGORY_NAME_NO_HYPHEN_REGEXP = "[ \\p{L}]([- \\p{L}]+[ \\p{L}])*";
+	// CheckStyle: ignore LineLength for next 1 line
+	@SuppressWarnings("PMD")


Which exactly warning from PMD you're disabling? (Hint: git grep @SuppressWarnings to find examples)

Also, if we're using @SuppressWarnings we can suppress CheckStyle's warning too: @SuppressWarnings({"PMD.LongVariable", "checkstyle:linelength"})

Where can i read about SuppressWarnings ??

I don't know... I didn't write docs for that yet (tickets already exist).

php-coder · 2016-09-15T10:47:12Z

src/main/java/ru/mystamps/web/controller/editor/ReplaceRepeatingSpacesEditor.java

+		}
+
+		if (text.contains("  ")) {
+			text = text.replaceAll(String.valueOf(REP_SPACES), " ");


You're missed the point: we don't want to use String.replaceAll at all. Instead we should use Pattern and Matcher to do the same.

See also: http://stackoverflow.com/questions/1466959/string-replaceall-vs-matcher-replaceall-performance-differences

php-coder · 2016-09-15T10:47:44Z

src/main/java/ru/mystamps/web/controller/editor/ReplaceRepeatingSpacesEditor.java

+@RequiredArgsConstructor
+public class ReplaceRepeatingSpacesEditor extends PropertyEditorSupport {
+	private static final String REPEATING_SPACES_REGEXP = "[ ]{2,}";
+	private static final Pattern REP_SPACES = Pattern.compile(REPEATING_SPACES_REGEXP);


Rename this member to REPEATING_SPACES

php-coder · 2016-09-15T10:48:14Z

src/main/java/ru/mystamps/web/controller/editor/ReplaceRepeatingSpacesEditor.java

+ */
+@RequiredArgsConstructor
+public class ReplaceRepeatingSpacesEditor extends PropertyEditorSupport {
+	private static final String REPEATING_SPACES_REGEXP = "[ ]{2,}";


I don't see why we need this as a separate constant. Inline this string.

php-coder

Let me know (by sending e-mail or leaving comment here) once you update this branch (because I don't receive notifications and have to check for any updated manually).

bodom91 · 2016-09-19T14:39:09Z

Ok , sorry for delay , i m on holiday )

php-coder · 2016-09-19T14:49:45Z

@bodom91 No problem, I just wanted to say that I stopped polling this PR every day and now I'm waiting when you ping me.

php-coder · 2016-09-19T14:50:11Z

@bodom91 Have a great time there!

bodom91 · 2016-09-23T07:56:47Z

You can check it. =)

php-coder · 2016-09-23T08:20:54Z

@bodom91 Ok, thank you! It's in my todo list for today.

php-coder · 2016-09-23T17:22:21Z

LGTM. Please, squash commits.

php-coder · 2016-09-23T17:25:31Z

src/main/java/ru/mystamps/web/controller/CategoryController.java

+		// We can't use StringTrimmerEditor here because "only one single registered custom editor per property path is supported".
+		ReplaceRepeatingSpacesEditor nameEditor = new ReplaceRepeatingSpacesEditor(true);
+		binder.registerCustomEditor(String.class, "name", nameEditor);
+		binder.registerCustomEditor(String.class, "nameRu", nameEditor);


BTW, it would be good if you also will change the name from nameEditor to editor. I don't see a reason for such renaming.

php-coder · 2016-09-26T12:01:10Z

Merged in c8f2dca commit.

Thank you for your patience and contribution!

php-coder added the in progress label Aug 28, 2016

php-coder reviewed Aug 28, 2016
View reviewed changes

bodom91 force-pushed the gh465_Prohibit_spec_char_collection branch from 4e16819 to 613fe42 Compare August 28, 2016 15:39

php-coder reviewed Aug 28, 2016
View reviewed changes

bodom91 force-pushed the gh465_Prohibit_spec_char_collection branch 5 times, most recently from 22760b2 to 8e3be2d Compare August 29, 2016 13:45

php-coder reviewed Aug 29, 2016
View reviewed changes

bodom91 force-pushed the gh465_Prohibit_spec_char_collection branch 2 times, most recently from 23a9bb8 to 5a546bf Compare August 29, 2016 19:39

php-coder reviewed Sep 14, 2016
View reviewed changes

bodom91 force-pushed the gh465_Prohibit_spec_char_collection branch from 5a546bf to af6a087 Compare September 14, 2016 22:45

php-coder requested changes Sep 15, 2016

View reviewed changes

php-coder reviewed Sep 19, 2016

View reviewed changes

bodom91 force-pushed the gh465_Prohibit_spec_char_collection branch 3 times, most recently from 070aa92 to 1edb4ed Compare September 22, 2016 11:12

php-coder approved these changes Sep 23, 2016

View reviewed changes

php-coder reviewed Sep 23, 2016

View reviewed changes

Prohibit special chars collection

ae8a699

bodom91 force-pushed the gh465_Prohibit_spec_char_collection branch from 1edb4ed to ae8a699 Compare September 26, 2016 10:43

php-coder added resolution/merged and removed in progress labels Sep 26, 2016

php-coder closed this Sep 26, 2016

Remove repeat hyphen #474

Remove repeat hyphen #474

Uh oh!

Conversation

bodom91 commented Aug 28, 2016 • edited by php-coder Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

php-coder commented Aug 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

php-coder commented Aug 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bodom91 Aug 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

php-coder commented Aug 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

php-coder left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

php-coder left a comment

Choose a reason for hiding this comment

Uh oh!

bodom91 commented Sep 19, 2016

Uh oh!

php-coder commented Sep 19, 2016

Uh oh!

php-coder commented Sep 19, 2016

Uh oh!

bodom91 commented Sep 23, 2016

Uh oh!

php-coder commented Sep 23, 2016

Uh oh!

php-coder commented Sep 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

php-coder commented Sep 26, 2016

Uh oh!

bodom91 commented Aug 28, 2016 •

edited by php-coder

Loading

php-coder commented Aug 28, 2016 •

edited

Loading

bodom91 Aug 29, 2016 •

edited

Loading