Skip to content

Conversation

@jasukej
Copy link
Contributor

@jasukej jasukej commented Jun 3, 2025

PR Type

Enhancement


Description

  • Use word count for min/max validation

  • Introduce countWords helper function

  • Update error messages to mention words

  • Comment-out regex pattern validation stub


Changes walkthrough 📝

Relevant files
Enhancement
application_controller.ts
Switch string length validation to word count                       

functions/src/controllers/application_controller.ts

  • Added countWords helper to count words
  • Switched min/max checks to use word count
  • Updated error messages to reference words
  • Commented-out regex pattern validation code
  • +27/-4   

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @jasukej jasukej requested a review from hib4 June 3, 2025 05:25
    @github-actions
    Copy link

    github-actions bot commented Jun 3, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Performance

    The countWords function is invoked twice in the same validation block, causing redundant computation. Consider calling it once, storing the result in a local variable, and reusing it for both the min/max checks.

    if (validation.minLength && countWords(fieldValue) < validation.minLength) {
      errors.push({
        field_id: `${question.id}`,
        message: `Must be at least ${validation.minLength} word(s)`,
      });
    } else if (validation.maxLength && countWords(fieldValue) > validation.maxLength) {
    Dead Code

    The commented-out regex validation stub spans many lines without clear guidance. Either implement the pattern validation with tests or remove the block to keep the codebase clean.

    // check regex pattern
    // if (validation.pattern) {
    //   try {
    //     const regex = new RegExp(validation.pattern);
    //     if (!regex.test(fieldValue)) {
    //       errors.push({
    //         field_id: `${question.id}`,
    //         message: `Value does not match the required pattern`,
    //       });
    //     }
    //   } catch (regexError) {
    //     errors.push({
    //       field_id: `${question.id}`,
    //       message: `Invalid validation pattern configured`,
    //     });
    //   }
    // }

    @github-actions
    Copy link

    github-actions bot commented Jun 3, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Use explicit nullish checks

    Use an explicit nullish check (!= null) for validation.minLength so that a value of
    0 is correctly honored and not skipped.

    functions/src/controllers/application_controller.ts [505]

    -if (validation.minLength && countWords(fieldValue) < validation.minLength) {
    +if (validation.minLength != null && countWords(fieldValue) < validation.minLength) {
    Suggestion importance[1-10]: 6

    __

    Why: Using != null ensures a validation.minLength of 0 is not skipped, preventing unintended bypass of zero-length checks.

    Low
    Extract helper to module scope

    Move the countWords helper out of validateStringValue to module scope so it's not
    redeclared on every function call.

    functions/src/controllers/application_controller.ts [499-502]

    +// at top of file
     function countWords(text: string): number {
       if (!text || text.trim() === "") return 0;
       return text.trim().split(/\s+/).length;
     }
    Suggestion importance[1-10]: 6

    __

    Why: Moving countWords out of validateStringValue prevents redeclaration on each call and enhances performance.

    Low
    Precompute word count

    Precompute the word count once before the conditional checks to avoid redundant
    calls to countWords and improve readability.

    functions/src/controllers/application_controller.ts [505-514]

    -if (validation.minLength && countWords(fieldValue) < validation.minLength) {
    +const wordCount = countWords(fieldValue);
    +if (validation.minLength && wordCount < validation.minLength) {
       errors.push({
         field_id: `${question.id}`,
         message: `Must be at least ${validation.minLength} word(s)`,
       });
    -} else if (validation.maxLength && countWords(fieldValue) > validation.maxLength) {
    +} else if (validation.maxLength && wordCount > validation.maxLength) {
       errors.push({
         field_id: `${question.id}`,
         message: `Must be less than ${validation.maxLength} word(s)`,
       });
     }
    Suggestion importance[1-10]: 5

    __

    Why: Precomputing wordCount avoids redundant countWords calls and improves readability with minimal code change.

    Low
    Clean up commented code

    Either re-enable or remove the commented-out pattern validation block to prevent
    drifting requirements or dead code.

    functions/src/controllers/application_controller.ts [518]

    -// if (validation.pattern) {
    +// Remove this commented block if pattern validation is no longer needed,
    +// or uncomment and integrate it back for runtime checks.
    Suggestion importance[1-10]: 3

    __

    Why: Removing or re-enabling the commented pattern validation reduces dead code, but impact is minor cleanup.

    Low

    @hib4 hib4 changed the base branch from main to fix/application June 3, 2025 07:33
    @hib4 hib4 changed the base branch from fix/application to main June 3, 2025 07:34
    @hib4 hib4 merged commit 1959a81 into main Jun 3, 2025
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    3 participants