Python stack and queue classes, implemented using lists I have made a stack and queue implementation with OOP in Python (sorry for variable names) . It is made from scratch. I just watched a video how these two things work. I am still trying to improve in OOP so I would be thankful for every advice. Two things which bother me the most are using que and sta in the beginning and whether it is okay to just use already implemented functions in Python such as .pop() .

QUEUE

que = []

class Queue:
    def __init__(self,queue):
        self.queue = queue

    def push(self,item):
        print ("PUSH",item)

        if len(self.queue)!=0:
            self.queue.append(0)
            for i in range(len(self.queue)-1,0,-1): ## replacing elements
                self.queue[i] = self.queue[i-1]
            self.queue[0] = item

        else:
            self.queue.append(item)

    def pop(self):
        print ("POP",self.queue[-1])

        if len(self.queue)!=0:
            self.queue.pop()
        else:
            raise ValueError('Cant pop from empty queue.')

    def __str__(self):
        return (str(self.queue))

## creating QUEUE
que = Queue(que)
que.push(5)
print (que)
que.push(6)
print (que)
que.push(7)
print (que)
que.pop()
print (que)

STACK

class Stack:
    def __init__(self,stack):
        self.stack = stack

    def push(self,item):
        print ("PUSH",item)

        if len(self.stack)!=0:
            self.stack.append(0)
            for i in range(len(self.stack)-1,0,-1):
                self.stack[i] = self.stack[i-1]
            self.stack[0] = item

        else:
            self.stack.append(item)

    def pop(self):
        print ("POP",self.stack[0])

        if len(self.stack)!=0:
            self.stack.pop(0)
        else:
            raise ValueError('Cant pop from empty stack.')

    def __str__(self):
        return (str(self.stack))
## creating STACK
stac = Stack(sta)
stac.push(5)
print(stac)
stac.push(7)
print(stac)
stac.push(2)
print(stac)
stac.pop()
print(stac)

There are a few things to mention.

At first, it might be a good idea to initialize both Queue and Stack with empty lists by default:

def __init__(self, queue=None):
    self.queue = queue or []    # if queue is given, it overrides the default

Next, it doesn't make sense to write something like if len(self.queue)!=0 since it is equivalent to if self.queue (empty list is False when it's casted to bool). You can go even further and introduce the property called size using the built-in decorator:

@property
def size(self):
    return len(self.queue)

Now it can be accessed by self.size without a function call.


Given some integers, return the k most frequent elements Given a non-empty array of integers, return the k most frequent elements.

For example, given [1,1,1,2,2,3] and k = 2, return [1,2].

Following is my submission to this problem on Leetcode and it is accepted. Can you please provide feedback on any issue that you see in this code? I am looking for improvements in the quality of the code from both software engineering perspective as well as from algorithmic complexity or space and run-time performance perspective.

#include <algorithm>
#include <map>
#include <vector>

using namespace std;

class Node
{
public:

    int value{};
    unsigned frequency{};

    Node()
    {

    }

    Node (int value, unsigned frequency)
    {
        this->value = value;
        this->frequency = frequency;
    }

    bool operator< (const Node& other)
    {
        return this->frequency < other.frequency;
    }
};

class Solution {
public:

    vector<int> topKFrequent(vector<int>& nums, int k)
    {
        map<int, unsigned> frequency;

        for (auto n : nums )
        {
            if ( frequency.find(n) != frequency.end())
                frequency[n]++;
            else
                frequency[n] = 0;
        }

        vector<Node> heap;

        for ( auto f : frequency )
            heap.push_back(Node(f.first, f.second));

        make_heap(heap.begin(), heap.end());

        vector<int> result;

        unsigned i = 0;
        while ( i < k )
        {
            result.push_back(heap.front().value);
            pop_heap(heap.begin(), heap.end()-i);
            i++;
        }

        return result;
    }
};

Just the general C++ coding style treatment.

Do not use using namespace std;. It can introduce subtle bugs, makes the code harder to read and writing out the std:: prefix generally is not going to be to time intensive.
Keep your spacing consistent. Compare the following two lines

bool operator< (const Node& other)
vector<int> topKFrequent(vector<int>& nums, int k)

Why is one method name followed by a space while the other is not? Consistency is important! If you are not sure which spacing variant to choose, you should read a C++ style guide. The same applies to control structures, such as

for (auto n : nums )
if ( frequency.find(n) != frequency.end())
for ( auto f : frequency )

Use a member initializer list in your constructor. Although it does not matter in a lot of cases, it is a more concise way of expressing member initialization and leaves the constructor's body open for more complex setup tasks. The Node constructor would then look something like this:

Node(int value, unsigned frequency) : value(value), frequency(frequency) { }

Also, since you default constructor does not actually do anything, you should remove it and the initializers for value and frequency and add default values to the constructor parameters instead, so that Node's constructor becomes something like

Node(int value = 0, unsigned frequency = 0) : value(value), frequency(frequency) { }

Keep in mind, however, that this constructor allows implicit conversions from integer types, which you typically want to avoid. This is easily prevented by making the constructor explicit.
Most people prefer to not write this-> explicitly when not needed.
const-correctness is important. You should take the parameter nums in topKFrequent by const-reference since you do not actually modify it.
Always use curly brackets with if,else,for,while etc. This prevents a lot of errors from statements not being included in conditionally dependent blocks when they should be. In fact, this kind of error is so common that gcc even introduced a diagnostic for misleading indentation in version 6.


Writing an excel doc with a subset of my data in R. Currently the code is excessively long I'm fairly new to R. I have used the 'r2excel' package to create an excel workbook from a subset of my R data. This should have 3 sheets for the three different subsets. What I have written works fine, but its a lot of code and is very clunky, just re written three times.
If there is a better way of framing my question/best practice tips, I'd love to learn.

# Create an Excel workbook.
filename <- "validitycheck.xlsx" #The name of the file that will be saved
wb <- createWorkbook(type="xlsx") #Creating an excel workbook

# Create a sheet in that workbook to contain the data table
sheet1 <- createSheet(wb, sheetName = "prime1")
sheet2 <- createSheet(wb, sheetName = "prime2")
sheet3 <- createSheet(wb, sheetName = "prime3")

# Add header for sheet 1
xlsx.addHeader(wb, sheet1, value="Prime 1",level=1,
               color="black", underline=1)
xlsx.addLineBreak(sheet1, 1)

# Add header for sheet 2
xlsx.addHeader(wb, sheet2, value="Prime 2",level=1,
               color="black", underline=1)
xlsx.addLineBreak(sheet2, 1)

# Add header for sheet 3
xlsx.addHeader(wb, sheet3, value="Prime 3",level=1,
               color="black", underline=1)
xlsx.addLineBreak(sheet3, 1)

# Add a paragraph : Author
author=paste("Author : Gabriella. \n",
             "20% randomly chosen validity check.",
             "\n Source: filename", sep="")

#Add author for sheet 1
xlsx.addParagraph(wb, sheet1,value=author, isItalic=TRUE, colSpan=5,
                  rowSpan=4, fontColor="darkgray", fontSize=14)

#Add author para for sheet 2
xlsx.addParagraph(wb, sheet2,value=author, isItalic=TRUE, colSpan=5,
                  rowSpan=4, fontColor="darkgray", fontSize=14)

#Add author para for sheet 3
xlsx.addParagraph(wb, sheet3,value=author, isItalic=TRUE, colSpan=5,
                  rowSpan=4, fontColor="darkgray", fontSize=14)

xlsx.addLineBreak(sheet1, 3)
xlsx.addLineBreak(sheet2, 3)
xlsx.addLineBreak(sheet3, 3)

# Add table : add a data frame for sheet 1
xlsx.addHeader(wb, sheet1, value="Validty for prime 1")
xlsx.addLineBreak(sheet1, 1)
xlsx.addTable(wb, sheet1, sample_prime1,
               fontColor="darkblue", fontSize=14,
              rowFill=c("white", "lightblue"),
              startCol=2,
              row.names=FALSE,
              col.names = TRUE)

# Add table : add a data frame for sheet 2
xlsx.addHeader(wb, sheet2, value="Validty for prime 2")
xlsx.addLineBreak(sheet2, 1)
xlsx.addTable(wb, sheet2, sample_prime2,
               fontColor="darkblue", fontSize=14,
              rowFill=c("white", "lightblue"),
              startCol=2,
              row.names=FALSE,
              col.names = TRUE)

# Add table : add a data frame for sheet 3
xlsx.addHeader(wb, sheet3, value="Validty for prime 3")
xlsx.addLineBreak(sheet3, 1)
xlsx.addTable(wb, sheet3, sample_prime3,
               fontColor="darkblue", fontSize=14,
              rowFill=c("white", "lightblue"),
              startCol=2,
              row.names=FALSE,
              col.names = TRUE)

# save the workbook to an Excel file
saveWorkbook(wb, filename, password = "secret")
xlsx.openFile(filename)# View the file
```

@Duck from Stack overflow provided the answer. Pasting it here. Its is essentially writing a function:
library(r2excel)
#Data
sample_prime1 <- 1:4
sample_prime2 <- 10:14
sample_prime3 <- 22:26
#Store in a list
List <- list(sample_prime1,sample_prime2,sample_prime3)
#Function
myfun <- function(wb,name,df) {
  # Create object
  sheet <- createSheet(wb, sheetName = name)
  # Add headers
  xlsx.addHeader(wb, sheet, value=paste0('Validty for prime ',name))
  xlsx.addLineBreak(sheet, 1)
  xlsx.addTable(wb, sheet, data.frame(df),
                fontColor="darkblue", fontSize=14,
                rowFill=c("white", "lightblue"),
                startCol=2,
                row.names=FALSE,
                col.names = FALSE)
}
#Create workbook
filename <- "validitycheck.xlsx"
wb <- createWorkbook(type="xlsx")
#Loop
#Vector for names
vnames <- paste0('prime',1:length(List))
#Code
for(i in 1:length(List))
{
  myfun(wb,vnames[i],List[i])
}


Find the perfect numbers within the range of 1 and a chosen number My program is complete and working, but I would like second opinions on it. The program prompts the user for a positive value that will serve as the maximum for the range that will be checked for perfect numbers. Although this is one of those do-what-you-want-with-it problems, I have to keep the two prototypes. Are there any extra adjustments I should make in the program or is it already efficient the way it is?

#include "stdafx.h"
#include <iostream>
using namespace std;

/* Function prototypes */
bool isPerfect(int n);
int sumOfProperDivisors(int n);

/* Main program*/
int main()
{
    // Declare variable(s)
    int bound;  // Stores upper limit of number range

    // Prompt user to enter a positive integer
    cout << "Enter upper bound: ";
    cin >> bound;

    // Display the information
    cout << "\n\nThe perfect numbers between 1 and " << bound << " are: " << endl;

    for (int i = 1; i <= bound; i++)
    {
        if (isPerfect(i))
        {
            cout << i << endl;
        }
    }

    system("PAUSE");

    return 0;
}

/*
* Function: isPerfect
* -------------------
* Returns the sum of the number(s) within the testing range
*/
bool isPerfect(int n)
{
    int sum = 0;

    for (int i = 1; i < n; i++)
    {
        if (n % i == 0)
        {
            sum += i;
        }
    }

    return sum == n;
}

/*
* Function: int sumOfProperDivisors
* ---------------------------------
* Returns the sum of a number's divisors
*/
int sumOfProperDivisors(int n)
{
    int sum = 1;

    for (int i = 2; i <= n/2; i++)
    {
        if ((n % i) == 0)
        {
            sum += i;

            if (i * i == n)
            {
                sum -= i;
            }
        }
    }

    return sum;
}

MS Specific

MS specific don't need it here

#include "stdafx.h"

Namespace std

Never do this.

using namespace std;

I know every book you read does this. This is because they are paying for the ink. Its actually very bad style and will cause a huge amount of problems for any program longer than ten lines or for code that lives longer than a year. So it is bad habit you should get out of the habit of doing.

The reason they picked std rather than standard so it is easy to type and use as a prefix. Its not that much harder to type than the normal object or type.

std::cout << "Testing\n"; // Not that hard see.

For details about the problem see: Why is “using namespace std;” considered bad practice?

Comments

Don't add us-less comments.

/* Main program*/
int main()
{
    // Declare variable(s)

Comments can do more harm than good when used incorrectly. This is because over time comments fall out of line with the code unless very explicitly maintained. Because the compiler does not check the comments for accuracy this happens a lot.

Thus a maintainer who sees a comment that does not agree with the code then has to try and work out which is wrong. The code or the comment and then has to fix it.

You should restrict your comments to "WHY" comments. Why is the code working like this. "WHY" we need to do something. The code itself should explain "HOW" (as long as you use good variable and function names).

See: What are examples of comments that tell you why instead of how or what?

USless comment: (I can see what it does by reading the code)

    // Display the information
    cout << "\n\nThe perfect numbers between 1 and " << bound << " are: " << endl;

Don't like this comment.

/*
* Function: isPerfect
* -------------------
* Returns the sum of the number(s) within the testing range
*/

In this case the comment says one thing. The name of the function says something else. It does not return the sum it returns bool. Because the function is named  perfectly you don't really need to say anything (the function name is enough).

bool isPerfect(int n)

Don't particularly like the comment.

/*
* Function: int sumOfProperDivisors
* ---------------------------------
* Returns the sum of a number's divisors
*/

It does not tell me anything that the name of the function already tells me.

Validate User Input

You don't validate user input.

    cin >> bound;

Input from many sources can be trusted. Input from a human can not. Validate their input and anticipate that they will either be stupid or malicious in trying to crash your program.

std::endl and flushing

Prefer to use "\n" instead of std::endl.

     cout << "\n\nThe perfect numbers between 1 and " << bound << " are: " << endl;

The reason is that std::endl just puts "\n" then flushes the stream. Stream flushing can make the code very inefficient if done incorrectly. Also the streams flush themselves when they need to without your help.

see: C++: “std::endl” vs “\n”

Prefer Prefix increment

Prefer prefix increment ++i over suffix increment i++.

    for (int i = 1; i <= bound; i++)

Though in this case it does not explicitly matter. If sombody came along and changed the type of you loop (to say BigInt a type that supports 128 bit integers) then it may make a difference. Also you want them to be able to change the type without having to change any code and still get the best performance.

The reason that i++ can be less efficient for types is because of the default way of implementing the operator.

see: Questions about operator overloading

The default method means that the suffix version creates an extra copy of the object that is not required by the prefix version.

System Pause Bad

Don't do this.

    system("PAUSE");

Use a standard happy way of apuing the the code.

cin.ignore(255, '\n'); // ingore input to the end of line (as you did not do that in the previous read).
std::cout << "Hit Enter to continue\n";
cin.get();             // Wait for user input to be flushed.

Return in Main

The function main() is special. If you don't specify a return value the compiler will generate one for you return 0. It is the only function this will happen in.

As a result we use having no return as an indicator to the maintainer that there is no way for this application to fail. If you have an explicit return 0; at the end then I am looking through your code to find a place where you return an error code.

    return 0;


Optimizing lighting algorithm for a voxel engine I am creating a Minecraft-like terrain engine in XNA to learn HLSL and more XNA. The problem is my lighting algorithm. Whenever a block which emits light is placed, it takes about half a second to calculate it. I have implemented the lighting in a recursive way.

public void DoLight(int x, int y, int z, float light)
{
    Vector3i xDecreasing = new Vector3i(x - 1, y, z);
    Vector3i xIncreasing = new Vector3i(x + 1, y, z);
    Vector3i yDecreasing = new Vector3i(x, y - 1, z);
    Vector3i yIncreasing = new Vector3i(x, y + 1, z);
    Vector3i zDecreasing = new Vector3i(x, y, z - 1);
    Vector3i zIncreasing = new Vector3i(x, y, z + 1);

    if (light > 0)
    {
        light--;

        world.SetLight(x, y, z, (int)light);
        Blocks.Add(new Vector3(x, y, z));

        if (world.GetLight(yDecreasing.X, yDecreasing.Y, yDecreasing.Z) < light &&
            !world.GetBlock(yDecreasing.X, yDecreasing.Y, yDecreasing.Z).IsSolid &&
            world.InBounds(yDecreasing.X, yDecreasing.Y, yDecreasing.Z))
            DoLight(x, y - 1, z, light);
        if (world.GetLight(yIncreasing.X, yIncreasing.Y, yIncreasing.Z) < light &&
            !world.GetBlock(yIncreasing.X, yIncreasing.Y, yIncreasing.Z).IsSolid &&
            world.InBounds(yIncreasing.X, yIncreasing.Y, yIncreasing.Z))
            DoLight(x, y + 1, z, light);
        if (world.GetLight(xDecreasing.X, xDecreasing.Y, xDecreasing.Z) < light &&
            !world.GetBlock(xDecreasing.X, xDecreasing.Y, xDecreasing.Z).IsSolid &&
            world.InBounds(xDecreasing.X, xDecreasing.Y, xDecreasing.Z))
            DoLight(x - 1, y, z, light);
        if (world.GetLight(xIncreasing.X, xIncreasing.Y, xIncreasing.Z) < light &&
            !world.GetBlock(xIncreasing.X, xIncreasing.Y, xIncreasing.Z).IsSolid &&
            world.InBounds(xIncreasing.X, xIncreasing.Y, xIncreasing.Z))
            DoLight(x + 1, y, z, light);
        if (world.GetLight(zDecreasing.X, zDecreasing.Y, zDecreasing.Z) < light &&
            !world.GetBlock(zDecreasing.X, zDecreasing.Y, zDecreasing.Z).IsSolid &&
            world.InBounds(yDecreasing.X, yDecreasing.Y, zDecreasing.Z))
            DoLight(x, y, z - 1, light);
        if (world.GetLight(zIncreasing.X, zIncreasing.Y, zIncreasing.Z) < light &&
            !world.GetBlock(zIncreasing.X, zIncreasing.Y, zIncreasing.Z).IsSolid &&
            world.InBounds(zIncreasing.X, zIncreasing.Y, zIncreasing.Z))
            DoLight(x, y, z + 1, light);
    }
}

Does anyone see what is costing me so much performance? And if so, could you please suggest any way to fix it?

Especially when I try to update only a single light every frame, I get a depressingly low FPS of 4 (maybe the recursion is the problem).  Does anyone know if a iterative would increase performance ?  If so, how would I implement it?

Building upon Vadym's answer - the vector instantiation could probably be eliminated entirely.  It appears they are only created to provide nice groupings of x,y,z coordinates, as they are only ever referenced by pulling the x,y,z values back out.

If performance is really an issue, I would suggest keeping individual values instead for xincreasing, xdecreasing, etc. and using those.  It saves on the object creation and the property getter calls. (edit: and, to me at least, it is a little easier to read)

public void DoLight(int x, int y, int z, float light)
{
    int xDecreasing = x - 1;
    int xIncreasing = x + 1;
    int yDecreasing = y - 1;
    int yIncreasing = y + 1;
    int zDecreasing = z - 1;
    int zIncreasing = z + 1;

    if (light > 0)
    {
        light--;

        world.SetLight(x, y, z, (int)light);
        Blocks.Add(new Vector3(x, y, z));

        if (world.GetLight(x, yDecreasing, z) < light &&
            !world.GetBlock(x, yDecreasing, z).IsSolid &&
            world.InBounds(x, yDecreasing, z))
            DoLight(x, yDecreasing, z, light);
        if (world.GetLight(x, yIncreasing, z) < light &&
            !world.GetBlock(x, yIncreasing, z).IsSolid &&
            world.InBounds(x, yIncreasing, z))
            DoLight(x, yIncreasing, z, light);
        if (world.GetLight(xDecreasing, y, z) < light &&
            !world.GetBlock(xDecreasing, y, z).IsSolid &&
            world.InBounds(xDecreasing, y, z))
            DoLight(xDecreasing, y, z, light);
        if (world.GetLight(xIncreasing, y, z) < light &&
            !world.GetBlock(xIncreasing, y, z).IsSolid &&
            world.InBounds(xIncreasing, y, z))
            DoLight(xIncreasing, y, z, light);
        if (world.GetLight(x, y, zDecreasing) < light &&
            !world.GetBlock(x, y, zDecreasing).IsSolid &&
            world.InBounds(x, y, zDecreasing))
            DoLight(x, y, zDecreasing, light);
        if (world.GetLight(x, y, zIncreasing) < light &&
            !world.GetBlock(x, y, zIncreasing).IsSolid &&
            world.InBounds(x, y, zIncreasing))
            DoLight(x, y, zIncreasing, light);
    }
}


Calculate regression coefficients for specific conditions in a data frame I have a data set where leaf mass (MASS) was measured in 30 different tanks (TANK) on 3 dates (DATE). The tanks were also assigned to 5 different treatments.

I want to calculate the regression slope of the mass loss for each tank so I wrote the following function in R

 k.tank <- function() {
   k <- numeric(0)
   tank <- numeric(0)
   treat <- character(0)

   for(i in TANK)
     k <- c(coef(summary(lm(log(MASS)[TANK == i] ~ DATE[TANK == i])))[2, 1], k)

   for(i in TANK)
     tank <- c(i, tank)

   for(i in TANK)
     treat <- c(TREATMENT, treat)

   k.list <- data.frame(tank, treat, k)

   return(k.list)
}

This function is currently working (but see below) but I feel like I made some non-optimal choices.  Please provide feedback on how to improve this function.

NOTE: The code is working but it does return the list in reverse order 30 - 1 and it changes the treatment labels from letters ("A", "B", "C", etc...) to numbers ("1", "2", "3", etc...). Any thoughts on these idiosyncrasies would also be appreciated.

A bunch of things to say.

You're looping 3 times on the same vector "TANK", once would have be enough.
You're growing vectors in each loop
One loop just recreate the same vector reversed (more on this later)
One loop repeat a vector as much as there's entries in TANK (which is  not what you're after I'm pretty sure).
You're creating a local data.frame just to return it.

For your questions:

  it does return the list in reverse order 30 - 1

You're making the vector by prepending the value before the actual state:

for(i in TANK)
     tank <- c(i, tank)

Using tank <- c(tank, i) will give you the normal order.

  it changes the treatment labels from letters ("A", "B", "C", etc...)
  to numbers ("1", "2", "3", etc...).

I highly suspect TREATMENT to be of class factor, concatenation in the vector drop the factor and keep only the indices values.

DISCLAIMER I made the assumption that your variables are:

All of length 90
Matching by the TANK name position

You code can be optimized to:

k.tank <- function() {
  # allocate the vector once before the loop
  k <- numeric(30)
  treat <- character(30)
  tanks=unique(TANK)

  for(i in seq_along(tanks)) { # loop on indice instead of value
    # Assign the result in the proper place directly
    treat[i] <- TREATMENT[TANK == i][1]
    k[i] <- coef(summary(lm(log(MASS)[TANK == tanks[i]] ~ DATE[TANK == tanks[i]])))[2, 1]
  }

  # create the data.frame, as last statement this will be the function return value
  data.frame(tanks, treat, k)
}

With sample values:

set.seed(42)
MASS=runif(90)*100
TANK=rep(1:30,3)
TREATMENT=rep(rep(LETTERS[1:5],6),3)
DATE=sort(rep(seq.Date(Sys.Date(),by=1,length.out=3), 30))

This gives:

   tanks treat           k
1      1     A -0.15155006
2      2     B  0.02382969
3      3     C  0.48811951
4      4     D -0.19125411
5      5     E  0.14033970
6      6     A -0.50391863
7      7     B -0.49942663
8      8     C  0.90820123
9      9     D  0.02682661
10    10     E -0.53769180
11    11     A -1.18268285
12    12     B -0.81647939
13    13     C -0.73156739
14    14     D  0.31479427
15    15     E -0.42545699
16    16     A -0.13376959
17    17     B -2.41040604
18    18     C  0.58095049
19    19     D  0.03985375
20    20     E -2.93855112
21    21     A -0.22053714
22    22     B  0.06480414
23    23     C -0.50659181
24    24     D -0.19135960
25    25     E  1.12094187
26    26     A  0.04589634
27    27     B -0.25630776
28    28     C -1.15457853
29    29     D -0.82633221
30    30     E -0.50380311

Now you read until there, and still assuming your variables are of same length (but if not I can't really get your code) you may try doing something more direct:

# Create a data.frame with your observations
df <- data.frame(TANK,MASS,TREATMENT,DATE, stringsAsFactors=FALSE)
# Create a function for ease of use, taking a data.frame as input
tank.coef <- function(int.df) {
  coef(summary(lm(log(int.df$MASS) ~ int.df$DATE)))[2,1]
}

# run the above function on global data.frame, subsetting by TANK value
k <- by(df,df$TANK,tank.coef)

This will return a by object (coef by tank):

> head(k)
df$TANK
          1           2           3           4           5           6
-0.15155006  0.02382969  0.48811951 -0.19125411  0.14033970 -0.50391863

You can get rid of the name with as.vector and recreate a data.frame with:

> rdf <- data.frame(tank = df$TANK[df$TANK==names(k)], treat = df$TREATMENT[df$TANK==names(k)],k = as.vector(k))
> head(rdf)
  tank treat           k
1    1     A -0.15155006
2    2     B  0.02382969
3    3     C  0.48811951
4    4     D -0.19125411
5    5     E  0.14033970
6    6     A -0.50391863


Iterating through a dictionary in C# I am trying to get my dictionary to iterate seven times after the dateTime is found. I want to iterate through the next week of objects in my dictionary pagesInRange. I figured out how to do it, but I am sure this is not good code. I am wondering if someone could point me in the direction of what I can do to improve this code.

I could not figure out how to iterate through the range I want, so instead I iterate through the entire dictionary and have a bunch of it statements to find the values I am looking for. Here is my code. I know I can use a switch statement to improve the code, but I wondering if there is a way given the first value to iterate over the next seven values.

Console.WriteLine("Please enter a date in the following formate MM/DD/YYYY");
string date = Console.ReadLine();
DateTime dt = Convert.ToDateTime(date);
DateTime end1 = dt.AddDays(1);
DateTime end2 = dt.AddDays(2);
DateTime end3 = dt.AddDays(3);
DateTime end4 = dt.AddDays(4);
DateTime end5 = dt.AddDays(5);
DateTime end6 = dt.AddDays(6);

int i = 0;
foreach (var pair in pagesInRange)
{
    if (pair.Key.ToString("d")==date)
    {
        Console.WriteLine(pair.Key.ToString("d") + " " + pair.Value);
    } else if (pair.Key == end1)
    {
        Console.WriteLine(pair.Key.ToString("d") + " " + pair.Value);
    }
    else if (pair.Key == end2)
    {
        Console.WriteLine(pair.Key.ToString("d") + " " + pair.Value);
    }
    else if (pair.Key == end3)
    {
        Console.WriteLine(pair.Key.ToString("d") + " " + pair.Value);
    }
    else if (pair.Key == end4)
    {
        Console.WriteLine(pair.Key.ToString("d") + " " + pair.Value);
    }
    else if (pair.Key == end5)
    {
        Console.WriteLine(pair.Key.ToString("d") + " " + pair.Value);
    }
    else if (pair.Key == end6)
    {
        Console.WriteLine(pair.Key.ToString("d") + " " + pair.Value);
    }
}

If I understand your intent:

Console.WriteLine("Please enter a date in the following formate MM/DD/YYYY");
string date = Console.ReadLine();
DateTime startRange = Convert.ToDateTime(date);
DateTime endRange = startRange.AddDays(6);

foreach (var pair in pagesInRange)
    if (pair.Key >= startRange && pair.Key <= endRange)
        Console.WriteLine(pair.Key.ToString("d") + " " + pair.Value);

This does not take advantage of the fact that the dictionary is sorted.


Fast 32x32 bit multiply on ARM M0 I have a time-critical calculation running on an ARM Cortex-M0. This is the fastest 32x32 bit -> 64 bit multiplication that I can come up with.

The following C compiles almost 1-1 to assembly instructions:

//consider each arg as two 16-bit parts.
//r0 is Aa is A<<16+a. r1 is Bb is B<<16+b
//then Aa * Bb = (A*B)<<32 + (a*B+A*b)<<16 + a*b
//     result  =  Hh<<32   +  KmM<<16      + Ll

uint64_t mul32x32(uint32_t r0, uint32_t r1)
    register uint32_t r2= r1&0xFFFF;  // b
    register uint32_t r3= r1>>16;     // B
    register uint32_t r4= r0&0xFFFF;  // a
    r1 = r0 >> 16;                    // A
    r0 = r4;                // a
    r4 *= r2;               // Ll = a*b
    r0 *= r3;               // a*B
    r3 *= r1;               // Hh = B*A
    r2 *= r1;               // b*A
    r1 = 0;                 // K
    ADD_W_CARRY(r1,r0,r2);  // 0K.mM = a*B + b*A
    r1 <<= 16;              // K0
    r2 = r0 >> 16;          // 0m
    r0 <<=16;               // M0
    DBL_ADD_W_CARRY(r1,r0,r2,r4); // K0.M0 += 0m.Ll
    r1 += r3;                     // Km += Hh
    return (((uint64_t)r1)<<32)|r0;
}

It uses this bit of inline assembly, to deal with C's inability to express the carry flag:

//R.r+=x // clears x to avoid consuming another register
#define ADD_W_CARRY(R,r,x) __asm {\
        ADDS r,r,x;\
        MOVS x,0x0;\
        ADCS R,R,x;}
//pure C equiv is:  {r+=x;R+=(r<x);x=0;}

//R.r+=X.x
#define DBL_ADD_W_CARRY(R,r,X,x) __asm{\
        ADDS r,r,x;\
        ADCS R,R,X;}
//Pure C equiv is:  {r+=x;R+=X+(r<x); }

This compiles to:

PUSH     {r4}         ;// 2
LSRS     r3,r0,#16    ;// 1
UXTH     r2,r1        ;// 1
UXTH     r0,r0        ;// 1
MOV      r4,r0        ;// 1
LSRS     r1,r1,#16    ;// 1
MULS     r4,r2,r4     ;// 1
MULS     r0,r1,r0     ;// 1
MULS     r1,r3,r1     ;// 1
MULS     r2,r3,r2     ;// 1
ADDS     r0,r0,r2     ;// 1
MOVS     r3,#0        ;// 1
MOVS     r2,#0        ;// 1
ADCS     r3,r3,r2     ;// 1
LSLS     r2,r3,#16    ;// 1
LSRS     r3,r0,#16    ;// 1
LSLS     r0,r0,#16    ;// 1
ADDS     r0,r0,r4     ;// 1
ADCS     r2,r2,r3     ;// 1
POP      {r4}         ;// 2
ADDS     r1,r2,r1     ;// 1
BX       lr           ;// 3

                      ;== 26 cycles

Is it possible to do better?

First of all, yes, CortexM0 lacks any way to do 32x32=64 multiplication in hardware. CortexM3 and CortexM4 have the umull instruction, which lets you do 32x32=64 really easily.

And yes, since you're writing in C, one possible implementation would be

uint64_t mul32x32(uint32_t r0, uint32_t r1) { return r0*(uint64_t)r1; }

but I assume you've already tried that (with -O3 and whatever other optimization and inlining options you can turn up) and discovered that your compiler doesn't inline the multiplication, but leaves it as a call to some internal libc function.

A quick Google search turned up this previous StackOverflow question on exactly the same topic, where someone in the comments linked to GCC's implementation of 64x64=64 multiplication for CortexM0 (here), with the suggestion that you could constant-propagate "upper bits are known to be zero" through the whole thing by hand and that would give you something decent. I don't know if that's true.

Have you also benchmarked the "naive" approach of

uint64_t mul32x32(uint32_t r0, uint32_t r1)
{
    uint16_t r0h = r0 >> 16, r0l = r0 & 0xFFFF;
    uint16_t r1h = r1 >> 16, r1l = r1 & 0xFFFF;
    uint64_t result = (r0h * r1h);
    result <<= 16;
    result += r0h*r1l;
    result += r0l*r1h;
    result <<= 16;
    result += r0l*r1l;
    return result;
}

or equivalently

uint64_t mul32x32(uint32_t r0, uint32_t r1)
{
    uint16_t r0h = r0 >> 16, r0l = r0 & 0xFFFF;
    uint16_t r1h = r1 >> 16, r1l = r1 & 0xFFFF;
    return ((uint64_t)(r0h * r1h) << 32)
         + ((uint64_t)(r0h * r1l) << 16)
         + ((uint64_t)(r0l * r1h) << 16)
         + ((uint64_t)(r0l * r1l) << 0);
}

? As long as none of the arithmetic operations get turned into library function calls, this has the benefit of being portable ANSI C and being susceptible to inlining by the compiler. If you care about speed, susceptibility-to-inlining should be your #1 concern.

Since you have access to your compiler and we don't (I'm guessing Green Hills, from the __asm{ } syntax for inline assembly blocks?), you might get better answers if you posted the assembly that results from the above three C implementations.

Finally, note that CortexM0's MULS instruction takes either 1 cycle or 32 cycles, depending on the processor. If you're on one of those 32-cycle processors, doing four MULS instructions in a row is probably one of the worst things you can do. If MULS only takes 1 cycle, then you're probably okay; I don't think there's any need to space out those MULS instructions the way one might on a machine where they took multiple cycles (software pipelining).


Validate a string for positive number and normal string I have a method in which I am accepting a String clientid and that has below requirements:

clientid can be a positive number greater than zero. But if it is negative number or zero, then throw IllegalArgumentException with a message.
clientid cannot be a null or empty string. But if it is, then throw IllegalArgumentExceptionwith a message.
clientid can be a normal string as well. For example - it can be abcdefgh or any other string.

import static com.google.common.base.Preconditions.checkArgument;

public Builder setClientId(String clientid) {
    checkArgument(!Strings.isNullOrEmpty(clientid), "clientid cannot not be null or an empty string, found '%s'.",
            clientid);
    try {
        final long id = Long.parseLong(clientid);
        checkArgument(id > 0, "clientid must not be negative or zero, found '%s'.", clientid);
    } catch (NumberFormatException e) {}
    this.clientid = clientid;
    return this;
}

I cannot use Guava library greater than 11 so I cannot use Longs.tryParse method here so just using normal ugly way of parsing string. Is there anything I can improve here which is without try catch block? Anything from Apache Commons?

If all negative and zero number strings are invalid, you do not necessarily have to parse it to a long. You can use java regex (regular expressions) to test if the code is negative or zero:

string.matches("-\\d+") // Matches all negative integers, even if they are less than `Long.MIN_VALUE`
string.equals("0") // If the string is zero
string.matches("0+") // If the string consists of a bunch of zeroes, nothing else, or even better
string.matches("0*") // If the string contains nothing except zeroes, meaning this will also match an empty string

So, other than that, this code looks fine to me, but parsing values is fairly expensive and catching exceptions is very expensive, so using regular expressions is probably better.


Monoalphabetic Substitution Cipher In light of this months Community Challenge, I've taken a stab at building something related. Keep in mind, this isn't a submission for the challenge, just a utility to help in the later process of completing the challenge.

What we have here is your simple encryption and decryption (with a given key) tool. I'd like to know if what I have is the most efficient, the most well written, and the simplest form of these functions.

import random

alpha = "abcdefghijklmnopqrstuvwxyz"

def encrypt(original, key=None):

    if key is None:
        l = list(alpha)
        random.shuffle(l)
        key = "".join(l)

    new = []

    for letter in original:
        new.append(key[alpha.index(letter)])

    return ["".join(new), key]

def decrypt(cipher, key=None):
    if key is not None:
        new = []
        for letter in cipher:
            new.append(alpha[key.index(letter)])

        return "".join(new)
    # we'll have an else here for the actual Community Challenge!

I'm nowhere near an expert with Python, so if I'm not following coding standards, please guide me there too.

A simple test might be:

e = encrypt("helloworld", None)
print(e)
print(decrypt(e[0], e[1]))

and we'd get something along the lines of:

['wussndnkst', 'qxvtupaweroslynzhkbigcdjmf']
helloworld

"abcdefghijklmnopqrstuvwxyz" is just the constant string.ascii_lowercase.

Whenever you have this pattern:

some_list = []
for dummy in some_iterable:
    some_list.append(some_function_of(dummy))

… that's a candidate for replacement with a list comprehension.

alpha.index(letter) and its inverse could be a performance issue when processing longer messages.  Making a dictionary for the translation table would be better.  Note that any character that is not ASCII lowercase will cause a ValueError.  For encrypt(),

translation = dict(zip(key, string.ascii_lowercase))
ciphertext = ''.join(translation.get(c, c) for c in original)

Here, translation.get(c, c) causes unrecognized characters to be passed through.  You can use translation[c] to restore the original exception-raising behaviour.


Move generation n c++ for Knight So I have been working on this Chess project and for that I need a move generator. I wish to know if I can improve my code for the move generation for the Knight. Here is my current code;
Let me tell a few things so you can better understand it. I am using vector<vector<int>> for the main pseudo moves. I use add the move to a vector<int> first, then I add it to the pseudomoves.
The board is simply represented with int board[8][8]. Here are the values and the pieces that they correspond to;

1-Pawn
2-Bishop
3-Knight
5-Rook
6-Queen
10-King

The negative of the same numbers correspond to the black pieces!
This is the code for generating all moves of the knight.
                if (row > 0 && col < 6 && board[row-1][col+2] <= 0){
                    move.push_back(row);
                    move.push_back(col);
                    move.push_back(row-1);
                    move.push_back(col+2);
                    pseudomoves.push_back(move);
                    move.clear();
                }
                if (row > 1 && col < 7 && board[row-2][col+1] <= 0){
                    move.push_back(row);
                    move.push_back(col);
                    move.push_back(row-2);
                    move.push_back(col+1);
                    pseudomoves.push_back(move);
                    move.clear();
                }
                if (row < 7 && col < 6 && board[row+1][col+2] <= 0){
                    move.push_back(row);
                    move.push_back(col);
                    move.push_back(row+1);
                    move.push_back(col+2);
                    pseudomoves.push_back(move);
                    move.clear();
                }
                if (row < 6 && col < 7 && board[row+2][col+1] <= 0){
                    move.push_back(row);
                    move.push_back(col);
                    move.push_back(row+2);
                    move.push_back(col+1);
                    pseudomoves.push_back(move);
                    move.clear();
                }
                if (row < 6 && col > 0 && board[row+2][col-1] <= 0){
                    move.push_back(row);
                    move.push_back(col);
                    move.push_back(row+2);
                    move.push_back(col-1);
                    pseudomoves.push_back(move);
                    move.clear();
                }
                if (row < 7 && col > 1 && board[row+1][col-2] <= 0){
                    move.push_back(row);
                    move.push_back(col);
                    move.push_back(row+1);
                    move.push_back(col-2);
                    pseudomoves.push_back(move);
                    move.clear();
                }
                if (row > 1 && col > 0 && board[row-2][col-1] <= 0){
                    move.push_back(row);
                    move.push_back(col);
                    move.push_back(row-2);
                    move.push_back(col-1);
                    pseudomoves.push_back(move);
                    move.clear();
                }
                if (row > 0 && col > 2 && board[row-2][col-1] <= 0){
                    move.push_back(row);
                    move.push_back(col);
                    move.push_back(row-2);
                    move.push_back(col-1);
                    pseudomoves.push_back(move);
                    move.clear();
                }
}

Of course this can be improved. Look at all the repetition... then, factor it out into a function. You'll end up with something like this:
pseudomoves.push_back_if_legal(row, col, row-1, col+2);
pseudomoves.push_back_if_legal(row, col, row-2, col+1);
pseudomoves.push_back_if_legal(row, col, row+1, col+2);
pseudomoves.push_back_if_legal(row, col, row+2, col+1);
pseudomoves.push_back_if_legal(row, col, row+2, col-1);
pseudomoves.push_back_if_legal(row, col, row+1, col-2);
pseudomoves.push_back_if_legal(row, col, row-2, col-1);
pseudomoves.push_back_if_legal(row, col, row-2, col-1);

As a bonus, it is now relatively obvious where the bug is!


Simple and safe file reader for C As an exercise to improve my coding ability in C, I have attempted to write a simple and safe file reader. I believe this should be portable as well to major platforms (correct me if I'm wrong). For simplicity's sake I assume the file will fit in memory which is mostly true for my use case.

Please let me know where I can improve.

(Note: I've used goto because I think it makes the code flow better)

int read_file(const char* filename, char** buffer) {
    FILE* fp = fopen(filename, "r");
    if (!fp) {
        goto f_read_error;
    }

    ssize_t file_len;
    if (fseek(fp, 0L, SEEK_END) == 0) {
        file_len = ftell(fp);
        if (file_len == -1) {
            goto f_read_error;
        }
        if (fseek(fp, 0L, SEEK_SET) != 0) {
            goto f_read_error;
        }
        *buffer = (char*) malloc(sizeof(char) * (file_len + 1));

        int output = fread(*buffer, sizeof(char), file_len, fp);

        if (output != file_len) {
            goto f_read_error;
        };
        (*buffer)[file_len] = '\0';
    } else {
        f_read_error: ;
               char error_msg[1024];
               // let's trunc filenames larger than 256, to not overflow error_msg buffer
               char safe_filename[256];
               if (strlen(filename) > 256) {
                   memcpy(safe_filename, filename, 256);
               }
               sprintf(error_msg, "Unable to read file : %s", safe_filename);
               perror(error_msg);
               if (buffer) {
                   free(buffer);
               }
               if (fp) {
                   fclose(fp);
               }
               return -1;
     }
     if (fp) {
        fclose(fp);
     }
     return 1;
}

int main(int argc, char** argv)  {
      char* source = NULL;
      int success = read_file("test.txt", &source);
      if (success) {
            //read_file("test.txt", &source); - a copy paste error, edited as a comment
            printf("%s\n", source);
            free(source);
      }
}

This code is not safe, and exhibits undefined behavior. I don't think you've actually tested it.

           char safe_filename[256];
           if (strlen(filename) > 256) {
               memcpy(safe_filename, filename, 256);
           }
           sprintf(error_msg, "Unable to read file : %s", safe_filename);

Let's see what happens if filename is short. The test is false, so execution proceeds to the sprintf. It is constructing a string and is supposed to interpolate safe_filename... but that hasn't been initialized yet. Whatever garbage is in safe_filename will be copied into error_msg until it happens to hit a '\0', unless it gets a segfault first. It might even overflow the bounds of error_msg.

And what happens if filename is long? The test is true, so 256 characters of filename get copied into safe_filename. No terminating '\0' because memcpy() doesn't do that (and there wouldn't be room if it did). Execution proceeds to the sprintf. Again, it starts interpolating from safe_filename into the string, but the copying continues without stopping until it happens to hit a '\0' or segfaults. Same risk to error_msg.

How did this problem arise? You wrote code without testing it. Let me take a minute to pontificate about testing. You need to test your code. You need to test all your code. You need to be able to do the tests in an automated manner. That means your automatic testing harness needs to be able to cause each branch to be either taken or not taken.


Swift Attempt To Reduce Massive View Controller In an effort to reduce the size of a View Controller class I am wondering if this structure is an acceptable, safe way to modify the UI on a Controller Class.

The only thing that I see "wrong" with this approach is not being able to set my outlets or variables to private which is usually very import in order to minimize cohesion and keep the files conforming to Single Responsibility Principle

// In File ViewController.swift

final class ViewController: UIViewController {
    @IBOutlet weak var myButton: UIButton
    @IBOutlet weak var mylabel : UILabel
    @IBOutlet weak var myview  :  UIView

    private lazy var viewModel: VCViewModel = {
        return VCViewModel(main: self)
    }()

    override func viewDidLoad() {
        super.viewDidLoad()

        initializeUI()
    }

    func initializeUI() {
        viewModel.configureMyButton()
        viewModel.configureMyLabel()
        viewModel.configureMyView()
    }

    ...
}

// Seperate File VCViewModel.swift

final class VCViewModel {
    private main: UIViewController

    init(main: UIViewController) {
        self.main = main
    }

    func configureMyButton() {
        main.myButton.titleLabel.title = "This class configures all UI related code"
    }

    func configureMyLabel() {
        main.myLabel.backgroundColor = UIColor.green
    }

    func configureMyView() {
        main.myLabel.isHidden = true
    }
}

You should read more up on MVVM, It would be better for your ViewModel to have no UIKit Dependencies. Testing your implementation will be difficult too.

Ideally, public properties on your ViewModel  i.e titleText and color and in your VC;

self.titleLabel.text = viewModel.titleText
self.titleLabel.backGroundColor = viewModel.titleBackgroundColor


CodeLess Questions query As part of an exercise by Jamal to clean up old questions on Code Review that have no code in them, I helped by writing this query on Stack Exchange Data Explorer: CodeLess Questions

select Id as [Post Link], CreationDate
from Posts
where PostTypeId = 1 --questions
  and Body not like '%
%'
  and ClosedDate is null
order by CreationDate

I realize this is a relatively simple query, but I am always looking for ways to avoid like conditions (and not like), and this query irks me.

Note that the like condition is %
% - this is to trap the automatically managed code blocks that Stack Exchange uses to identify code. I cannot easily trap the open-tags because sometimes, when a custom syntax is used, the open tags are hard to search on.

I am looking for insights on whether the query can be improved or whether there are alternatives to the systems I have used for identifying code-less questions.

For reference, when Stack Exchange gets a question, the markup is converted to HTML and stored in the Body column.

An example is as follows:

MarkDown:

I didn't write this but I noticed it in our codebase at work.

    var data = object && object.attribute && object.attribute.data ? object.attribute.data : {};

I get that the programmer is trying to check that the variable
`object.attribute.data` is defined before using it, but is seems like
so much repeated code. What's the best way to assign `data` the value
of `object.attribute.data` if it's defined, otherwise giving it a
value of `{}`?

HTML Encoding:

I didn't write this but I noticed it in our codebase at work.

var data = object &amp;&amp; object.attribute &amp;&amp; object.attribute.data ? object.attribute.data : {};

I get that the programmer is trying to check that the variable object.attribute.datais defined before using it, but is
seems like so much repeated code. What's the best way to assign
data the value of object.attribute.data if
it's defined, otherwise giving it a value of {}?

I have never used it before but I think what you are looking for is CHARINDEX I found a StackOverflow Answer that has several upvotes and no downvotes using it like this

WHERE CHARINDEX('
', Body) = 0

So far from what I have seen they are relatively close to the same speed, and is giving the same results as your query.

I would much rather read this than Body not like '%
%'

I think we have found a new "hammer", let's find some nails.

Read more about CHARINDEX Here

There is also a CONTAINS function in T-SQL, but unfortunately it cannot be used on that column of that table because the column is not Full-Text Indexed

Read more about CONTAINS Here


Php bad glob pattern I am getting images using this glob pattern, but the code looks really bad.

First I get all images which have the name screenshot + a number from 0-9 in an array, after that I get all screenshort + a number from 10-99 and so on.

I want to improve this code.

    $dirScreenshotsOne[]   = glob($value.'/screenshot[0-9].png'); // 0-9
    $dirScreenshotsTwo[]   = glob($value.'/screenshot[0-9][0-9].png'); // 10-99
    $dirScreenshotsThree[] = glob($value.'/screenshot[0-9][0-9][0-9].png'); // 100-999

        $dirScreenshotsAll = array_merge(
                         $dirScreenshotsOne,
                         $dirScreenshotsTwo,
                         $dirScreenshotsThree);

    foreach($dirScreenshotsAll as $screenshotKey => $screenshotValue)
    ...

Consider working with SPL's fileinfo and iterator classes  (i.e. SplFileInfo, DirectoryIterator and FilterIterator, CallbackFilterIterator, etc) to build something a little more resilient.

Example:

// your filter logic
$imageFilter = function (SplFileInfo $splFileInfo) {
    return preg_match(
        '/^screenshot[0-9]{1,3}\.png$/',
        $splFileInfo->getFilename()
    ) ? true : false;
}

// get files in directory based on filter function
try {
    $directoryIterator = new DirectoryIterator($directoryPath);
    $filteredImageIterator = new CallbackFilterIterator(
         $directoryIterator,
         $imageFilter
    );
} catch (Exception $e) {
     // do something here, or don't wrap in try-catch at all if you want to bubble up exception.
}

foreach($filteredImageIterator as $image) { /* your code */ }

Note that you can build upon the filter shown here by adding things like verifying file is readable/writable, get file modification timestamps, or other such functionality exposed by SplFileInfo, which could include ability to easily create SplFileObject for working with file contents.


Naming a complex crud function I have a function below that does the following uses a bunch of smaller CRUD operations which I call create, retrieve (instead of read), update, delete. Because this function does a bit of everything it can be hard to name it.

Here's a bit about its behavior:

pass in array of redirects (objects with url, and path properties)
retrieves all existing redirects
checks to see if requesting redirect path exists
if existing redirect with path exists and url is the same returns existing
if existing redirect with path exists updates redirect
if existing redirect with path does not exist creates new redirect

Here are some possible candidates:

retrieveAllRedirectsUpdateExistingRedirectsCreateNewRedirectsDoNothingForRedirectsWithNoChange
retrieveAllUpdateExistingCreateNewRedirects
retrieveUpdateCreateRedirects
ensureRedirectsExist
ensureRedirects
overwriteRedirects // implies will delete redirects not passed to it
createRedirects
redirects

Here's the function:

Shopify.prototype.ensureRedirects = function(redirects){
  return this.retrieveAllRedirects().then(function(existingRedirects){
    return Promise.map(redirects, function(redirect){
      return Promise.resolve(existingRedirects).then(_).call("findWhere", {
        "path": redirect.path,
      }).then(function(match){
        if(match && match.url == redirect.url) return match
        if(match) return this.updateRedirect(match.id, redirect)
        return this.createRedirect(redirect)
      }.bind(this))
    }.bind(this))
  }.bind(this))
}

What naming convention lends itself to the most flexibility and follows the best pattern?

Other functions that I have in this library include but aren't limited to:

retrieveRedirects (paginated)
retrieveRedirectsCount
retrieveAllRedirects (all pages)
createRedirect
updateRedirect

This also begs the question should this function above be createRedirects as an alias of createRedirect that detects argument type object v.s. array and creates the redirects accordingly, making the createRedirect function more versatile to handle argument types? Then what do you name the function and argument? Convention for naming a function or variable as both plural and singular

Abstractions
When working with collections, there are three layers of functions - each essentially justifying a module:
  h(url[], url[]) -> url[]
  g(url, url[])   -> url[]
  f(url1, url2)   -> url

The dependencies are h <- g <- f. The functions at f are more reusable than those at g are more reusable than those at the highest level h.
Complected.
I'll reflect my bias:  If a function is so complex it is hard to name, it might need to be refactored.
The place to start is with a function [let's call it g] that takes one item from redirects and modifies existingRedirects atomically. Then the existing function ensureRedirects can focus on operations at the level of the collection.
Conceptual mangling of the three levels - processing a collection of possible changes versus processing one possible change versus comparing two items - is clear from the description of the ensureRedirects's behavior in the question. A lot of it is at the level of processing a single URL against a collection of URL's even though ensureRedirects operates on collections. Some of it is about dealing with two URL's.
The naming should flow from smaller pieces to larger pieces. Or h could just be redirects.foreach(g)


Javascript typehead using debounce This is the code I wrote for making a JavaScript typeahead/autocomplete, which displays cities based on the user input.

It would be great if you could suggest some improvements on the code quality.

'use strict';

var data = ['alabama','alaska', 'arizona','arkansas','california', 'colorado', 'connecticut', 'delaware'];

/**
 * variables: searchBox is where you type the city name
*/
var searchBox = getElementById('typeahead');

/**
 * methods
 */
function getElementById(id) {
    return document.getElementById(id);
}

function iterate(arr, callback) {
    for(var i=0;i<arr.length;i++) {
        callback(arr[i], i);
    }
}

var search = function(val) {
    var arr = [];
    iterate(data, function(item, index) {
        if(item.indexOf(val) !== -1) {
            arr.push(item);
        }
    })
    return arr;
}

var addEvent = function(elm) {
    elm.addEventListener('click', function(event) {
        if(event.target !== event.currentTarget) {
            searchBox.value = event.target.textContent;
            removeChild();
        }
        event.stopPropagation();
    });
}

var appendChild = function(parent, child) {
    parent.appendChild(child);
}

var removeChild = function() {
    var ul = getElementById('searchResults');
    if(ul) {
        document.body.removeChild(ul);
    }
}

var bindData = function(arr) {

    var ul = document.createElement('ul');
    ul.setAttribute('id','searchResults')
    iterate(arr, function(item, index) {
        var li = document.createElement('li');
        var a = document.createElement('a');
        a.textContent = item;
        appendChild(li, a)
        appendChild(ul, li);
    });

    appendChild(document.body, ul);
    addEvent(ul);
}

function debounce(func, wait, immediate) {
    var timeout;
    return function() {
        var context = this, args = arguments;
        var later = function() {
            timeout = null;
            if(!immediate) {
                func.apply(context, args);
            }
        };
        var callNow = immediate && !timeout;
        clearTimeout(timeout);
        timeout = setTimeout(later, wait);
        if(callNow) {
            func.apply(context, args);
        }
    };
}

var typeAhead = debounce(function(event) {
    removeChild();
    var value = event.target.value;
    if(value !== '') {
        var arr = search(value);
        bindData(arr);
    }
}, 400);

searchBox.addEventListener('keyup', typeAhead);
<input type="text" placeholder="type to search" id="typeahead">

DOM querying is expensive
Search on Google for "js DOM query expensive" and you will likely find many posts from the past 10 years that discuss how in-efficient it is to be querying the DOM each time. Stop Writing Slow Javascript appears to be somewhat recent - see the section Cache DOM Lookups. This answer on SO about various DOM-selector functions will likely be interesting as well.
In the code below, notice that the variables searchBox and ul are declared at the top
var searchBox, ul;

Those variables don't get assigned until the callback for a new event listener for the DOMContentLoaded event is triggered.
document.addEventListener("DOMContentLoaded", function(event) {
    searchBox = getElementById('typeahead');
    searchBox.addEventListener('keyup', typeAhead);

    ul = document.createElement('ul');
    ul.setAttribute('id', 'searchResults')
    appendChild(document.body, ul);
    hideList();
    addEvent(ul);
});

You will also notice that in that event callback, the unordered list (i.e. ul) is added to the document and hidden. That way there is only one list ever added, and because of that, we can hide and show the list instead of removing it. The function removeChild can be replaced with a function hideList that will set the display style (using HTMLElement.style) to none. Additionally, the call to addEvent() was moved into that callback function, so we only add the event listener once. One could also use event delegation and have one click handler for the whole page - it would just handle clicks differently depending on the type of element clicked.
Keep the scopes limited
Another thing that article mentions is limiting the scope as much as possible (see the section Keep your scopes close and your scope even closer). One way to not clutter up the global namespace is to wrap the code with an IIFE:
;(function(window, document, undefined) {
    var data = ['alabama', 'alaska', 'arizona', 'arkansas', 'california', 'colorado', 'connecticut', 'delaware'];
    //...
})(window, document);

See the changes applied below. There are likely other improvements that can be made as well...

'use strict';;
;(function(window, document, undefined) {
  var data = ['alabama', 'alaska', 'arizona', 'arkansas', 'california', 'colorado', 'connecticut', 'delaware'];

  /**
   * variables: searchBox is where you type the city name, ul is the list of suggestions
   */
  var searchBox, ul;

  /**
   * methods
   */
  function getElementById(id) {
    return document.getElementById(id);
  }

  function iterate(arr, callback) {
    for (var i = 0; i < arr.length; i++) {
      callback(arr[i], i);
    }
  }

  var search = function(val) {
    var arr = [];
    iterate(data, function(item, index) {
      if (item.indexOf(val) !== -1) {
        arr.push(item);
      }
    })
    return arr;
  }

  var addEvent = function(elm) {
    elm.addEventListener('click', function(event) {
    console.log('list click - target: ',event.target,' currentTarget: ',event.currentTarget);
      if (event.target !== event.currentTarget) {
        searchBox.value = event.target.textContent;
        hideList();
      }
      event.stopPropagation();
    });
  }

  var appendChild = function(parent, child) {
    parent.appendChild(child);
  }
  var hideList = function() {
    ul.style.display = 'none';
    while (ul.firstChild) {
      ul.removeChild(ul.firstChild);
    }
  }

  var bindData = function(arr) {
    ul.style.display = '';
    iterate(arr, function(item, index) {
      var li = document.createElement('li');
      var a = document.createElement('a');
      a.textContent = item;
      appendChild(li, a)
      appendChild(ul, li);
    });
  }

  function debounce(func, wait, immediate) {
    var timeout;
    return function() {
      var context = this,
        args = arguments;
      var later = function() {
        timeout = null;
        if (!immediate) {
          func.apply(context, args);
        }
      };
      var callNow = immediate && !timeout;
      clearTimeout(timeout);
      timeout = setTimeout(later, wait);
      if (callNow) {
        func.apply(context, args);
      }
    };
  }

  var typeAhead = debounce(function(event) {
    hideList(); //removeChild();
    var value = event.target.value;
    if (value !== '') {
      var arr = search(value);
      bindData(arr);
    }
  }, 400);

  document.addEventListener("DOMContentLoaded", function(event) {
    searchBox = getElementById('typeahead');
    searchBox.addEventListener('keyup', typeAhead);

    ul = document.createElement('ul');
    ul.setAttribute('id', 'searchResults')
    appendChild(document.body, ul);
    hideList();
    addEvent(ul);
  });
})(window, document);
<input type="text" placeholder="type to search" id="typeahead">


Modern HTTP-Requests for login/register system I am working on a login/register system and want the register and log in process as smooth as possible. One way of making it smoother is in my opinion to make a multi-step login/register.

What I mean: If a user want's to log in, he first have to type in his username and if this username exists, he can type in a password and log in. The user has more feedback, especially in the register process. Since I started to learn OOP and know that es6 was introduces a while ago, I want to improve in JS aswell.

I only have one button that does everything. That's why I have a switch case in my button event listener that compares the value of it to select the right method in my form class.

The code at the bottom is what I have came up with. It works but I don't think it is how it's done normally.

Is this code 'advanced' or does it look like it's from a noob.

I am aware that doing a login like that leaks information, but I have already setup a robust protection against it. So that attackers don't have a lot of tries, while users can login wihtout a DoS.

class Form {
    content = document.getElementById('content');
    username = document.getElementById('username');
    password = document.getElementById('password');
    submit_btn = document.getElementById('right');
    loader = document.getElementById('loader');
    request;

    constructor () {
        this.request = new XMLHttpRequest();
    }

    execute(type, path, Data) {
        this.loading();
        this.request.open(type, path);
        this.request.setRequestHeader('Content-type', 'application/x-www-form-urlencoded');
        this.request.send(Data);
    }

    loading() {
        this.loader.style.opacity = 1;
    }

    loaded() {
        this.loader.style.opacity = 0;
    }

    submit_username () {
        this.request.onload = () => {
            this.loaded();

            //If response value is true than
            //Switch Input field to password
            //Change button value to login_two
            //else show error messages

        }

        let Data = `username=${this.username.value}`

        this.execute('post', '/login/step_one', Data);
    }

    submit_login() {
        this.request.onload = () => {
            this.loaded();

            //If response value is true than
            //Redirect the user to home
            //else show error messages

        }

        let Data = `password=${this.password.value}`

        this.execute('post', '/login/step_two', Data);

    }

}

const form = new Form();

form.submit_btn.addEventListener("click", () => {
    let method = form.submit_btn.value;
    switch (method) {
        case 'login_one':
            form.submit_username();
        break;
        case 'login_two':
            form.submit_login();
        break;
    }
});

Maybe someone can provide an example of a better way.

Codepen

Declaring a class makes sense if it can be reused, but less so if the logic and element ids are hardcoded to just one case, it only adds the unnecessary this. to variables.
It's unclear why the click listener is handled outside of the form class as it's logically and conceptually a part of the form.
JS naming convention is camelCase, not snake_case.
Consider URLSearchParams API so the HTTP headers are set automatically and values are properly escaped.
Consider making the login routes more declarative e.g. routes in the code below.
If your goal was to use a class as a namespace then it might be better to switch to modules. For example using the built-in ES modules the inner contents of the function will be the entire contents of the module.

function handleLogin() {
  const request = new XMLHttpRequest();
  const loader = document.getElementById('loader');
  const btn = document.getElementById('right');
  let step = 0;
  const routes = [{
    path: 'one',
    field: 'username',
    onload() {
      step++;
      getDataElement().focus();
    },
    onerror(e) { /* show error */ },
  }, {
    path: 'two',
    field: 'password',
    onload() { /* show home page */ },
    onerror(e) { /* show error */ },
  }];

  btn.addEventListener('click', () => {
    loader.style.opacity = 1;
    const {path, field} = routes[step];
    request.open('post', `/login/step_${path}`);
    request.send(new URLSearchParams({[field]: getDataElement().value}));
  });

  request.addEventListener('loadend', e => {
    loader.style.opacity = 0;
    routes[step][request.response === 'true' ? 'onload' : 'onerror'](e);
  });

  function getDataElement() {
    return document.getElementById(routes[step].field);
  }
}


JavaScript - prototype, getters, setters, functions I'm just getting to know the whole prototype world, and now I'm trying to do in JS what is normally done in OO languages, namely classes and DAO classes.

I'd be grateful for any comments on whether the code is written in accordance to JS best practices and whatnot. More specifically, I didn't implement getters or setters per se, is my approach still sound?

function Person(obj) {
  this.id = obj.id;
  this.name = obj.name;
  this.surname = obj.surname;
}

Person.prototype.toString = function(){
  return "["+this.id+"] name: " + this.name + ", surname: " + this.surname;
};

/*
Field.prototype = {
    get value(){
        return this._value;
    },
    set value(val){
        this._value = val;
    }
};
*/

function PersonDAO(){
   this.arrayOfPeople = [];
}

PersonDAO.prototype = {
  save:function(personObj){
    this.arrayOfPeople.push(personObj);
  },
  getAll:function(){
    return this.arrayOfPeople;
  }
};

var John = new Person({
  id: 1,
  name: "John",
  surname: "Smith"
});

var Bob = new Person({
  id: 2,
  name: "Bob",
  surname: "Doe"
});

var pDAO = new PersonDAO();
pDAO.save(John);
pDAO.save(Bob);
console.log(pDAO.getAll()[0].toString());

Though dynamic, you should not force JavaScript it to use complicated patterns that aren't worth the hassle. In your case, the code you want can be simplified into:

var people = [];

people.push({
  id: 1,
  name: "John",
  surname: "Smith"
});

people.push({
  id: 2,
  name: "Bob",
  surname: "Doe"
});

Another thing is that JavaScript has no natural concept of privacy (save closures of course). So when doing some prototypal OOP, almost everything is public. Getters and setters might not hold much purpose.

The key to a big JavaScript app is to never make it big in the first place. Keep components small, simple and easy to debug in case they go wild. Additional baggage will drag you down eventually.

Also, I suggest you don't modify native methods. Though it's common that developers do override some of them, like toString(). But some other code might use it in a way that it expects the native functionality. A general rule in JS is don't modify objects you don't own.


Polling script to determine the continuous time a user has been connected I've been asked to keep track of how long a user has been connected to a site without interruption.

So far the solution I've come up with is to use ajax to poll the site every now and then to check how much time has elapsed since the last call.

Any suggestions for possible improvements would be appreciated.

P.S: Not sure why but text isn't displaying if no text is in the div or echoed.

The javascript

The PHP code

if( isset( $_POST["data"] ) ){
    $id = (int) $_POST["data"];

    $sql ='INSERT INTO users_timeconnected ( profil_id, last_ajax_call ) VALUES ('. $id .', NOW() ) ON DUPLICATE KEY UPDATE id=id';

    $db->query( $sql );

    $sql ="
    select (
        (
            unix_timestamp( now() )
            - unix_timestamp( (select last_ajax_call from users_timeconnected where profil_id =" . $id ." ) )
        ) / 60
    )";

    $db->query( $sql );

    $timeSinceLastCall = $db->fetch_one();

    $sql = 'select minutes_connected from users_timeconnected where profil_id =' . $id;

    $db->query( $sql );

    $minutesConnected = $db->fetch_one();

    echo "current time:" . date("d/m/y : H:i:s", time()).'
';
    echo "id:" . $id .'
';
    echo "time since last call:" .$timeSinceLastCall.'
';
    echo "connected:" . $minutesConnected.'
';
    echo "lower than or equal to 6:" . (int)($timeSinceLastCall <= 6);

    if( $timeSinceLastCall <= 6 ){
        $timeSinceLastCall = 0; // reset
        $sql ='UPDATE users_timeconnected SET minutes_connected = minutes_connected+5 WHERE profil_id='. $id ;
        $db->query( $sql );
        $sql ='UPDATE users_timeconnected SET last_ajax_call = NOW() WHERE profil_id='. $id;
        $db->query( $sql );
    }
}

The SQL table

CREATE TABLE IF NOT EXISTS `users_timeconnected` (
  `id` int(10) unsigned NOT NULL auto_increment,
  `profile_id` int(10) unsigned NOT NULL default '0',
  `last_ajax_call` timestamp NULL default NULL,
  `minutes_connected` int(10) default '0',
  PRIMARY KEY  (`id`),
  UNIQUE KEY `profil_id` (`profile_id`)
);

The html for tests

Output

Structure of your Javascript is tightly coupled with your logic, decoupling will allow change without major changes.

a) Move you Ajax initialization [xmlHttp] to a usable function.

b) UnIntended display of UserId req.send("data=<?php echo $user->id; ?>"); as plain text inside your javaScript is security risk

c) Refactor your client script and least packit(dean edwards works ok) and compress it.

d) Using a comet based server is Best solution to this rather than polling the server at defined intervals. I would recommend you learn APE Project

another point! you can do it this way, when user navigates away from page ask them a few seconds to send ajax request. You have login time and now you have leaving time , calculate Hit rate yourself.
Also using LocalStorage will be viable option too


Writing values from JSON on image and display image This is a script I made that opens img.png, downloads the JSON data from the API, writes it on image and displays it. It currently is a community ad over Arqade.

<?php
header('Content-type: image/png'); //setting header to png so it will display as an image
$donationImage = imagecreatefrompng("img.png"); //loading the donation image's base from the file img.png
$white = imagecolorallocate($donationImage, 255, 255, 255);

$json = json_decode(utf8_encode(file_get_contents("http://www.mariomarathon.com/rest/partners/arqade.com")));
$total = round($json->total); //Rounding the total amount because it uses floats
$byus = $json->domainTotal;

imagettftext($donationImage,30,0,60,450,$white,'arial.ttf',"$".$total); //writing the total count to x:60 y:450
imagettftext($donationImage,30,0,370,450,$white,'arial.ttf',"$".$byus); //writing the donations by arqade.com to x:370 y:450
imagepng($donationImage); //displaying image
imagedestroy($donationImage); //removing image from memory
?>

Code and additional resources are available at GitHub. This file is the one named mariobeta.php.

How can I improve this code? I'm not that good with PHP but I think that the code doesn't have any major issues.

header('Content-type: image/png');
imagepng($donationImage);

No real point in specifying the content type prior to actually outputting the image.

As mentioned in the other answer, json_decode(utf8_encode(file_get_contents("URI"))) can error out in multiple ways. If error reporting is active, this will result in a mismatch between content type and the response body. Even if it isn't, you end up sending a wrong content type for an empty body.

I hope you have placed a cache in front of that script. You are stalling the script with a REST query, and the memory footprint isn't negligible either.

If the web server isn't configured to cache, you want to fix that.

Caching the output with plain PHP is slightly more complex than that, with regard to acquiring a mutex on the cache during updates.

?>

Eventually you will end up with a newline following the closing code tag (be it the IDE or the VCS software being "smart"). This will add to the output, resulting in an invalid response.

Avoid using the closing code tag if there isn't any content following. This actually applies to all PHP files.


Use of external memory or a custom allocator I'm creating a class which uses a custom buffer. I want to offer the possibility to pass an external memory address (for higher interoperability between languages) or (for convenience) to specify a custom allocator type. The following code outlines what I mean:

template<typename int_type, class alloc = void>
class uses_custom_buffers
    : uses_custom_buffers<int_type, void>
{
public:
    uses_custom_buffers<int_type, alloc>* set_buffer(std::size_t count, std::size_t size)
    {
        typename alloc::rebind<int_type*>::other pointer_alloc;
        int_type** buffer = pointer_alloc.allocate(count);
        for (std::size_t i = 0; i < count; ++i)
            buffer[i] = this->m_alloc.allocate(size);
        this->uses_custom_buffers<int_type, void>::set_buffer(count, buffer, size);
        return this;
    }

private:
    using uses_custom_buffers<int_type, void>::set_buffer;

    alloc m_alloc;
};

template<typename int_type>
class uses_custom_buffers<int_type, void>
{
public:
    uses_custom_buffers<int_type, void>* set_buffer(std::size_t count, int_type** buffers, std::size_t size)
    {
        this->m_buf   = buffers;
        this->m_count = count;
        this->m_size  = size;
        return this;
    }

private:
    int_type**  m_buf;
    std::size_t m_count,
                m_size;
};

Please note: This example doesn't care about deallocating any resource or exception safeness (to simplify matters).
Do you see any kind of problems with that design?

I think that in order to be a good review candidate, this should be more than an "outline." Like, there should be an example of how you intend to use it.

But here's some stylistic feedback:

CamelCase your template parameter names: IntType (or just T), Alloc.
You can (and should) use the injected class-name uses_custom_buffers inside the class itself, rather than typing out uses_custom_buffers<int_type, alloc> every time.
Don't define multiple (member) variables on the same line.
Mark your constructors explicit unless you want to enable the implicit conversion for some specific reason.
Explicitly mark your base classes public and private, for clarity.
Writing std::size_t instead of size_t, or typename instead of class, is just extra keyboard practice. Personally, I always go for the shorter versions.
Always brace your if and for bodies. Don't goto fail!

    this->uses_custom_buffers<int_type, void>::set_buffer(count, buffer, size);

This seems complicated. Ideally we'd just write

    set_buffer(count, buffer, size);

But since that set_buffer is located in a dependent base class, we actually have to write this-> in front:

    this->set_buffer(count, buffer, size);

And then we still have trouble, because the declaration of set_buffer in uses_custom_buffers<int_type, alloc> hides the declaration of set_buffer in uses_custom_buffers<int_type, void>! There are two clean ways to fix this, depending on what you want to do. The first is to bring the hidden set_buffer back into scope with a using-declaration:

using uses_custom_buffers<int_type, void>::set_buffer;

The second is to pick a different name for one or both of the functions in this overload set.

  "If you have two [functions] that are doing something very very different, please, name them differently." —Titus Winters, 2018

I would even argue that uses_custom_buffers<int_type, void> is doing something "very very different" from uses_custom_buffers<int_type, alloc>, and therefore it should be named differently.

Very important:

typename alloc::rebind<int_type*>::other pointer_alloc;

This is (A) missing a template keyword, and (B) waaay too complicated for one line of code. Break it down by using a typedef:

using PointerAlloc = typename alloc::template rebind<int_type *>::other;
PointerAlloc pointer_alloc;

However, this is (C) still broken, because it fails to use allocator_traits. You need to write this instead:

using PointerTraits = typename std::allocator_traits<alloc>::template rebind_traits<int_type *>;
using PointerAlloc = typename std::allocator_traits<alloc>::template rebind_alloc<int_type *>;
PointerAlloc pointer_alloc;

And then on the next line:

int_type** buffer = pointer_alloc.allocate(count);

should be

int_type **buffer = PointerTraits::allocate(pointer_alloc, count);

However, after all that, I am still super duper confused about where your allocator is supposed to come from! You just default-constructed it and immediately used it to allocate some memory? Where is the memory supposed to come from?

If you want to use the standard allocator model, you need to provide a way for the user to pass in an allocator for you to use. You can't just default-construct one and expect it to magically know where its heap is located. (That happens to work for std::allocator because it just uses new and delete, which are global; but it is highly unlikely to work for any user-provided allocator type.)

Let's put it all together and see how it looks:

template<class T>
class use_custom_buffers_base {
public:
    use_custom_buffers_base *set_buffers(size_t count, T **buffers, size_t size) {
        this->m_buf   = buffers;
        this->m_count = count;
        this->m_size  = size;
        return this;
    }

private:
    T **m_buf;
    size_t m_count;
    size_t m_size;
};

template<class T, class Alloc>
class use_custom_buffers : private use_custom_buffers_base<T>
{
    using Base = use_custom_buffers_base<T>;
    using ATraits = std::allocator_traits<Alloc>;
    using PTraits = typename ATraits::template rebind_traits<T*>;
    using PAlloc = typename PTraits::allocator_type;

public:
    explicit use_custom_buffers(Alloc alloc) : m_alloc(std::move(alloc)) {}

    use_custom_buffers *set_buffers(size_t count, size_t size) {
        PAlloc pointer_alloc(m_alloc);
        T **buffers = PTraits::allocate(pointer_alloc, count);
        for (size_t i = 0; i < count; ++i) {
            buffers[i] = ATraits::allocate(m_alloc, size);
        }
        this->Base::set_buffers(count, buffers, size);
        return this;
    }

private:
    Alloc m_alloc;
};

There's still work to do. count and size are strange names, especially since size is also a count (of T objects), not the "size" of any entity in the program. I decided to punt on the question of what to call Base::set_buffers. The arguments to set_buffers are in a weird order (length, pointer, other-length). It is unclear from your description whether anyone in the codebase actually cares about use_custom_buffers_base or whether it could be completely hidden away in a detail namespace — or, indeed, inlined into use_custom_buffers, since the inheritance relationship here seems like it's doing more harm than good.

Finally, a possible performance issue: Why are you calling allocate in a loop?

        for (size_t i = 0; i < count; ++i) {
            buffers[i] = ATraits::allocate(m_alloc, size);
        }

Surely it would be more performant to allocate just once and then use pointers into different parts of that buffer?

        auto ptr = ATraits::allocate(m_alloc, count * size);
        for (size_t i = 0; i < count; ++i) {
            buffers[i] = ptr + (i * size);
        }


Edit/Details form in JavaScript I have a Edit/Details form which has 4 user related fields. On click of Save, I save the edited fields to local storage (if supported) and display the same values in the Details view.

Below is the code which does that;

  var UserDataObj = {
name:"John",
email:"john@example.com",
phone:"9999999999",
desc:"some description"
}

/**
 * Check HTML5 Local Storage support
 */
function supports_html5_storage()
{
    try {
        window.localStorage.setItem( 'checkLocalStorage', true );
        return true;
    } catch ( error ) {
        return false;
    };
};

/**
 * Save form data to local storage
 */
function saveFormData()
{
    if (supports_html5_storage())
    {
    $(".formFieldUserData").each(function(){
        UserDataObj[$(this).attr("name")] = $(this).val();
    });

    localStorage.setItem('UserDataObj', JSON.stringify(UserDataObj));
    }
}

/**
 * Set field values based on local storage
 */
function setFormFieldValues()
{
    if (supports_html5_storage())
    {
            var retrievedUserDataObj = JSON.parse(localStorage.getItem('UserDataObj'));
            if(retrievedUserDataObj)
            {
                $(".formFieldUserData").each(function(){
                    var currentKey = $(this).attr("name");
                    var retrievedValue = retrievedUserDataObj[$(this).attr("name")]
                    $("#"+currentKey).val(retrievedValue);
                    $("#"+currentKey+"Txt").html(retrievedValue);
                });
            }
            else
            {
                $(".formFieldUserData").each(function(){
                    var currentKey = $(this).attr("name");
                    var userDataObjValue = UserDataObj[$(this).attr("name")]
                    $("#"+currentKey).val(userDataObjValue);
                    $("#"+currentKey+"Txt").html(userDataObjValue);
                });
            }
    }
    else
    {
        $(".formFieldUserData").each(function(){
        var currentKey = $(this).attr("name");
            if ($(this).val() != "")
            {
            var fieldValue = $(this).val();
            $("#"+currentKey).val(fieldValue);
            $("#"+currentKey+"Txt").html(fieldValue);
            }
            else
            {
            var defaultuserDataObjValue = UserDataObj[$(this).attr("name")];
            $("#"+currentKey).val(defaultuserDataObjValue);
            $("#"+currentKey+"Txt").html(defaultuserDataObjValue);
            }
        })
    }
}

My question is;

This is a completely working code. But is there any further that I can do as far as best coding practices go.
Also performance wise, can I do anything to improve the overall code?

You can start with how you code. Have them indented properly and checked by a quality-check tool. Code-review doesn't only deal with optimization, but also maintainability of your code and that includes making it readable.
After running it in JSHint, I found some minor problems in your code. Most notable are missing semi-colons. Although semi-colons in line endings are optional in JS, it's still best practice to include them. You could also avoid code packer/compressor errors by actually putting them in. This avoids your lines from being connected to the next line.
Use strict comparison as much as possible in JS. JS treats "falsy" values as false, which includes null, undefined, 0, false, a blank string and  NaN. For example, `"" == false is actually equal.
Place your code in a namespace, or some inner-scope to avoid polluting the global scope.

For code related stuff:

(function (ns, $) {
  "use strict"; //prevents "bad practice"

  //variables needed for localStorage test
  var storage, fail, uid;

  //For flexibility, it would be best if your code
  //wasn't build for a specific form only
  //instead, allow it to create instances and attach
  //to different forms with different default data
  function FormSave(selector, defaultValues) {
    this.defaultValues = defaultValues || {};
    this.dataName = selector;
    this.form = $(selector);
    this.userFields = $('.formFieldUserData', this.form);
  }

  //our save function
  FormSave.prototype.save = function () {

    var userData = {};

    //using the "early return" pattern to avoid further
    //code exectution if condition fails, and avoid
    //deep indentation
    if (!storage) {
      return;
    }

    //since each() assigns the current DOM element in
    //the collection as this, we can get name and value
    //directly from the DOM element, saving you function calls
    this.userFields.each(function () {
      userData[this.name] = this.value;
    });

    //set the items
    localStorage.setItem(this.dataName, JSON.stringify(userData));
  };

  //our setting function
  FormSave.prototype.set = function () {

    //cache the default values into this scope
    var defaultValues = this.defaultObject,
      retrievedValues;

    //if local storage is present, get and parse
    if (storage) {
      retrievedValues = JSON.parse(localStorage.getItem(this.dataName));
    }

    this.userFields.each(function () {

      //cache the current key
      var currentKey = this.name,
        //then the value to be used is:
        //if storage is present, use either the retrieved values or the default
        //if not, then use the present value or the default
        value = ((storage) ? retrievedValues[currentKey]: this.value) || defaultValues[currentKey];

      //for safe text insertion, use text() since it escapes the value first
      $("#" + currentKey + "Txt").text(value);
      $("#" + currentKey).val(value);
    });

    //our easy binding function exposed to our FormSave namespace
    ns.bind = function (selector) {
      return new FormSave(selector);
    };
  };

  //a quick test for localstorage
  //from: http://mathiasbynens.be/notes/localstorage-pattern
  try {
    uid = new Date();
    (storage = window.localStorage).setItem(uid, uid);
    fail = storage.getItem(uid) !== uid;
    storage.removeItem(uid);
    fail && (storage = false);
  } catch (e) {}

}(this.FormSave = this.FormSave || {}, jQuery));

//usage:

//bind this functionality to a form id'ed "someForm" with
//the given default values
var someForm = FormSave.bind('#someForm', {
  name: 'John',
  email: 'john@example.com',
  phone: '9999999999',
  desc: 'some description'
});

//then with the given reference, call save and set
someForm.save();
someForm.set();

When packed, it's this small!

(function (ns, $) {
  "use strict";
  var storage, fail, uid;

  function FormSave(selector, defaultValues) {
    this.defaultValues = defaultValues || {};
    this.dataName = selector;
    this.form = $(selector);
    this.userFields = $('.formFieldUserData', this.form)
  }
  FormSave.prototype.save = function () {
    var userData = {};
    if (!storage) {
      return
    }
    this.userFields.each(function () {
      userData[this.name] = this.value
    });
    localStorage.setItem(this.dataName, JSON.stringify(userData))
  };
  FormSave.prototype.set = function () {
    var defaultValues = this.defaultObject,
      retrievedValues;
    if (storage) {
      retrievedValues = JSON.parse(localStorage.getItem(this.dataName))
    }
    this.userFields.each(function () {
      var currentKey = this.name,
        value = ((storage) ? retrievedValues[currentKey] : this.value) || defaultValues[currentKey];
      $("#" + currentKey + "Txt").text(value);
      $("#" + currentKey).val(value)
    });
    ns.bind = function (selector) {
      return new FormSave(selector)
    }
  };
  try {
    uid = new Date();
    (storage = window.localStorage).setItem(uid, uid);
    fail = storage.getItem(uid) !== uid;
    storage.removeItem(uid);
    fail && (storage = false)
  } catch (e) {}
}(this.FormSave = this.FormSave || {}, jQuery));


Initializing a controller that uses Selenium I've outsourced the setup functions from my class to another file, I outsourced initDriver from the class PortalController to the class SeleniumUtility.

Before:

class PortalController extends Controller
{
    protected $driver = NULL;

    const URL_GOOGLE = 'https://www.google.com';
    const SELENIUM_SERVER_ADDRESS = 'http://localhost:4444/wd/hub';
    const DRIVER_TIMEOUT = 5000;

    /**
    *   Initialize Driver
    * As you can see I am using `initDriver` to initialize the protected property `$driver`.
    */
    public function initDriver()
    {
        if ($this->driver === NULL) {

            $this->driver = RemoteWebDriver::create(self::SELENIUM_SERVER_ADDRESS, DesiredCapabilities::firefox(), self::DRIVER_TIMEOUT);
        }
    }

    public function main()
    {
        $this->initDriver();

        //start firefox and open google
        $this->driver->get(self::URL_GOOGLE);
    }

...

}

After:

PortalController

use App\Http\Utility\SeleniumUtility As Selenium;

class PortalController extends Controller
{
    protected $driver = NULL;

    const URL_GOOGLE = 'https://www.google.com';

    public function main()
    {
        if ($this->driver === NULL) {
            $this->driver = Selenium::initDriver(array($this, "driver"));
        }

        //start firefox and open google
        $this->driver->get(self::URL_GOOGLE);
    }
...

}

SeleniumUtility

class SeleniumUtility
{
    const SELENIUM_SERVER_ADDRESS = 'http://localhost:4444/wd/hub';
    const DRIVER_TIMEOUT = 5000;

    /**
    *   Initialize Driver
    *   @return RemoteWebDriver
    */
    public static function initDriver($thisObj)
    {
        return RemoteWebDriver::create(self::SELENIUM_SERVER_ADDRESS, DesiredCapabilities::firefox(), self::DRIVER_TIMEOUT);
    }
...
}

But I am not sure if this is the right way, it feels dirty.
I also now have to do the check if ($this->driver === NULL) { in PortalController, If i do the check in initDriver then I get:

Call to a member function get() on null on the line where I try to open the browser with google $this->driver->get(self::URL_GOOGLE);

I appreciate any feedback on how to improve this if it is possible.

I found two better alternatives.

1. Change $driver from protected to public

Then you can pass the $this object directly to initDriver:

PortalController

public $driver = null;

public function main()
{
    //Before: $this->driver = Selenium::initDriver(array($this, "driver"));

    //After:
    Selenium::initDriver($this);
    ...
}

SeleniumUtility

public static function initDriver($thisObj)
{
    //Now I can do the if null check here

    if (thisObj->driver === NULL) {
       $thisObj->driver = RemoteWebDriver::create(self::SELENIUM_SERVER_ADDRESS, DesiredCapabilities::firefox(), self::DRIVER_TIMEOUT);
    }
}

2. Use getter and setter

PortalController

protected $driver = null;

public function getDriver()
{
    return $this->driver;
}

public function setDriver($driver)
{
    $this->driver = $driver;
}

public function main()
{
    //Before: $this->driver = Selenium::initDriver(array($this, "driver"));

    //After:
    Selenium::initDriver($this);
    ...
}

SeleniumUtility

public static function initDriver($thisObj)
{
    //Now I can do the if null check here

    if (thisObj->getDriver() === NULL) {
        $thisObj->setDriver( RemoteWebDriver::create(self::SELENIUM_SERVER_ADDRESS, DesiredCapabilities::firefox(), self::DRIVER_TIMEOUT));
    }
}

But in my opinion it makes no sense to make the driver protected, so the first solution is faster and better. As long as we do not store any valuable information like passwords in the driver, keeping it public is perfectly OK.


Adventure Game (text based) in C++ I'm working on a simple text based adventure game. I've just finished working on the character creation portion. The code works perfectly fine when run, but I would just like to get some feedback to ensure that it checks off from a professional and efficient standpoint.

The values of the materials, weapons, and spells are representing the amount of damage points each does for further and future calculations with the characters stats later in the game.

The code itself essentially allows you to input a characters name. Then, the strength, stamina, and intellect is randomly generated so that your character build is different at the beginning of each game. Then, once that has finished, the code takes into account the weapon type, the material of the weapon, the spell type and adds additional damage based on the players stats.

I would just like to know if there is a better way of writing my code to make it look cleaner, or if there possibly any concerns that might cause me issues down the road when I add more functionality to the game. Seeing as I am a beginner to C++, I am not incredibly great at determining whether my code is optimal or not.

#include <iostream>
#include <string>
#include <ctime>

using namespace std;

/* Materials Structure */

typedef struct materials {
    int wood = 1, oak = 2, maple = 3, ash = 4, bronze = 2, iron = 3, steel = 4, mithril = 5, dragon = 6;
};

/* Weapon Structure */

typedef struct weapons {
    int dagger = 2, sword = 3, axe = 4, mace = 5, bow = 3, arrows = 2;
} weapons;

/* Spell Structure */

typedef struct spells {
    int fire = 4, frost = 6, dark = 8, chaos = 10;
} spells;

/* Character Structure */

typedef struct character {
    string name;
    int health = 100, mana = 100, strength, stamina, intellect, weaponAttack, spellAttack, souls = 0;
    spells spell;
    weapons weapon;
    materials material;
} character;

/* Function Declaration */

character characterCreation(string name);
void printInfo(character createChar);

/* Main Function */

int main() {
    string characterName;
    cout << "Please input character name: ";
    cin >> characterName;

    srand(time(NULL));
    character player = characterCreation(characterName);
    printInfo(player);

    system("pause");
    return 0;
}

/* Function Definition */

character characterCreation(string name) {
    character createChar;
    createChar.name = name;
    createChar.strength = rand() % 5 + 5;
    createChar.stamina = rand() % 5 + 5;
    createChar.intellect = rand() % 5 + 5;
    createChar.health += 2 * createChar.stamina;
    createChar.mana += 3 * createChar.intellect;
    createChar.weaponAttack = (createChar.weapon.dagger * createChar.material.bronze) + (2 * createChar.strength);
    createChar.spellAttack = (createChar.spell.fire + (createChar.intellect * 2));
    return createChar;
}

void printInfo(character createChar) {
    cout << createChar.name << endl;
    cout << createChar.health << endl;
    cout << createChar.mana << endl;
    cout << createChar.strength << endl;
    cout << createChar.stamina << endl;
    cout << createChar.intellect << endl;
    cout << createChar.weaponAttack << endl;
    cout << createChar.spellAttack << endl;
    cout << createChar.souls << endl;
}

typedef struct weapons {
    int dagger = 2, sword = 3, axe = 4, mace = 5, bow = 3, arrows = 2;
} weapons;

The typedef struct X { ... } X; pattern is a C-ism; in C++ you don't need the typedef and can just write struct X { ... };.

You're creating a struct type named weapons with a bunch of per-instance member variables. This is almost certainly not what you meant to do. Probably what you meant was

enum class Weapon {
    dagger = 2,
    sword = 3,
    axe = 4,
    mace = 5,
};

so that you could later write

Weapon w = Weapon::sword;
if (w == Weapon::axe) { ... }

What you actually wrote, unfortunately, is simply nonsense.

character characterCreation(string name);

Look up the C++ notion of "constructors" (and also destructors). What you have here would normally be spelled something like

Character::Character(const std::string& name) {
    this->name = name;
    this->strength = rand() % 5 + 5;
}

and so on.

Also consider writing yourself a helper function

int randint(int lo, int hi) {
    return rand() % (hi - lo) + lo;
}

so that you can write simply

    this->strength = randint(5, 10);

Ninety percent of what we call "programming" is just finding sources of repetition and eliminating them.