Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SequenceRule refactoring. #23

Merged

Conversation

serac
Copy link
Member

@serac serac commented Oct 20, 2014

  1. Rename SequenceRule->IllegalSequenceRule for consistency with other
    rules that have negative semantics.
  2. Rename SequenceData->IllegalSequenceData for consistency.
  3. Add IllegalSequence type to describe sequences (e.g. differing case,
    variant forms).
  4. Create EnIllegalSequenceData enum with common sequences (alpha, numeric,
    keyboard).

1. Rename SequenceRule->IllegalSequenceRule for consistency with other
   rules that have negative semantics.
2. Rename SequenceData->IllegalSequenceData for consistency.
3. Add IllegalSequence type to describe sequences (e.g. differing case,
   variant forms).
4. Create EnIllegalSequenceData enum with common sequences (alpha, numeric,
   keyboard).
{
return this.original.length();
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would call this class 'Sequence', there's nothing inherently illegal about it. It may have other uses outside of the IllegalSequenceRule.
I don't find the term original or variant to be meaningful and I don't think providing these as Strings versus char[][] provides any more clarity.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would call this class 'Sequence'

Fair enough

I don't find the term original or variant to be meaningful and I don't think providing these as Strings versus char[][] provides any more clarity.

Agree on the naming feedback, but the use of Strings goes much farther than simply character specification. The use of the IllegalSequence class clarified if not simplified the implementation of IllegalSequenceRule.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, FWIW I don't think char[][] is any clearer. It's a wash.

@serac
Copy link
Member Author

serac commented Dec 1, 2014

These changes got merged with #22.

@serac serac closed this Dec 1, 2014
@serac serac reopened this Dec 1, 2014
serac added a commit that referenced this pull request Dec 1, 2014
@serac serac merged commit b9ace8e into vt-middleware:international Dec 1, 2014
@serac serac deleted the character-sequence-data-enum branch December 1, 2014 15:48
@serac
Copy link
Member Author

serac commented Dec 1, 2014

I will address the suggestions you made above on the international branch.

serac added a commit that referenced this pull request Dec 1, 2014
Name changes as inspired by discussion on #23.
serac added a commit that referenced this pull request Dec 1, 2014
Name changes as inspired by discussion on #23.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants