8
\$\begingroup\$

Two days ago I started reading about SignalR and today I started writing a simple application. I have written the code below so far, but I was wondering if you have some tips to improve my code.

Program

  • Desktop client will automatically create a random group (random) and displays the random generated code to the user.
  • Mobile client has the possibility to connect with the desktop client (both clients in one group) by entering the random generated code.
  • Maximum users per group: 2 (1; Desktop, 2; Mobile).
  • All sessions need to be registrered in my database.
  • The session has to be removed from the database when closing it.

    public class ExpertHub : Hub
    {
        // OnDisconnected
        public override Task OnDisconnected(bool stopCalled)
        {
            if(stopCalled)
            {
                removeSession(Context.ConnectionId);
            }
            else
            {
                // This server hasn't heard from the client in the last ~35 seconds.
            }
            return base.OnDisconnected(stopCalled);
        }
    
        // Send Message To Specified GroupName
        public void Send(string message, string groupName)
        {
            Clients.OthersInGroup(groupName).addMessage(Context.ConnectionId + ": " + message);
        }
    
        // Join Group At Desktop
        public async Task JoinDesktop(string groupName)
        {
            if(CountGroup(groupName) < 1)
            {
                // Write new session to database.
                newSession(groupName);
                // Add user to group
                await Groups.Add(Context.ConnectionId, groupName);
            }
            else
                Clients.Caller.addMessage("Something went wrong. Please refresh your browser");
        }
    
        // Join Group At Mobile
        public async Task JoinMobile(string groupName)
        {
            // Check if group already exists
            if (CheckGroup(groupName))
            {
                if (CountGroup(groupName) < 2)
                {
                    // Write new session to database.
                    newSession(groupName);
                    // Add user to group and send message to all users in group.
                    await Groups.Add(Context.ConnectionId, groupName);
                    Clients.Group(groupName).addMessage("Connected");
                }
                else
                {
                    Clients.Caller.addMessage("Connection already established.");
                }
            }
            else
                Clients.Caller.AddMessage("Wrong Code");
        }
    
        // Check If Group Already Exists
        public bool CheckGroup(string groupName)
        {
            using (var db = new ConnectionContext())
            {
                var group = db.Connections.Where(a => a.RoomName == groupName).FirstOrDefault();
    
                if (group == null)
                    return false;
                else
                    return true;
            }
        }
    
        // Count Occurences of GroupName
        public int CountGroup(string groupName)
        {
            using (var db = new ConnectionContext())
            {
                int group = db.Connections.Where(a => a.RoomName ==     groupName).Count();
                return group;
            }
        }
    
        // Write A New Session To The Database
        public void newSession(string groupName)
        {
            using (var db = new ConnectionContext())
            {
                var session = new Connection()
                {
                    Connected = true,
                    ConnectionID = Context.ConnectionId,
                    RoomName = groupName
                };
    
                db.Connections.Add(session);
                db.SaveChanges();
            }
        }
    
        // Remove A Existing Session From The Database
        public void removeSession(string Id)
        {
            using (var db = new ConnectionContext())
            {
                var sessionToRemove = db.Connections.Where(a =>     a.ConnectionID == Id).FirstOrDefault();
                if (sessionToRemove != null)
                {
                    db.Connections.Remove(sessionToRemove);
                    db.SaveChanges();
                }
            }
        }
    
        // Generate Random String
        public string GenerateRandom()
        {
            return Randomizer.RandomString();
        }
    }
    

Mobile client

    <script type="text/javascript">
        $(function () {
            // Start connection hub 
            var chat = $.connection.expertHub;

            // Declare a function on the chat hub so the server can invoke it
            chat.client.AddMessage = function (message) {
                $('#discussion').append('<li>' + message + '</li>');
            };

            // Enter group by group ID
            $.connection.hub.start(function () {
                $('#displaygroup').val(prompt('Enter group ID:', ''));
                chat.server.joinMobile($('#displaygroup').val());
            });

        });
    </script>

Desktop client

<script type="text/javascript">
    $(function () {
        // Start connection hub
        var chat = $.connection.expertHub;

        $.connection.hub.start(function () {
            chat.server.generateRandom().done(function (result) {
                // Render code display
                for (var x = 0; x < result.length; x++)
                {
                    $('#code').append('<span>' + result[x] + '</span>')
                }
                // Define group as a global variable
                group = result;
                // Start session and join group
                chat.server.joinDesktop(group);
            });
        });

        // Declare a function on the chat hub so the server can invoke it
        chat.client.AddMessage = function (message) {
            $('#discussion').append('<li>' + message + '</li>');
        };

        // Start the connection
        $.connection.hub.start().done(function () {
            $("#broadcast").click(function () {
                // Call the chat method on the server
                chat.server.send($('#msg').val(), group);
                // Clear text box and reset focus for next comment.
                $('#msg').val('').focus();
            });
        });
    });

</script>

The program itself is working, but I want to improve the code. I am just a beginner, so I don't know where to start with improving it. I prefer improvements for the C# (ASP.NET) code. Anybody?

\$\endgroup\$
2
\$\begingroup\$

In :

if(stopCalled)
{
    removeSession(Context.ConnectionId);
}
else
{
    // This server hasn't heard from the client in the last ~35 seconds.
}

Why do you keep the else block if there's nothing in it?

You should always use brackets when there's a bug possibility. In such a case :

else
    Clients.Caller.addMessage("Something went wrong. Please refresh your browser");

should use brackets, just to make sure you don't introduce some nasty bugs.

And hey, you could reverse your condition to remove some nesting :

if(CountGroup(groupName) < 1)
{
    // Write new session to database.
    newSession(groupName);
    // Add user to group
    await Groups.Add(Context.ConnectionId, groupName);
}
else
    Clients.Caller.addMessage("Something went wrong. Please refresh your browser");

could be :

if(CountGroup(groupName) > 1)
{
    Clients.Caller.addMessage("Something went wrong. Please refresh your browser");
    return;
}

// Write new session to database.
newSession(groupName);
// Add user to group
await Groups.Add(Context.ConnectionId, groupName);

This method name isn't very explicit :

 // Check if group already exists
if (CheckGroup(groupName))

Read your comment, read your method name. Does it mean the same thing? No it doesn't. In that case, CheckGroup should be GrouExists or something like that. Check is a very vague term.

CountGroup(groupName) < 2 should be GetGroupCount or something like that. Because right now your method doesn't explain enough.

That :

if (group == null)
    return false;
else
    return true;

Could be changed to return group != null;

You have some methods that don't respect naming like newSession(groupName);. It should be PascalCased, so NewSession(groupName);

Finally, your header comments should follow conventions.

// Send Message To Specified GroupName
public void Send(string message, string groupName)

You should use the triple dash style comments, which will be used by the IDE when you type your code. But overall your comments need to be more useful. For example, it's useless to state what your code does. It should state why you're doing it. If you feel like your code doesn't need to be explained, don't put any comments.

\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you for submitting some improvement suggestions. I will mark your answer as accepted. More tips are always welcome. \$\endgroup\$ – Lycaon Dec 11 '15 at 9:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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