SolV1 by TomaszKrzywicki · Pull Request #160 · mate-academy/js_validate_email

TomaszKrzywicki · 2025-09-24T18:50:53Z

No description provided.

mateacademy-ai-mentor

Review Summary

I'm requesting changes because there is a critical blocker that prevents the code from running.

Critical blocker (src/validateEmail.js, around line 28): The forbidden-character regex literal includes an unescaped /, causing a SyntaxError at module load time. This breaks the test run and functionality. Why: an unescaped / terminates the regex literal early. How to fix: either escape the slash (/[!$%&'*+\/=?^{}|~]/) or remove this redundant check and rely on the allow-list ^[A-Za-z0-9._-]+$ (line ~24) which already excludes these characters.

Minor/quality improvements (not blocking):

The forbidden-character check is redundant with the allow-list; consider removing it to reduce risk of mistakes. Add a short comment if you keep both, explaining the intent.
You currently reject domains ending with a dot (domain.endsWith('.')). This is stricter than the spec (which only forbids starting with a dot). Either keep it and document the intention, or remove it to match the spec exactly.
Tests: please add cases for multiple @ (e.g., a@b@c.com), the exact example false@email, and iterate through the full forbidden set ! $ % & ' * + / = ? ^ { | } ~ for personal_info.

You’ve done a solid job on the local-part dot rules and main validations. Fixing the regex issue and tightening the tests will get this over the line.

✨ Please rate this review in the Platform's notification center¹. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button² in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag³.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

mateacademy-ai-mentor · 2025-09-24T18:56:51Z

src/validateEmail.js

+    return false;
+  }
+
+  if (/[!$%&'*+/=?^{}|~]/.test(personal)) {


Critical — regex syntax error (module will fail to load).

This line uses a regex literal that contains an unescaped /, which breaks the literal and will cause a SyntaxError at load time. You can either escape the slash or remove this check because the earlier allow-list already prevents these characters. Example fixes:

Escape the slash: if (/[!$%&'*+\/=?^{}|~]/.test(personal)) { ... }

Or remove lines 28-29 and rely on the allow-list at line 24.

Fixing this is required before tests can run. See the task forbidden-character list in the description.

mateacademy-ai-mentor · 2025-09-24T18:56:51Z

src/validateEmail.js

+    return false;
+  }
+
+  if (!/^[A-Za-z0-9._-]+$/.test(personal)) {


Redundant check — allow-list already enforces permitted characters.

This line enforces allowed characters for personal using ^[A-Za-z0-9._-]+$, which already excludes the forbidden characters listed in the task. The subsequent forbidden-character test (lines 28-29) is redundant and led to the syntax bug. Consider removing the second check or add a comment explaining why both are needed. Keeping only the allow-list simplifies the code and reduces risk of mistakes.

mateacademy-ai-mentor · 2025-09-24T18:56:51Z

src/validateEmail.js

+    return false;
+  }
+
+  if (!domain || domain.startsWith('.') || domain.endsWith('.')) {


Semantic note — domain ending with '.' is rejected (stricter than spec).

This line rejects domains that end with a dot (domain.endsWith('.')). The description explicitly forbids domains starting with a dot, but does not require rejecting trailing dots. Tests in the project expect user@mail to be invalid, so rejecting missing dot in domain is fine; however, decide whether rejecting trailing dot is intentional. If you want to exactly follow the spec, remove the domain.endsWith('.') check; otherwise keep it but add a comment documenting the choice.

mateacademy-ai-mentor · 2025-09-24T18:56:51Z

src/validateEmail.js

+    return false;
+  }
+
+  if (!domain.includes('.')) {


Domain dot presence check — compatible with tests though not explicitly in spec.

This check requires that the domain include a dot (domain.includes('.')). The description doesn't explicitly require a dot in the domain, but the provided tests expect addresses like 'user@mail' to be invalid. Keeping this check aligns the function with the test expectations. If you change this behavior, update tests accordingly.

mateacademy-ai-mentor · 2025-09-24T18:56:51Z

src/validateEmail.js

+  if (!personal || personal.startsWith('.') || personal.endsWith('.')) {
+    return false;
+  }
+
+  if (/\.{2,}/.test(personal)) {


Positive: handling of personal_info dot rules and double-dots.

The implementation correctly rejects empty personal, personal starting/ending with a dot, and consecutive dots via the checks on lines 16 and 20 — this matches the requirement that dots cannot be first/last or consecutive in the personal_info part. Good.

mateacademy-ai-mentor · 2025-09-24T18:56:52Z

src/validateEmail.test.js

+  it('should return false for invalid emails', () => {
+    expect(validateEmail('testmail.com')).toBeFalsy();


Missing test for multiple '@' and missing the exact example from the description. The invalid-emails block should include an assertion that multiple @ characters are invalid (requirement 3.1). Also add the exact example validateEmail('false@email') === false (requirement 3.5). Suggested additions (as separate expect statements):

expect(validateEmail('a@b@c.com')).toBeFalsy();

expect(validateEmail('false@email')).toBeFalsy();

Add these inside the invalid emails it block so they are run with the related cases.

mateacademy-ai-mentor · 2025-09-24T18:56:52Z

src/validateEmail.test.js

+    expect(validateEmail('user!@mail.com')).toBeFalsy();
+    expect(validateEmail('user$@mail.com')).toBeFalsy();


Coverage of forbidden characters in personal_info is incomplete. The tests currently assert only ! and $. The description lists the full forbidden set: ! $ % & ' * + / = ? ^ { | } ~. Add tests that iterate over this list and assert each makes the email invalid (requirement 3.2). Example approach: a small loop inside a test that constructs emails like user${ch}@mail.com and expects falsy — keeps tests concise and readable.

mateacademy-ai-mentor · 2025-09-24T18:56:52Z

src/validateEmail.test.js

+    expect(validateEmail('test@mail.com')).toBeTruthy();
+    expect(validateEmail('t@q.c')).toBeTruthy();


Good: valid-case tests already include several realistic examples including test@mail.com and t@q.c, which are explicitly listed in the description. Keep these positives — they satisfy requirement 1.6 for provided valid examples. Consider also adding an explicit test showing domain with hyphen/subdomain is accepted if you want additional coverage for domain rules (requirement 3.3), though it's optional since some domain cases are already covered.

mateacademy-ai-mentor

Review Summary

I’m requesting changes because a few checklist-required tests are missing, which is a blocker for task completion even though the validator implementation looks solid.

Critical issues to fix:

src/validateEmail.test.js: Add a negative test for multiple @ symbols (e.g., a@b@c.com) to verify the “exactly one @” requirement.
src/validateEmail.test.js: Include the explicit negative example false@email to confirm the domain must contain at least one dot.
src/validateEmail.test.js: Expand tests to cover the full forbidden-character set in the local part: ! $ % & ' * + / = ? ^ { | } ~. A simple loop over these characters is fine.
src/validateEmail.js (line with domain.endsWith('.')): Your code currently rejects domains that end with a dot, which is stricter than the spec. Either remove this check to match the spec, or keep it, add a brief English comment explaining the stricter policy, and add a corresponding negative test (e.g., user@mail.com. should be falsy) so code and tests align.

Minor suggestion (non-blocker): Replace the Polish inline comment with a short English note explaining the allow-list rationale for broader team readability.

You’ve done a great job implementing the main rules (single @, dot rules, allow-lists) and including realistic positive/negative cases. Once the above tests are added and the trailing-dot decision is aligned and documented, this will be ready to approve.

✨ Please rate this review in the Platform's notification center¹. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button² in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag³.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

mateacademy-ai-mentor · 2025-09-24T19:06:00Z

src/validateEmail.js

+  const parts = email.split('@');

-  if (email.match(validEmailMask)) {
-    return true;
+  if (parts.length !== 2) {
+    return false;


Splitting on @ and checking parts.length !== 2 correctly enforces exactly one @ in the email (missing or multiple @ will be rejected). Ensure your tests include a negative test for a multiple-@ input such as a@b@c.com so this behavior is explicitly validated.

mateacademy-ai-mentor · 2025-09-24T19:06:00Z

src/validateEmail.js

+  if (!personal || personal.startsWith('.') || personal.endsWith('.')) {
+    return false;


This check correctly rejects empty local part and local parts that start or end with a dot, which matches the spec requirements for personal_info.

mateacademy-ai-mentor · 2025-09-24T19:06:00Z

src/validateEmail.js

+  if (/\.{2,}/.test(personal)) {
+    return false;


The /{2,}/ (consecutive dots) test is present and will reject local parts with .., which satisfies the consecutive-dot rule in the specification.

mateacademy-ai-mentor · 2025-09-24T19:06:01Z

src/validateEmail.js

+  if (!/^[A-Za-z0-9._-]+$/.test(personal)) {
+    return false;


The allow-list regex /^[A-Za-z0-9._-]+$/ correctly permits only letters, digits, hyphen, underscore and dot in the local part and therefore excludes the forbidden characters specified in the description. The inline Polish comment notes this intention (consider adding a short English comment for wider team readability).

mateacademy-ai-mentor · 2025-09-24T19:06:01Z

src/validateEmail.js

+  if (!domain || domain.startsWith('.') || domain.endsWith('.')) {
+    return false;


This line rejects domains that start OR end with a dot. The spec required that domain cannot start with a dot, but it did not explicitly forbid a trailing dot. The domain.endsWith('.') part makes the validator stricter than the specification. Either remove the trailing-dot rejection or add a comment explaining why trailing dots are disallowed and ensure tests reflect that stricter policy.

mateacademy-ai-mentor · 2025-09-24T19:06:01Z

src/validateEmail.test.js

-  it(`should return 'true' for the valid email`, () => {
-    expect(validateEmail('test838@gmail.com.'))
-      .toBeTruthy();
+  it('should return false for invalid emails', () => {


Add a negative test for multiple @ symbols. The checklist explicitly requires verifying that exactly one @ is present (invalid if multiple). Add an expectation like:

expect(validateEmail('a@b@c.com')).toBeFalsy();

Place it in the invalid-emails block so both missing-@ and multiple-@ cases are covered.

mateacademy-ai-mentor · 2025-09-24T19:06:01Z

src/validateEmail.test.js

-      .toBeTruthy();
+  it('should return false for invalid emails', () => {
+    expect(validateEmail('testmail.com')).toBeFalsy();
+    expect(validateEmail('user@mail')).toBeFalsy();


Include the exact negative example from the description: false@email. Add a test such as:

expect(validateEmail('false@email')).toBeFalsy();

This example is explicitly mentioned in the task description and should be present in the invalid tests.

mateacademy-ai-mentor · 2025-09-24T19:06:01Z

src/validateEmail.test.js

+  it('should return false for invalid emails', () => {
+    expect(validateEmail('testmail.com')).toBeFalsy();
+    expect(validateEmail('user@mail')).toBeFalsy();
+    expect(validateEmail('user!@mail.com')).toBeFalsy();


The tests check a couple of forbidden characters in the local part (! and $), but the spec lists a full set of forbidden characters: ! $ % & ' * + / = ? ^ { | } ~. Please add tests that assert the local part is rejected for each of these characters. You can add them explicitly or iterate through the list inside a test (e.g., a loop that checks each character yields a falsy result).

mateacademy-ai-mentor · 2025-09-24T19:06:01Z

src/validateEmail.test.js

+  it('should return true for valid emails', () => {
+    expect(validateEmail('test@mail.com')).toBeTruthy();
+    expect(validateEmail('t@q.c')).toBeTruthy();


Good: the positive tests include the required examples 'test@mail.com' and 't@q.c' and other realistic valid cases — this aligns with the description's examples and the hint to prioritize realistic cases. Keep these examples as they are.

mateacademy-ai-mentor · 2025-09-24T19:06:01Z

src/validateEmail.test.js

  });

-  // write more tests here
+  it('should return false for invalid domain chars', () => {


The file already contains tests for invalid domain characters (e.g., user@mail!.com and user@ma#il.com). Consider also adding a test to reflect the intended behavior for domains that end with a dot only if you decide to enforce that rule; currently the implementation rejects trailing dots in domain (see src/validateEmail.js). If trailing-dot rejection is intentional, add a test such as:

expect(validateEmail('user@mail.com.')).toBeFalsy();

Otherwise, do not add this test and remove the trailing-dot check from the implementation so tests and code match the spec.

mateacademy-ai-mentor

Review Summary

Educational approval: moving you forward while highlighting a few refinements to align code, comments, and tests.

What you did well:

The core validation logic in src/validateEmail.js is solid: exactly one @, non-empty parts, correct local-part rules (no leading/trailing dot, no consecutive dots, allow-list), and sensible domain checks. Your tests cover many essentials, including the full forbidden-character set in the local part.

What to improve (actionable):

In src/validateEmail.js lines 35–38: the comment says a stricter policy disallows a trailing dot in the domain, but the code doesn’t enforce domain.endsWith('.'). Either add if (domain.endsWith('.')) return false; and a test asserting validateEmail('user@mail.com.') is falsy, or update/remove the comment to explicitly allow trailing dots. Keep the comment in English and make the policy explicit so implementation and tests stay consistent.
In src/validateEmail.test.js: add missing checks required by the spec:
- Non-empty parts around @: expect(validateEmail('@domain.com')).toBeFalsy(); and expect(validateEmail('user@')).toBeFalsy();
- Domain must not start with a dot: expect(validateEmail('abc@.domain.com')).toBeFalsy();
- Add a positive example covering hyphens in domain and mixed local chars: test-name_123@mail-domain.co.uk.
Minor readability: convert Polish inline comments to concise English (e.g., “must contain exactly one ‘@’”, “double dots disallowed”) for team clarity.

You’re very close—tighten the trailing-dot policy and add the few missing tests, and you’ll have a clear, consistent, and well-covered solution. Keep up the great work!

✨ Please rate this review in the Platform's notification center¹. I will be grateful for your feedback! ✨

Rate AI review example ↩

mateacademy-ai-mentor · 2025-09-24T19:17:43Z

src/validateEmail.js


-  if (email.match(validEmailMask)) {
-    return true;
+  // musi być dokładnie jeden @


The inline comment at this line is written in Polish: // musi być dokładnie jeden @. Consider replacing it with a short English comment like // must contain exactly one '@' for broader team readability. This is a maintenance/readability suggestion — the check itself is implemented correctly just below (parts.length !== 2).

mateacademy-ai-mentor · 2025-09-24T19:17:43Z

src/validateEmail.js

+  if (/\.{2,}/.test(personal)) {
+    return false;
+  } // podwójne kropki niedozwolone
+
+  if (!/^[A-Za-z0-9._-]+$/.test(personal)) {
+    return false;


Good: this block correctly enforces the local (personal_info) constraints required by the task: non-empty, not starting/ending with ., disallowing consecutive dots, and restricting characters to [A-Za-z0-9._-]. These checks align with the specification (no leading/trailing dot, no double dots, allow-list of characters). No changes required here unless you want different error messages or logging.

mateacademy-ai-mentor · 2025-09-24T19:17:43Z

src/validateEmail.js

+  // stricter policy: końcowa kropka w domenie niedozwolona
+  if (!/^[A-Za-z0-9.-]+$/.test(domain)) {
+    return false;
+  }


Inconsistent comment vs behaviour. The comment says a stricter policy disallows a trailing dot in the domain, but there is no code enforcing domain to not end with .. Decide which policy you want and reconcile:

If you want to disallow trailing dot: add if (domain.endsWith('.')) return false; and add a test asserting user@mail.com. is invalid.

If you want to allow trailing dot: remove or update this comment so it doesn't claim a stricter policy.
Make the policy explicit in English so tests and implementation remain consistent.

mateacademy-ai-mentor · 2025-09-24T19:17:43Z

src/validateEmail.test.js

+    const validEmails = [
+      'test@mail.com',
+      't@q.c',
+      'user.name_123@domain.com',
+      'a-b_c.d@sub.domain.com',
+      'user@mail.com.', // końcowa kropka w domenie jest dozwolona
+    ];


The validEmails array includes user@mail.com. and a Polish comment that says a trailing dot in the domain is allowed. This contradicts the implementation comment (in src/validateEmail.js) that refers to a stricter policy disallowing trailing dot. Decide on the intended policy and make tests reflect it. If trailing dots should be disallowed, remove user@mail.com. from validEmails and add an explicit invalid test (for example expect(validateEmail('user@mail.com.')).toBeFalsy();). If trailing dots are allowed keep the test but update (or remove) the misleading implementation comment. Also add the required realistic positive case test-name_123@mail-domain.co.uk to cover hyphens in domain and mixed allowed chars in local part.

mateacademy-ai-mentor · 2025-09-24T19:17:43Z

src/validateEmail.test.js

+  it('should return false for invalid emails', () => {
+    // brak @
+    expect(validateEmail('testmail.com')).toBeFalsy();
+    // wiele @
+    expect(validateEmail('a@b@c.com')).toBeFalsy();
+    // brak kropki w domenie
+    expect(validateEmail('false@email')).toBeFalsy();


Add tests that confirm @ splits the string into two non-empty parts. The spec requires both personal_info and domain to be non-empty. Insert assertions such as:

expect(validateEmail('@domain.com')).toBeFalsy();

expect(validateEmail('user@')).toBeFalsy();
Place these inside the invalid-emails test block near the other @ related checks so this explicit rule is covered.

mateacademy-ai-mentor · 2025-09-24T19:17:43Z

src/validateEmail.test.js

+  it('should return false for invalid emails', () => {
+    // brak @
+    expect(validateEmail('testmail.com')).toBeFalsy();
+    // wiele @
+    expect(validateEmail('a@b@c.com')).toBeFalsy();
+    // brak kropki w domenie
+    expect(validateEmail('false@email')).toBeFalsy();


The spec requires that the domain must not start with a dot. Add an explicit negative test like expect(validateEmail('abc@.domain.com')).toBeFalsy(); (this is currently missing). Place it in the invalid-emails block near the other domain-related negative tests.

mateacademy-ai-mentor · 2025-09-24T19:17:43Z

src/validateEmail.test.js

+    // zakazane znaki w personal_info
+    const forbiddenChars = "!$%&'*+/=?^{}|~";
+
+    forbiddenChars.split('').forEach(char => {
+      expect(validateEmail(`user${char}name@mail.com`)).toBeFalsy();
+    });


Good: the forbiddenChars string at this line contains the full set required by the checklist (! $ % & ' * + / = ? ^ { | } ~) and the loop covers each character in the local part, asserting they cause false. Keep this — it satisfies the checklist requirement for forbidden-character tests.

mateacademy-ai-mentor · 2025-09-24T19:17:43Z

src/validateEmail.test.js

+    // domena z niedozwolonym znakiem
+    expect(validateEmail('user@mail!.com')).toBeFalsy();
+    expect(validateEmail('user@ma#il.com')).toBeFalsy();


You already test some invalid characters in the domain (e.g., user@mail!.com, user@ma#il.com), which is good. After adding the domain-start-dot test and the empty-part tests referenced above, the invalid-domain cases will be fully covered per the checklist.

SolV1

1a524e3

mateacademy-ai-mentor suggested changes Sep 24, 2025

View reviewed changes

SolV2

c4fa80f

TomaszKrzywicki requested a review from mateacademy-ai-mentor September 24, 2025 19:00

mateacademy-ai-mentor suggested changes Sep 24, 2025

View reviewed changes

TomaszKrzywicki added 2 commits September 24, 2025 21:09

SolV3

c05155c

SolV4

03f4753

TomaszKrzywicki requested a review from mateacademy-ai-mentor September 24, 2025 19:11

mateacademy-ai-mentor approved these changes Sep 24, 2025

View reviewed changes

		it('should return false for invalid emails', () => {
		expect(validateEmail('testmail.com')).toBeFalsy();

		expect(validateEmail('user!@mail.com')).toBeFalsy();
		expect(validateEmail('user$@mail.com')).toBeFalsy();

		expect(validateEmail('test@mail.com')).toBeTruthy();
		expect(validateEmail('t@q.c')).toBeTruthy();

		if (!personal \|\| personal.startsWith('.') \|\| personal.endsWith('.')) {
		return false;

		if (!domain \|\| domain.startsWith('.') \|\| domain.endsWith('.')) {
		return false;

Conversation

TomaszKrzywicki commented Sep 24, 2025

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

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!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

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!

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!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants