patterncsharpMinor
Dynamic Language Interpreter in C#
Viewed 0 times
interpreterlanguagedynamic
Problem
This code interprets an abstract syntax tree generated by a recursive descent parser for a dynamic scripting language called Hassium. The syntax of this language looks like this
Anyways, people on an IRC network I am a member of have been complaining with this specific class. Obliviously, this a very big file so I wouldn't ask you all to review every line. It works, but what stylistic things should be refactored; such as readability, design, coding style, ect? Also does this reek of anti-patterns or bad code smell?
// Credit to contributer Zdimension, who has done countless amounts of work on this project
```
using System;
using System.Collections;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Net;
using System.Net.Sockets;
using System.Reflection;
using System.Text;
using System.Threading.Tasks;
using Hassium.Functions;
using Hassium.HassiumObjects;
using Hassium.HassiumObjects.Collections;
using Hassium.HassiumObjects.Conversion;
using Hassium.HassiumObjects.Debug;
using Hassium.HassiumObjects.Drawing;
using Hassium.HassiumObjects.IO;
using Hassium.HassiumObjects.Math;
using Hassium.HassiumObjects.Networking;
using Hassium.HassiumObjects.Networking.HTTP;
using Hassium.HassiumObjects.Text;
using Hassium.HassiumObjects.Types;
using Hassium.Lexer;
using Hassium.Parser;
using Hassium.Parser.Ast;
using Hassium.Semantics;
namespace Hassium.Interpreter
{
public delegate void ExitEventHandler(int code);
///
/// Interpreter.
///
public class Interpreter : IVisitor
{
public Stack CallStack = new Stack();
public Dictionary Globals = new Dictionary();
public AstNode Code { get; set; }
public SymbolTable Sy
# Recursion functions
func fibonacci(n) {
if (n == 0) {
return 0;
} else if (n == 1) {
return 1;
}
return fibonacci(n - 1) + fibonacci(n - 2);
}
func main () {
for (i = 0; i < 33; i++) {
println(fibonacci(i));
}
}Anyways, people on an IRC network I am a member of have been complaining with this specific class. Obliviously, this a very big file so I wouldn't ask you all to review every line. It works, but what stylistic things should be refactored; such as readability, design, coding style, ect? Also does this reek of anti-patterns or bad code smell?
// Credit to contributer Zdimension, who has done countless amounts of work on this project
```
using System;
using System.Collections;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Net;
using System.Net.Sockets;
using System.Reflection;
using System.Text;
using System.Threading.Tasks;
using Hassium.Functions;
using Hassium.HassiumObjects;
using Hassium.HassiumObjects.Collections;
using Hassium.HassiumObjects.Conversion;
using Hassium.HassiumObjects.Debug;
using Hassium.HassiumObjects.Drawing;
using Hassium.HassiumObjects.IO;
using Hassium.HassiumObjects.Math;
using Hassium.HassiumObjects.Networking;
using Hassium.HassiumObjects.Networking.HTTP;
using Hassium.HassiumObjects.Text;
using Hassium.HassiumObjects.Types;
using Hassium.Lexer;
using Hassium.Parser;
using Hassium.Parser.Ast;
using Hassium.Semantics;
namespace Hassium.Interpreter
{
public delegate void ExitEventHandler(int code);
///
/// Interpreter.
///
public class Interpreter : IVisitor
{
public Stack CallStack = new Stack();
public Dictionary Globals = new Dictionary();
public AstNode Code { get; set; }
public SymbolTable Sy
Solution
Well, it's a large chunk of code so here just a few things which came to mind when skimming over it:
-
You don't have any unit tests (at least none that I could find on github). You should start writing them. I suspect it will be a PITA to do so which is good since it will highlight refactoring targets plus it will provide regression tests for when you break things while refactoring. Yes it means you'll have to invest time in it without making actual progress on features but if you'd like the project to grow and more people contributing to it you can easily put yourself into a world of pain if you don't.
-
Check all your
-
There is no documentation on the public interface of the class so you are pretty much resigned to read the code if you want to use it.
-
Related to this: The public interface of this class is very broad and it's absolutely not obvious how it is meant to be used. I assume methods are meant to be called in a certain order and in certain states or otherwise things will go wrong. Also having a public dictionary called
-
I tend to start refactoring these kind of things in baby steps. A good candidate seems to the be
makes things very hard to read and is scattered around in various places. Create a
-
I'm not 100% sure what exactly the purpose of the
-
Also reading a lot of code like
-
You don't have any unit tests (at least none that I could find on github). You should start writing them. I suspect it will be a PITA to do so which is good since it will highlight refactoring targets plus it will provide regression tests for when you break things while refactoring. Yes it means you'll have to invest time in it without making actual progress on features but if you'd like the project to grow and more people contributing to it you can easily put yourself into a world of pain if you don't.
-
Check all your
public/private modifiers and make sure that only things which need to be public actually are. In large blobs like these I often find public methods or properties which aren't used anywhere outside of the class (or not even there). Anything public is a lot harder to change since it will potentially affect a lot of calling code. Things which are private can be changed around at will (as long as you don't break anything, see unit testing above).-
There is no documentation on the public interface of the class so you are pretty much resigned to read the code if you want to use it.
-
Related to this: The public interface of this class is very broad and it's absolutely not obvious how it is meant to be used. I assume methods are meant to be called in a certain order and in certain states or otherwise things will go wrong. Also having a public dictionary called
Constants which is apparently everything but constant seems like an oxymoron to me.-
I tend to start refactoring these kind of things in baby steps. A good candidate seems to the be
CallStack. Code like this:if (CallStack.Count > 0 &&
(CallStack.Peek().Scope.Symbols.Contains(name) || CallStack.Any(x => x.Locals.ContainsKey(name))))
CallStack.First(x => x.Locals.ContainsKey(name) || x.Scope.Symbols.Contains(name))
.Locals.Remove(name);makes things very hard to read and is scattered around in various places. Create a
Callstack class, define the interface to it in terms of how the Interpreter should interact with it on the most basic level. This should clean up some code.-
I'm not 100% sure what exactly the purpose of the
Accept methods is and how they are related to the interpret methods and how it is all related to Execute. So at the first glance it seems your Interpreter has at least two responsibilities: Turn the AST nodes into Hassium nodes and execute the Hassium nodes. This is at least one responsibility too much and turning the AST nodes into Hassium nodes should probably be extracted.-
Also reading a lot of code like
if (evaluated is HassiumArray || evaluated is HassiumString) seems like a step backwards in the age of OOP.Code Snippets
if (CallStack.Count > 0 &&
(CallStack.Peek().Scope.Symbols.Contains(name) || CallStack.Any(x => x.Locals.ContainsKey(name))))
CallStack.First(x => x.Locals.ContainsKey(name) || x.Scope.Symbols.Contains(name))
.Locals.Remove(name);Context
StackExchange Code Review Q#105127, answer score: 7
Revisions (0)
No revisions yet.