DZone
Thanks for visiting DZone today,
Edit Profile
  • Manage Email Subscriptions
  • How to Post to DZone
  • Article Submission Guidelines
Sign Out View Profile
  • Post an Article
  • Manage My Drafts
Over 2 million developers have joined DZone.
Log In / Join
Please enter at least three characters to search
Refcards Trend Reports
Events Video Library
Refcards
Trend Reports

Events

View Events Video Library

Zones

Culture and Methodologies Agile Career Development Methodologies Team Management
Data Engineering AI/ML Big Data Data Databases IoT
Software Design and Architecture Cloud Architecture Containers Integration Microservices Performance Security
Coding Frameworks Java JavaScript Languages Tools
Testing, Deployment, and Maintenance Deployment DevOps and CI/CD Maintenance Monitoring and Observability Testing, Tools, and Frameworks
Culture and Methodologies
Agile Career Development Methodologies Team Management
Data Engineering
AI/ML Big Data Data Databases IoT
Software Design and Architecture
Cloud Architecture Containers Integration Microservices Performance Security
Coding
Frameworks Java JavaScript Languages Tools
Testing, Deployment, and Maintenance
Deployment DevOps and CI/CD Maintenance Monitoring and Observability Testing, Tools, and Frameworks

Last call! Secure your stack and shape the future! Help dev teams across the globe navigate their software supply chain security challenges.

Modernize your data layer. Learn how to design cloud-native database architectures to meet the evolving demands of AI and GenAI workloads.

Releasing software shouldn't be stressful or risky. Learn how to leverage progressive delivery techniques to ensure safer deployments.

Avoid machine learning mistakes and boost model performance! Discover key ML patterns, anti-patterns, data strategies, and more.

Trending

  • Unlocking AI Coding Assistants Part 3: Generating Diagrams, Open API Specs, And Test Data
  • Article Moderation: Your Questions, Answered
  • Building Resilient Networks: Limiting the Risk and Scope of Cyber Attacks
  • Beyond ChatGPT, AI Reasoning 2.0: Engineering AI Models With Human-Like Reasoning

A Code Review of C# Code

Sometimes, it helps to see how others conduct their code reviews — even if only to glean some new insight into a language.

By 
Michał Franc user avatar
Michał Franc
·
Apr. 20, 17 · Opinion
Likes (9)
Comment
Save
Tweet
Share
13.0K Views

Join the DZone community and get the full member experience.

Join For Free

I did a brief code review for a friend (note: this is in C# code).

        public IEnumerable < GitHubUser > FavoritesList() {
         CookieHelper cookieHelper = new CookieHelper(this.HttpContext);
         HttpCookie httpCookie = cookieHelper.SetAndGetHttpCookies();
         MyCookie myCookie = new MyCookie() {
          ID = Convert.ToInt32(httpCookie["Id"])
         };
         List < GitUser > favoritesList = new List < GitUser > ();

         using(var db = new GitHubContext()) {
          var results = (from ch in db.CookiesHistory where ch.UserId == myCookie.ID select new {
           GitUserId = ch.GitUserId
          });

          foreach(var result in results) {
           var user = (from u in db.GitUsers where u.Id == result.GitUserId select new {
            u
           }).First();

           favoritesList.Add(user.u);
          }
         }

         return favoritesList;
        }

Look like trivial code. Where to start? Context! Context is king. It will be obvious. But first, you need to ask questions:

  • What are you trying to achieve here?
  • What are the requirements?

This is the best way to start every code-review process. A person asking for a code review already knows the answers to those questions. It would be a waste to try to discover this by yourself. This function returns a list of GitHub favorited users. On GitHub, there is an ability to follow users.

Initial observations:

  • I see Git, but there is no GitHub API client call.
  • LINQ indicates that we are doing some queries.
  • Context keyword is entity framework (ORM to access data).
  • HttpContext and Cookies are executed in ASP.NET scope.
  • Function without params returning a list of favorites.

Questions:

  • Where is the call to the GitHub API? There is a snapshot of GitHub data stored in an external data store.

Naming Convention

The ideal code is the one that is easy to grasp. I do like the KISS principle here; it's a pragmatic approach. A simple but elegant code triumphs everything. I want to read my code like a book with a narrative. There is a certain mechanism that we can use to make the code "read" better. Starting with the function name.

public IEnumerable<GitHubUser> FavoritesList()

This is not enough. It doesn’t tell me anything.

public IEnumerable<GitHubUser> FavouriteGitHubUsers()

Better...but GitHub doesn’t have a concept of favorites. There is a concept of followed users.

public IEnumerable<GitHubUser> FollowedGitHubUsers()

It is important to name functions correctly and take into account business language. DDD experts might call it ubiquitous language. The word "favorite" will confuse GitHub domain experts familiar with different terms. This confusion means lost time and, in the end, lost money. Don’t just create new names and when you need to "invent" a new word or convention. Discuss it. Ask domain experts and teammates. Maybe there is already a name for that functionality or piece of code. Just like we strive to create simple, short code, we should also try to keep our domain vocabulary clean, simple, and ubiquitous.

Parameterless Functions

For me, function without parameters is an antipattern. Yes, there are certain scenarios in which it might be useful, but those are very rare and mostly related to already-broken design. If the function doesn’t have any params, then what does it do? Where does it take the input and state? It could a global one — but that is an antipattern, too. Functions need input and output and should be pure. A pure function is a nice concept; I learned while exploring functional programming.

A pure function is a function in which the return value is only determined by its input values without observable side effects. This is how functions in math work. Math.cos(x) will, for the same value of x, always return the same result.

The current function is not pure due to global state in the form of cookies and a reliance on HTTPContext. My friend tried to hide the logic to access them in CookieHelper, but that is not enough. The problem is still there. In Asp.NET, use HTTPContext outside the function. It shouldn’t leak to the inside. Current code can’t work in different context. If we execute it in the process without global HTTPContext it will break.

There is another anti-pattern here, new keyword. Try to inject as many things as possible. This advice can be used incorrectly but dependency injection is your friend don’t ignore it.

A closer look at the code shows that CookieHelper gives access to myCookie.ID. We can remove all this logic from this function by introducing a function param ID.

public IEnumerable<GitHubUser> FollowedGitHubUsers(int cookieId)

We don’t care where the cookieId value is coming from. This gives more options as we can get the cookieId from other sources, not only cookies.

Code After Changes

        public IEnumerable < GitHubUser > FollowedGitHubUsers(int userId) {
         List < GitHubUser > favoritesList = new List < GitHubUser > ();

         using(var db = new GitHubContext()) {
          var results = (from ch in db.CookiesHistory where ch.UserId == userId select new {
           GitUserId = ch.GitUserId
          });


          foreach(var result in results) {
           var user = (from u in db.GitUsers where u.Id == result.GitUserId select new {
            u
           }).First();

           favoritesList.Add(user.u);
          }
         }

         return favoritesList;
        }

We gained some clarity and I no longer have to worry about cookies.

Divide the Code to Separate Functions

There are two queries to DB, both using the same Context. I want to have code that looks like this:

        public IEnumerable < GitHubUser > FollowedGitHubUsers(int userId) {
         using(var db = new GitHubContext()) {
          var followedUserIds = this.GiveMeFollowedUsersIdFor(db, userId);
          return this.FindUsers(db, followedUsersIds);
         }
        }

Isn’t that lovely and simple? All of this, thanks to the extraction of functions. It is now super easy to read. On this level, I don’t care how I am getting users. All I need to see is the orchestration of some functions and input. If I need more details I can jump into this function.

        private IEnumerable<GitHubUser>FindUsers(GitHubContext db, IEnumerable<int> userIds)
        {
                List<GitHubUser> favoritesList = new List<GitHubUser>();

                foreach (var result in results)
                {
                    var user = (from u in db.GitUsers
                                where u.Id == result.GitUserId
                                select new { u }).First();

                    favoritesList.Add(user.u);
                }
            }

            return favoritesList;
        }

Jumping into this function also gives me a simpler picture. There is a clear input and there is a clear output. I don’t have to worry about anything else but query here. Simple, small functions give more clarity and help the brain to digest information. With smaller functions, we have to worry about the smaller number of lines of code. With clear input and output, we can establish scope and context. There is no cookie helper here. There is no need to bother where do we get the userId from. We hide all the complexity and can concentrate on the important stuff. There is a lot less noise in functions like this.

That’s it. If you, dear reader, have some interesting code, I would love to review it.

Published at DZone with permission of Michał Franc, DZone MVB. See the original article here.

Opinions expressed by DZone contributors are their own.

Partner Resources

×

Comments
Oops! Something Went Wrong

The likes didn't load as expected. Please refresh the page and try again.

ABOUT US

  • About DZone
  • Support and feedback
  • Community research
  • Sitemap

ADVERTISE

  • Advertise with DZone

CONTRIBUTE ON DZONE

  • Article Submission Guidelines
  • Become a Contributor
  • Core Program
  • Visit the Writers' Zone

LEGAL

  • Terms of Service
  • Privacy Policy

CONTACT US

  • 3343 Perimeter Hill Drive
  • Suite 100
  • Nashville, TN 37211
  • support@dzone.com

Let's be friends:

Likes
There are no likes...yet! 👀
Be the first to like this post!
It looks like you're not logged in.
Sign in to see who liked this post!