\documentclass{scrartcl}

\author{Jamie Owen}
\date{\today}
\title{Practical 3 Feedback}
\subtitle{Comments and Future Tips From the Marker}

\usepackage{amsmath}
\usepackage{listings}


\begin{document}
\lstset{language = R}
\maketitle
	\section{General Comments}
		\begin{itemize}
			\item A number of students used the functions which had been declared as disallowed in the assignment, eg. sum(x), mean(x), \ldots
			\item One thing people lost marks for was not naming their file correctly, please read instructions carefully.
			\item Overall students made a good effort to format and indent code properly, thank you, this makes my life a lot easier when reading them.
			\item If you declare variables and functions in a script it is generally a good idea to name them sensibly. This also makes reading, debugging, and error finding far easier as you can from the name tell what a fucntion does rather than always having to check what function a() does etc.
			
			\item A function can only access variables it can see. If you have something like 
			\begin{lstlisting}
				f = function(x){
					sum(y) ...
				}
			\end{lstlisting}
			where y is not defined anywhere then you will have errors and the function will not be callable. See the note on global variables below alongside this.
		\end{itemize}
		
	\section{Tips for Making Code Better}
		\begin{itemize}
			\item Declaring a function within a function can be inefficient it you want to use that function more than once. For example doing something like
			\begin{lstlisting}
				MySD = function(x){
					MyMean = function(x){
					
					}
				}
			\end{lstlisting}
			you are actually redefining the MyMean() function here every time you call MySD when it does the same thing with each call.
			\item Calling the same function on the same argument repeatedly is inefficient and bad practice. E.g
			\begin{lstlisting}
				for(i in 1:n){
					(x[i] - MyMean(x))
				}
			\end{lstlisting}
			You are causing the computation in MyMean() to be done n times, it makes much more sense to store the outcome of MyMean() once and use that as it gives the same answer in each of those n calls in the loop.
			\item I saw a few instances of the following:
			\begin{lstlisting}
				x[which(x>0)]
			\end{lstlisting}
			Whilst this is doing what you are intending i.e returning the elements in x according to the vector of indices returned by the which function, it is also possible to subset x via a boolean condition. I.e x[ x $>$ 0] will return the same vector and is much more efficient.
			\item When using an if() test that only has two possible outcomes it does not make sense to have more than one if statement.
			\begin{lstlisting}
				if(x>0){
					\ldots
				} else if(x <= 0){
					\ldots
				}
			\end{lstlisting}
			By necessity and definition if the test is negative in the first if() statement then the condition inside the sceond has to be true and so this extra check is needless and hence inefficient.
			\item Multiple definitions of a function that does the same thing. Doing something akin to the following is not sensible:
			\begin{lstlisting}
				Xmean = function(x){
					return(sum(x)/length(x))
				}
				Ymean = funciton(y){
					return(sum(y)/length(y))
				}
			\end{lstlisting}
			Here the two functions perform the exact same calculation i.e Xmean(x) = Ymean(x) for a given vector x. The point of the x variable within the function header is not to say that it has to receive a vector which has been called x. It says that the function will take a vector which from then on will be reffered to as x. You could, without any issues, do:
			\begin{lstlisting}
				mean = function(x){
					return(sum(x)/length(x))
				}
				y = 1:5
				mean(y)
			\end{lstlisting}
			as within your function it is reffering to your vector y as x and performs operations on this x which you passed in, could be called absolutely anything you like.
			\item You should always try to avoid global variables where possible.
			\begin{lstlisting}
				a = 0
				f = function(){
					return(a+1)
				}
				g = function(){
					return(a+2)
				}
			\end{lstlisting}
			This is not good practice, while you may get away with this in some cases, if one was to accidentally set the value of a to be something different later on in the code then these two functions would take in that value of a rather than the intended 0. Should always define variables that you want to use locally within the function
			\begin{lstlisting}
				f = function(){
					a = 0
					return(a + 1)
				}
				g = function(){
					a = 0
					\ldots
				}
			\end{lstlisting}
			
			\item If you want to declare a default value in a function you can do thsi by including it in the function header
			\begin{lstlisting}
				f = function(x, y = 10){
				
				}
				#rather than
				f = function(x,y){
					if(missing(y)){
						y = 10
					}
					
				}
			\end{lstlisting}
		\end{itemize}
\end{document}