2
\$\begingroup\$

So I was recently asked to complete the following challenge for an interview:

Programming Problem Provide a production ready solution that accepts a new user name, validates a single string input as the password, and stores it locally. The solution should be written in C#/Xamarin with native UI (Android or iOS, your choice, not Xamarin Forms) UI requirements:

  1. The UI should have at least a page that is a list of users and their information.
  2. The UI should have the ability to add a new user, which brings up a new screen, and then appends that user to the bottom of the list view.
  3. Alert the user if their passwords are invalid either on submission or real time.

The following are the string validations:

  1. String must consist of a mixture of letters and numerical digits only, with at least one of each.
  2. String must be between 5 and 12 characters in length.
  3. String must not contain any sequence of characters immediately followed by the same sequence.

My solution is here: https://github.com/rjowell/TestApp

I was recently notified that I did not pass. What could I have done differently, or is the interviewer being too nit-picky?

Here's a sample of my code:

using Foundation;
using Newtonsoft.Json;
using System;
using System.CodeDom.Compiler;
using System.Collections.Generic;
using System.IO;
using UIKit;

namespace CharterTestApp
{
public class User
{
    public string name { get; set; }
    public string location { get; set; }
    public string username { get; set; }
    public string password { get; set; }

    public User(string nameInput, string loc, string user, string pass)
    {
        name = nameInput;
        location = loc;
        username = user;
        password = pass;
    }
}

public partial class LoginController : UIViewController
{
    

    
    public LoginController(IntPtr handle) : base(handle)
    {
    }

    public override bool ShouldPerformSegue(string segueIdentifier, NSObject sender)
    {
        return ProcLogin(usernameInput.Text, passwordInput.Text);
    }
   
    public override void ViewDidLoad()
    {
        base.ViewDidLoad();
        
    }

    public override void DidReceiveMemoryWarning()
    {
        base.DidReceiveMemoryWarning();
        // Release any cached data, images, etc that aren't in use.
    }

    bool ProcLogin(string username, string password)
    {
        string errorText = "";
        

        if(username == "")
        {
            errorText += "Username is blank\n";
            if(password == "")
            {
                errorText += "Password is blank\n";
            }
            ErrorLabel.Text = errorText;
            ErrorLabel.Hidden = false;
            return false;
        }
        else
        {
            foreach(User item in Application.userList)
            {
                if(usernameInput.Text == item.username)
                {
                    if(password == "")
                    {
                        errorText += "Password is blank\n";
                        ErrorLabel.Text = errorText;
                        return false;
                    }
                    else if(passwordInput.Text != item.password)
                    {
                        
                        errorText += "Password Incorrect\n";
                        ErrorLabel.Text = errorText;
                        return false;

                    }
                    else
                    {
                        Application.currentName = item.name;
                        Application.currentUsername = item.username;
                        return true;
                    }
                    
                }
            }
            errorText += "Username doesn't exist\n";
            ErrorLabel.Text = errorText;
            return false;
        }

        }

    
}
}

CreateNewUser.cs

using Foundation;
using Newtonsoft.Json;
using System;
using System.IO;
using System.Text.RegularExpressions;
using UIKit;

namespace CharterTestApp
{
public partial class CreateUserController : UIViewController
{
    public CreateUserController (IntPtr handle) : base (handle)
    {
    }



    partial void ProcessUserInput(UIButton sender)
    {
        
        string errorDisplays = "";

        if (NameEntry.Text == "")
        {
            errorDisplays += "Name is blank\n";
        }
        else
        {
            foreach (User user in Application.userList)
            {
                if (user.name == NameEntry.Text)
                {
                    errorDisplays += "User already exists with that name\n";
                }
            }
        }
        if(LocationEntry.Text == "")
        {
            errorDisplays += "Location is blank\n";
        }
        if(UsernameEntry.Text == "")
        {
            errorDisplays += "Username is blank\n";
        }
        else
        {
            foreach (User user in Application.userList)
            {
                if (user.username == UsernameEntry.Text)
                {
                    errorDisplays += "User already exists with that name\n";
                }
            }
        }
        if(PasswordEntry.Text == "")
        {
            errorDisplays += "Password is blank\n";
        }
        else
        {
            if (Regex.IsMatch(PasswordEntry.Text,@"(..+)\1") || PasswordEntry.Text.Length < 5 || PasswordEntry.Text.Length > 12 || !Regex.IsMatch(PasswordEntry.Text, @"^[a-zA-Z0-9]+$", RegexOptions.IgnoreCase))
            {
                errorDisplays += "Password is not valid\n";
            }
   
        }


      
        if (errorDisplays != "")
        {
            
            ErrorLabel.Hidden = false;
            ErrorLabel.Text = errorDisplays;
           
        }
        else
        {
            Application.userList.Add(new User(NameEntry.Text,LocationEntry.Text,UsernameEntry.Text,PasswordEntry.Text));
            File.WriteAllText("LoginData.json", "[");
            foreach(User currentUser in Application.userList)
            {
                Console.WriteLine(JsonConvert.SerializeObject(currentUser));
                File.WriteAllText("LoginData.json", JsonConvert.SerializeObject(currentUser));
            }
            File.WriteAllText("LoginData.json", "]");
            Console.WriteLine(File.ReadAllText("LoginData.json"));
            UserSuccessWindow.Hidden = false;
        }

        
    }

    
}


}
\$\endgroup\$
6
  • 3
    \$\begingroup\$ Post all of your code here or specific parts for feedback, questions based on external sources get closed \$\endgroup\$
    – Coupcoup
    Commented Sep 22, 2020 at 17:33
  • 1
    \$\begingroup\$ So the project is a complete Xamarin.iOS app, so I'm hesitant to post ALL the code here. The app consists of a login screen, a TableView listing users, and a Create New Users page. Is there something specific I should post? \$\endgroup\$
    – Russ J
    Commented Sep 22, 2020 at 18:24
  • \$\begingroup\$ Does the code work? Did you test it? Are there any particular concerns you have about it if you can't show us everything? Please see our FAQ on asking questions. \$\endgroup\$
    – Mast
    Commented Sep 22, 2020 at 18:25
  • 1
    \$\begingroup\$ Pull out some core parts of the system like validation and examples of different coding techniques like UI. You probably won't get feedback on how it all works together but might get some responses on structure/coding practices that are more obviously not up to snuff \$\endgroup\$
    – Coupcoup
    Commented Sep 22, 2020 at 18:38
  • 1
    \$\begingroup\$ Not covered elsewhere, but ErrorLabel.Text is not set when there are no errors. So if the user corrects an error the error then the message remains. Sometimes a password can be blank. Why not allow this case? Also, use a dictionary for name and passwords. \$\endgroup\$
    – CWallach
    Commented Sep 22, 2020 at 22:30

2 Answers 2

5
\$\begingroup\$

I'm assuming that the code works and fulfills all the usability requirements that you got. Also assuming that this was a home assignment with "unlimited" time where you could code at your own pace and no one was hanging over your shoulder.

To summarize, I think there are a bunch of "red flags" showing poor habits and/or poor awareness of basic code standards, which doesn't mean the program won't run, but it means if I hired you I would need to spend more time bringing you up to speed and good habits. And it is a clear downside if there are 5 other candidates, all else equal, who don't make these mistakes.

Depending on the position you apply for, I suppose this could be a dealbreaker, but then of course I can't read the minds of your interviewer. If you got no feedback, who knows why they failed you?

Formatting

They probably don't fail you for formatting, but it still stands out to me as hard to read, non-standard, and sloppy, especially since it can be fixed automatically and for free.

Running a small piece of your code through an online formatter returns this instead, with consistent and readable indentation and brackets. Still a few extra lines though. Why are there so many empty lines all over the code?

https://codebeautify.org/csharpviewer


foreach(User item in Application.userList) {
  if (usernameInput.Text == item.username) {
    if (password == "") {
      errorText += "Password is blank\n";
      ErrorLabel.Text = errorText;
      return false;
    }
    else if (passwordInput.Text != item.password) {

      errorText += "Password Incorrect\n";
      ErrorLabel.Text = errorText;
      return false;

    }
    else {
      Application.currentName = item.name;
      Application.currentUsername = item.username;
      return true;
    }

  }
}

Error handling

I also find it weird to report multiple errors at the same time like you do here. I'm not a UI/UX expert, but I would check and tell the user about one error, and once they fix that, check for the next error.

There is probably some "standard way/best practice" of doing this and I could be wrong.


if(username == "")
        {
            errorText += "Username is blank\n";
            if(password == "")
            {
                errorText += "Password is blank\n";
            }
            ErrorLabel.Text = errorText;
            ErrorLabel.Hidden = false;
            return false;
        }

Don't repeat yourself


errorText += "Password Incorrect\n";
ErrorLabel.Text = errorText;
return false;

These lines are repeated multiple times within the same piece of code. There must be a better way of doing it, probably refactoring to some shared "error-report" function and early return that allows you to have these lines only once in your code and to restructure the validation/username parts.

Don't repeat yourself 2

The code below also looks like an invitation to refactor. Could you define a SortedDictionary<Element, String> or other useful structure, and iterate through your elements text, to validate and report the errors, rather than copy the same things over and over?

if (NameEntry.Text == "")
        {
            errorDisplays += "Name is blank\n";
        }
        else
        {
            foreach (User user in Application.userList)
            {
                if (user.name == NameEntry.Text)
                {
                    errorDisplays += "User already exists with that name\n";
                }
            }
        }
        if(LocationEntry.Text == "")
        {
            errorDisplays += "Location is blank\n";
        }
        if(UsernameEntry.Text == "")
        {
            errorDisplays += "Username is blank\n";
        }

Data structure

Storing the users in a list seems wrong when you need to do a lookup on their name. This is a clear case for a Dictionary and/or Set of some kind. Putting myself in the interviewer's seat: since you're not using a dictionary, it makes me suspect that you may not know about dictionaries, and if that's true, we have a problem.

foreach (User user in Application.userList)
            {
                if (user.username == UsernameEntry.Text)
                {
                    errorDisplays += "User already exists with that name\n";
                }
\$\endgroup\$
4
  • \$\begingroup\$ Thanks for your response. So getting back to my original question, do you think those mistakes are enough to disqualify for a Mid Dev role? What about a Jr role? \$\endgroup\$
    – Russ J
    Commented Sep 22, 2020 at 22:13
  • 3
    \$\begingroup\$ I don't think any of these in itself would disqualify the applicant, at least not for a Jr role. But most importantly from my point of view, if several candidates applied to 1 position and all else was equal, then this would matter. In the opposite situation, if the company needs to fill the position and there was just one applicant who got this far in the process, I would think that we can fix these things and I wouldn't be much worried. \$\endgroup\$
    – user985366
    Commented Sep 22, 2020 at 22:21
  • \$\begingroup\$ FWIW, in your “Error handling” section, I understand what you were going for, but you’re only checking for missing password if the username was missing. You are reproducing the problem in the OP’s code snippet. You want to check these two fields independently. \$\endgroup\$
    – Rob
    Commented Dec 29, 2020 at 17:02
  • \$\begingroup\$ @Rob I just copied the code to point out which piece of code I was talking about. I didn't change it or attempt to write a fix. \$\endgroup\$
    – user985366
    Commented Dec 31, 2020 at 20:21
1
\$\begingroup\$

There's a lot going on here. Let me just focus on the string validations, that being said, let me mention that it's not good that you've coupled all this logic tightly to the actual representation on the UI, this makes testing difficult and means you have all this logic to retrieve the values of fields interspersed with the logic handling them.

        if (Regex.IsMatch(PasswordEntry.Text,@"(..+)\1") || PasswordEntry.Text.Length < 5 || PasswordEntry.Text.Length > 12 || !Regex.IsMatch(PasswordEntry.Text, @"^[a-zA-Z0-9]+$", RegexOptions.IgnoreCase))

I guess this is suppose to encode the three validations.

Well, first thoughts, this is a very long line, the way you've structured makes it pretty apparent which parts correspond to which rules (but in reverse of the order they gave which is slightly confusing and with what I guess to be the most computationally expensive test first, but I have no evidence of that so ignore that comment), but nonetheless it's still too confusing by my reckoning.

Furthermore, where you've placed it means it's hard to test.

Ideally I would propose making each rule its own function with a good name

Then a function like ValidatePassword(string s) which returns either success, or something to identify which rule failed.

ValidatePassword should be fairly trivial, so you can be confident it works as desired provided the other functions work.

The actual implementations of the functions, well I think what you've done is pretty OK.

(..+)\1 Doesn't match abc2deff, which seems in error to me as 'f' is a sequence of characters. But maybe I've misinterpreted here. In any case I like it because (with the caveat of maybe removing the first .) it is clearly right.

Regex.IsMatch(PasswordEntry.Text, @"^[a-zA-Z0-9]+$", RegexOptions.IgnoreCase) I think you've missed the logic that you were supposed to have at least one digit and one letter.

\$\endgroup\$
4
  • \$\begingroup\$ So same question to you as the previous responder, would you consider these things disqualification for a Mid or Jr. role? \$\endgroup\$
    – Russ J
    Commented Sep 22, 2020 at 22:40
  • 2
    \$\begingroup\$ It's hard to say I'm not really in the world of hiring. For me your code reads like someone who has some understanding of programming and of c#, unfortunately the exercise asked doesn't really let me see your problem solving skills, in terms of the way you structure your code, it really does need some improvement it's not maintable. But from a junior who has maybe never had a professional job programming before it seems like something you might expect. So, not an instant disqualification, but if there's better people there's better people. Most important is that you take the lessons to heart. \$\endgroup\$ Commented Sep 22, 2020 at 22:47
  • \$\begingroup\$ "For me your code reads like someone who has some understanding of programming and of c#...But from a junior who has maybe never had a professional job programming before it seems like something you might expect." Yup, I check both of these categories. Self-taught but no pro experience yet. The ironic thing is most of the critiques on here are things I feel can only be learned by getting a coding job somewhere. \$\endgroup\$
    – Russ J
    Commented Sep 22, 2020 at 23:56
  • 2
    \$\begingroup\$ @RussJ I agree with the above comment. Also, I think you can learn a lot by doing what you're doing now. Do interviews, get feedback from them and write code and post it here. Also, while you can learn a lot on the job, there are also many jobs where you won't learn good habits, structure and style, because the companies don't enforce it, don't have a good code review process, don't have senior developers to help you learn, or they don't know better. So if you have a choice of jobs, it can be a good thing to make sure you have senior colleagues and good processes to help you learn and improve. \$\endgroup\$
    – user985366
    Commented Sep 23, 2020 at 0:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.