And they weren't even paid by the line



  • Investigating some security issues in a project, I found myself delving into its role-based permissions system. This is a representative pair of methods, with no redaction (although slight modifications for Community Server). In particular, it is as fully commented as the code I found:

    private bool HasRole(string pageRole)
    {
        string rolepagefound = string.Empty;
        List<string> pageRoleList = null;
        pageRoleList = GetPageRoleList(pageRole);
        if (pageRoleList != null && CurrentUserRoleList != null)
        {
            //All is permission for all user
            string arole = pageRoleList.Find(delegate(string item)
            {
                return item.ToLower() == "all";
            });
            if (!string.IsNullOrEmpty(arole))
            {
                return true;
            }
            for (int i = 0; i < CurrentUserRoleList.Count; i++)
            {
                rolepagefound = pageRoleList.Find(delegate(string item)
                {
                    return item.ToLower() == CurrentUserRoleList[i].NameSpace.ToLower() + ":" + CurrentUserRoleList[i].RoleName.ToLower();
                });
                if (!string.IsNullOrEmpty(rolepagefound))
                {
                    return true;
                }
            }
        }
        return false;
    }
    

    private List<String> GetPageRoleList(string rolelist)
    {
    string[]] separator = { "," };
    string[]] list = rolelist.Split(separator, StringSplitOptions.RemoveEmptyEntries);
    return new List<string>(list);
    }

    So, to start with, HasRole(pageRole) is actually HasAnyRoleFrom(commaSeparatedListOfRoles). Then we have random scopes (what is rolepagefound doing on the first line of that method?); assignments which are immediately overwritten; null checks against values which can't possibly be null; 8 lines involving a delegate and a null check to see whether a string is in a list (and, yes, List<string> does have a Contains(string) method); unnecessary work inside a nested loop. The preceding list is not intended to be exhaustive.



  • Dear God!



  • This ought to get into WTF.. there are so many points I thought why? as I read the code. It smells of inexperienced coder trying to adapt something they downloaded.



  • I bet the original developer got bitten in the ass because of case sensitivity issues, which explains the ToLower...



  • Havent gone through all of the alleged WTFs, but the code in question is doing manipulation of the strings, and not just "Contains". I have not yet spotted an immediate overwrite (I am too lazy to look more once finding a single issue with your list of WTFS)



  • Having a second look at the code, it has two steps:

    1. It looks to see if the passed in list of roles has an "all" role. If it does, return true, else go to step 2
    2. Compares the passed in list to the users role list, and is a single match is found, return true, else return false.

    It's poorly written, but it does make sense. It looks like the built-in .net role authorization logic (comma-delimited list of roles, etc.).



  • @TheCPUWizard said:

    Havent gone through all of the alleged WTFs, but the code in question is doing manipulation of the strings, and not just "Contains".

    Provided that the ToLower() were placed early enough (e.g. by lower-casing the argument before splitting it),

            string arole = pageRoleList.Find(delegate(string item)
            {
                return item.ToLower() == "all";
            });
            if (!string.IsNullOrEmpty(arole))
            {
                return true;
            }

    could be rewritten as

            if (pageRoleList.Contains("all")) return true;

    Similarly the second Find and check.

    @TheCPUWizard said:

    I have not yet spotted an immediate overwrite (I am too lazy to look more once finding a single issue with your list of WTFS)

    You only had to look at the first four lines.

        List<string> pageRoleList = null;
        pageRoleList = GetPageRoleList(pageRole);


  • @pjt33 said:

    List pageRoleList = null;
    pageRoleList = GetPageRoleList(pageRole);
    Agreed, this reeks of junior developer code-monkey.



  • @pjt33 said:

            string arole = pageRoleList.Find(delegate(string item)
            {
                return item.ToLower() == "all";
            });
            if (!string.IsNullOrEmpty(arole))
            {
                return true;
            }
    could be rewritten as
            if (pageRoleList.Contains("all")) return true;

     

    Uhh no. pageRoleList is a List<string> not a string (also String.Contains is case sensitive,) using the origonal call, and assuming C#:

            if (pageRoleList.Find(s => s.ToLower() == "all")) return true; 

    i would probably use a caseinsensiteve compare instead of ToLower():

            if (pageRoleList.Find(s => s.Equals("all", StringComparison.OrdinalIgnoreCase))) return true; 

     



  • @esoterik said:

    Uhh no. pageRoleList is a List<string> not a string (also String.Contains is case sensitive,)

    I can't comment about the rest of what you said (I don't know the .Net libraries so well) but the OP was talking about List<>.Contains(), not String.Contains().



  • @aihtdikh said:

    ... List<>.Contains() ...
    oops, still case sensitive though; if it really has to be case insensitive then you can't use Contains() (without specifying a comparator).

    I realized i should have uesd Count instead of find:

    list.Count(s => s.Equals("all" , ...) > 0

     



  • @C-Octothorpe said:

    @pjt33 said:
    List pageRoleList = null;
    pageRoleList = GetPageRoleList(pageRole);
    Agreed, this reeks of junior developer code-monkey.

    If someone was to declare the list without assigning a value, then in Visual Studio there would be a wiggle under the variable with a warning about using uninitialized variables. Adding = null or = String.Empty like for rolepagefound will make the wiggle go away.

    So here I would blame Visual Studio (and violent video games).



  • @Speakerphone Dude said:

    If someone was to declare the list without assigning a value, then in Visual Studio there would be a wiggle under the variable with a warning about using uninitialized variables. Adding = null or = String.Empty like for rolepagefound will make the wiggle go away.

    So here I would blame Visual Studio (and violent video games).

    Yes, because typing List pageRoleList = GetPageRoleList(pageRole); is against the law!!!



  • @esoterik said:

    I realized i should have uesd Count instead of find:

    list.Count(s => s.Equals("all" , ...) > 0


    No, if you're going to use Linq you should use list.Any(s => ...).

    @Speakerphone Dude said:

    If someone was to declare the list without assigning a value, then in Visual Studio there would be a wiggle under the variable with a warning about using uninitialized variables. Adding = null or = String.Empty like for rolepagefound will make the wiggle go away.


    I don't know which versions of VS you've used, but the ones I've used aren't that stupid.



  • @pjt33 said:

    I don't know which versions of VS you've used, but the ones I've used aren't that stupid.

    Speaky is on track to become the wrongest poster in TDWTF history.



  • @Speakerphone Dude said:

    @C-Octothorpe said:
    @pjt33 said:
    List pageRoleList = null;
    pageRoleList = GetPageRoleList(pageRole);
    Agreed, this reeks of junior developer code-monkey.

    If someone was to declare the list without assigning a value, then in Visual Studio there would be a wiggle under the variable with a warning about using uninitialized variables. Adding = null or = String.Empty like for rolepagefound will make the wiggle go away.

    So here I would blame Visual Studio (and violent video games).

    They added that to your version of the C# compiler for kicks. Most of us have the version where it only warns on variables that are never initialized.



  • @Speakerphone Dude said:

    If someone was to declare the list without assigning a value, then in Visual Studio there would be a wiggle under the variable with a warning about using uninitialized variables. Adding = null or = String.Empty like for rolepagefound will make the wiggle go away.

    So here I would blame Visual Studio (and violent video games).

    So, they installed VS For Retards in your PC? How appropiate...



  • @Speakerphone Dude said:

    @C-Octothorpe said:
    @pjt33 said:
    List pageRoleList = null;
    pageRoleList = GetPageRoleList(pageRole);
    Agreed, this reeks of junior developer code-monkey.

    If someone was to declare the list without assigning a value, then in Visual Studio there would be a wiggle under the variable with a warning about using uninitialized variables. Adding = null or = String.Empty like for rolepagefound will make the wiggle go away.

    But if the coder bulls through and writes the next line (the assignment), then the squiggle goes away.

    I don't think you can blame this on VisualStudio -- probably it's the violent video games. Or the Nanny State.



  • @rstinejr said:

    @Speakerphone Dude said:
    @C-Octothorpe said:
    @pjt33 said:
    List pageRoleList = null;
    pageRoleList = GetPageRoleList(pageRole);
    Agreed, this reeks of junior developer code-monkey.

    If someone was to declare the list without assigning a value, then in Visual Studio there would be a wiggle under the variable with a warning about using uninitialized variables. Adding = null or = String.Empty like for rolepagefound will make the wiggle go away.

    But if the coder bulls through and writes the next line (the assignment), then the squiggle goes away.

    I don't think you can blame this on VisualStudio -- probably it's the violent video games. Or the Nanny State.

    Ok I have an improved theory: what if the guy has been working in VB for years, where (by default) there is an annoying modal popup warning about any problem on the line when you press Enter. This person could have been traumatized and would feel the need to fix immediately any problem reported by Visual Studio, hence fixing the wiggle with a = null. Well that does not explain why he did not do the value assignment immediately; however if that person also has a background in a language where people usually declare variables before doing assignments (such as Cobol), then the mystery is solved.

    So here is the explanation for this part of the code: the programmer is a 50 year old who spent half his career working with Cobol and the other half with VB, and he just switched to C#. Obvious.


Log in to reply