0
\$\begingroup\$

This logic checks accessGroup and if accessGroup is equal to "Admin" then it only checks if result.Admin or baccess is true but if accessGroup is anthing else it will need to check two other objects result.Admin == true || result.PowerUser. Is there any other way to do this if condition?

if (accessGroup == "Admin")
{
    if (baccess == true || result.Admin == true)
    {
        var FileInfo = GetFile(fileManagerGuidId);
        if (FileInfo != null)
        {
            FileManagerLog _filemanagerLog = new FileManagerLog();
            _filemanagerLog.CustomerId =Request.Cookies["customerid"] != null ? Convert.ToInt32(Request.Cookies["customerid"].Value) : 0;
            _filemanagerLog.FileManagerGuid = new Guid(fileManagerGuidId);
            SaveFileManagerLog(_filemanagerLog);
            byte[] fileBytes = FileInfo.FileData;
            return File(fileBytes, System.Net.Mime.MediaTypeNames.Application.Octet, FileInfo.FileName);
        }
        else
        {
            return null;
        }
    }
}
else
{
    if (baccess == true || result.Admin == true || result.PowerUser)
    {
        var FileInfo = GetFile(fileManagerGuidId);
        if (FileInfo != null)
        {
            FileManagerLog _filemanagerLog = new FileManagerLog();
            _filemanagerLog.CustomerId =Request.Cookies["customerid"] != null ? Convert.ToInt32(Request.Cookies["customerid"].Value) : 0;
            _filemanagerLog.FileManagerGuid = new Guid(fileManagerGuidId);
            SaveFileManagerLog(_filemanagerLog);
            byte[] fileBytes = FileInfo.FileData;
            return File(fileBytes, System.Net.Mime.MediaTypeNames.Application.Octet, FileInfo.FileName);
        }
        else
        {
            return null;
        }
    }
}
\$\endgroup\$
7
  • \$\begingroup\$ if (baccess || (accessGroup == "Admin" ? result.Admin : (result.Admin || result.PowerUser))) \$\endgroup\$
    – Dmitry
    Sep 20, 2021 at 13:23
  • 1
    \$\begingroup\$ Is this your real code? If yes, which I can't believe, both branches will execute the same code hence you could just skip the if..else. \$\endgroup\$
    – Heslacher
    Sep 20, 2021 at 13:53
  • 2
    \$\begingroup\$ The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?. \$\endgroup\$
    – BCdotWEB
    Sep 20, 2021 at 14:12
  • 1
    \$\begingroup\$ The issue with indentation is significantly compounding the readability and subsequently a reader's understanding of the intended logic. This is a good case example of why deeply nested if structures are bad for readability. \$\endgroup\$
    – Flater
    Sep 20, 2021 at 14:15
  • 1
    \$\begingroup\$ on a side note Request.Cookies["customerid"] != null ? Convert.ToInt32(Request.Cookies["customerid"].Value) : 0; can be much simplified to Convert.ToInt32(Request.Cookies["customerid"]?.Value ?? 0); \$\endgroup\$
    – MikeT
    Oct 15, 2021 at 9:22

2 Answers 2

4
\$\begingroup\$

here is a clearer view of the current code :

if (accessGroup == "Admin")
{
    if (baccess == true || result.Admin == true)
    {
        var FileInfo = GetFile(fileManagerGuidId);

        if (FileInfo != null)
        {
            // return File(...);
        }
        else
        {
            return null;
        }
    }
}
else
{
    if (baccess == true || result.Admin == true || result.PowerUser)
    {
        var FileInfo = GetFile(fileManagerGuidId);
        
        if (FileInfo != null)
        {
            // return File(...);
        }
        else
        {
            return null;
        }
    }
}

Can't you see both return the same file ? If this works very well with current project, then you can simplify it to :

if (accessGroup == "Admin" || baccess == true || result.Admin == true || result.PowerUser)
{
    var FileInfo = GetFile(fileManagerGuidId);

    if (FileInfo != null)
    {
      // return File(...);
    }
    
}
else
{
    return null;    
}

if your actual file log is different than the code, and it has different logic based on the conditions above, then you can translate it to this :

if(baccess == false && result.Admin == false)
{
    return null;
}

var FileInfo = GetFile(fileManagerGuidId);


if(FileInfo == null)
{
   return null;
}

if(accessGroup == "Admin")  
{
    // Admin file logic 
   // return File(...);     
}

if(result.PowerUser)
{
     // PowerUser file logic 
    // return File(...);
}
\$\endgroup\$
1
  • \$\begingroup\$ i made the same mistake when i first read his code accessGroup != "Admin" And's with powerUsers not Or's \$\endgroup\$
    – MikeT
    Oct 15, 2021 at 9:16
1
\$\begingroup\$

this looks like it will have the same effect

if (baccess  || result.Admin || (accessGroup != "Admin" && Result.PowerUser))

though i suspect what you mean to do is AND (&&)

if (baccess  &&  (result.Admin || (accessGroup != "Admin" && result.PowerUser))

ie if they don't have access or the right access level don't run the code

\$\endgroup\$

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.