From 84191802dfee05b6e71c8f3679ebc36906c12004 Mon Sep 17 00:00:00 2001
From: Cyril Mougel <cyril.mougel@gmail.com>
Date: Fri, 8 Feb 2013 15:52:56 +0100
Subject: [PATCH] Extract the validation of Username format in own class to
 avoid complexity in user model object

---
 app/controllers/users_controller.rb    |  2 +-
 app/models/user.rb                     | 22 ++--------
 app/models/username_validator.rb       | 60 ++++++++++++++++++++++++++
 spec/models/user_spec.rb               | 15 -------
 spec/models/username_validator_spec.rb | 17 ++++++++
 5 files changed, 81 insertions(+), 35 deletions(-)
 create mode 100644 app/models/username_validator.rb
 create mode 100644 spec/models/username_validator_spec.rb

diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index d9c4bd2c073..3e9518f5bca 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -76,7 +76,7 @@ class UsersController < ApplicationController
   def check_username
     requires_parameter(:username)
 
-    if !User.username_valid?(params[:username])
+    if !UsernameValidator.new(params[:username]).valid_format?
       render json: {errors: [I18n.t("user.username.characters")]}
     elsif !SiteSetting.call_mothership?
       if User.username_available?(params[:username])
diff --git a/app/models/user.rb b/app/models/user.rb
index deb6271ea56..2d75ef8f492 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -374,26 +374,10 @@ class User < ActiveRecord::Base
   end
 
   def username_format_validator
-    unless username
-      return errors.add(:username, I18n.t(:'user.username.blank'))
+    validator = UsernameValidator.new(username)
+    unless validator.valid_format?
+      errors.add(:username, validator.error)
     end
-
-    if username.length < User.username_length.begin
-      return errors.add(:username, I18n.t(:'user.username.short', min: User.username_length.begin))
-    end
-
-    if username.length > User.username_length.end
-      return errors.add(:username, I18n.t(:'user.username.long', max: User.username_length.end))
-    end
-
-    if username =~ /[^A-Za-z0-9_]/
-      return errors.add(:username, I18n.t(:'user.username.characters'))
-    end
-
-    if username[0,1] =~ /[^A-Za-z0-9]/
-      return errors.add(:username, I18n.t(:'user.username.must_begin_with_alphanumeric'))
-    end
-    nil
   end
 
 
diff --git a/app/models/username_validator.rb b/app/models/username_validator.rb
new file mode 100644
index 00000000000..5e3839abeba
--- /dev/null
+++ b/app/models/username_validator.rb
@@ -0,0 +1,60 @@
+class UsernameValidator
+
+  def initialize(username)
+    @username = username
+    @error = []
+  end
+  attr_accessor :error
+  attr_reader :username
+
+  def user
+    @user ||= User.new(user)
+  end
+
+  def valid_format?
+    username_exist?
+    username_length_min?
+    username_length_max?
+    username_char_valid?
+    username_first_char_valid?
+    error.blank?
+  end
+
+  private
+
+  def username_exist?
+    return unless error.empty?
+    unless username
+      self.error = I18n.t(:'user.username.blank')
+    end
+  end
+
+  def username_length_min?
+    return unless error.empty?
+    if username.length < User.username_length.begin
+      self.error = I18n.t(:'user.username.short', min: User.username_length.begin)
+    end
+  end
+
+  def username_length_max?
+    return unless error.empty?
+    if username.length > User.username_length.end
+      self.error = I18n.t(:'user.username.long', max: User.username_length.end)
+    end
+  end
+
+  def username_char_valid?
+    return unless error.empty?
+    if username =~ /[^A-Za-z0-9_]/
+      self.error = I18n.t(:'user.username.characters')
+    end
+  end
+
+  def username_first_char_valid?
+    return unless error.empty?
+    if username[0,1] =~ /[^A-Za-z0-9]/
+      self.error = I18n.t(:'user.username.must_begin_with_alphanumeric')
+    end
+  end
+
+end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index d0df536f2a6..9f04c9e0217 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -193,7 +193,6 @@ describe User do
     end
   end
 
-
   describe 'new' do
 
     subject { Fabricate.build(:user) }
@@ -394,20 +393,6 @@ describe User do
     end
   end
 
-  context '.username_valid?' do
-    it 'returns true when username is both valid and available' do
-      User.username_valid?('Available').should be_true
-    end
-
-    it 'returns true when the username is valid but not available' do
-      User.username_valid?(Fabricate(:user).username).should be_true
-    end
-
-    it 'returns false when the username is not valid' do
-      User.username_valid?('not valid.name').should be_false
-    end
-  end
-
   describe '.suggest_username' do
     it 'corrects weird characters' do
       User.suggest_username("Darth%^Vadar").should == "Darth_Vadar"
diff --git a/spec/models/username_validator_spec.rb b/spec/models/username_validator_spec.rb
new file mode 100644
index 00000000000..d03de632648
--- /dev/null
+++ b/spec/models/username_validator_spec.rb
@@ -0,0 +1,17 @@
+require 'spec_helper'
+
+describe UsernameValidator do
+  context "#valid_format?" do
+    it 'returns true when username is both valid and available' do
+      expect(UsernameValidator.new('Available').valid_format?).to eq true
+    end
+
+    it 'returns true when the username is valid but not available' do
+      expect(UsernameValidator.new(Fabricate(:user).username).valid_format?).to eq true
+    end
+
+    it 'returns false when the username is not valid' do
+      expect(UsernameValidator.new('not valid.name').valid_format?).to eq false
+    end
+  end
+end