Displaying the number of elements larger than the average of an array I have to write a program in C that reads an array with n elements and then displays how many elements are bigger than the average of the elements of the vector.

Is there any other way to do this in a shorter or simpler way?

#include <stdio.h>
#include <stdlib.h>

#define SIZE 100

void readArray(int [] , int );
double findAverage(int [] , int);
int countAboveAverage(int [] , int , double);

int main(int argc, char* argv []) {
//declare an array of size 100
int arr[SIZE];
int n;

printf("Enter a value for n: ");
scanf("%d", &n);

if(n==0) {
fprintf(stderr, "Your size of n is too big. Run the program again.\n");
exit(1);
}
//this function will do the reading
readArray(arr, n);
//this is a function to find the average
double average = findAverage(arr, n);
//this is a function that will count the number above average
int count = countAboveAverage(arr, n, average);

printf("The number of elements above the average is %d:");

return EXIT_SUCCESS;
}

void readArray(int arr[], int n) {
int i;
for(i=0; i<n; i++) {
printf("Please enter #%d: ", i+1);
scanf("%d", arr + i); /* or &arr[i] */
}
}

double findAverage(int arr[], int n) {
int sum = 0;
int i;
for(i=0; i<n; i++)
sum+=arr[i];
return (double)sum / n;
}

int countAboveAverage(int arr[], int n, double average) {
int count=0;
int i;

for(i=0; i<n; i++)
count = arr[i] > average ? count+1: count;
return count;
}

A few extra comments:

define main at the bottom so that prototypes are unnecessary
define local functions 'static'
if you are going to use EXIT_SUCCESS then also use EXIT_FAILURE
your comments are just noise (no useful information) and should be deleted
add a \n at the end of the final printf  (which is missing count)
you don't check for n being greater than SIZE, only for zero (although
the error printf says n is too big)
define loop variables in the loop and always use braces, even when not necessary:

for (int i=0; i<n; ++i) {
    // do something
}

findAverage and countAboveAverage should declare arr as const

static double findAverage(const int arr[], int n);

personally I'd avoid using unsigned types (suggested by @Sulthan) as it often
leads to unexpected arithmetic results.  If you do use it, make sure you
enable the necessary compiler warnings.


Jquery improvement of this script - Adding and removing items from list I've been getting help with this script seen below. It allows one input box to be used to put values into a text area and also display a list of the items. The items can be deleted once inserted:

http://jsfiddle.net/ZTuDJ/50/

// If JS enabled, disable main input
$("#responsibilities").prop('disabled', true);
// $("#responsibilities").addClass("hidden");

// If JS enabled then add fields
$("#resp").append('<input placeholder="Add responsibility" id="resp_input" ></input><input type="button" value="Add" id="add"> ');

// Add items to input field
var eachline='';
$("#add").click(function(){
    var lines = $('#resp_input').val().split('\n');
    var lines2 = $('#responsibilities').val().split('\n');
    if(lines2.length>10)return false;
    for(var i = 0;i < lines.length;i++){
        if(lines[i]!='' && i+lines2.length<11){
        eachline += lines[i] + '\n';
        }
    }

    $('#responsibilities').text($("
" + eachline + "
").text()).before("
"+$("
"+lines+"
").text()+"Remove
");

    $('#resp_input').val('');
});

$(document).on('click', '.right a', function(){
    var el = $(this).closest('li')
    var index = $('li').index(el);
    var text = eachline.split('\n');
    text.splice(index, 1);
    eachline = text.join('\n')
    $('#responsibilities').text(eachline)
    el.remove()
})

I was wondering if it would be possible to tag links in the list with the following information:

Remove

By tagging the links in this way I was thinking I could easily point to them with jquery if I wanted to delete it or later edit it.

Is there room to simplify this script in this way?

What's good here:

It's fairly simple
Using prop over attr

What I did not like

Using HTML strings in code makes code very hard to modify later on. It's hard to debug and might cause potential issues (like making XSS easy for example).
No separation of concerns. You're treating your HTML like your source of knowledge instead of backing up your data with a model in the background. This is very harmful and I personally consider it a big design flaw. Your business logic (in this case the list items) and your presentational layer (the DOM nodes) is the same here. Every time you want to operate on the list you have to query the DOM. This is not only slow, but it becomes very nasty very fast as your code logic begins to expand.

There are frameworks like KnockoutJS that do data binding making this trivial to accomplish in just a few lines of code. However since you wanted a solution that has no 'magic' (Which I totally appreciate by the way) let's see what would be a simple JavaScript solution would look like.

Short note, I'm doing JavaScript with function constructors here since a lot of people find it easier, personally I'd use object initializers.

Here is a working fiddle

First, we'll have a data model for a responsibility.

//This is our element in the view model.
function Responsibility(text){
    this.text=text;
}

Next, we'll need to store all our responsibilities. We'll use an array:

// a list of our responsibilities, an actual view model to back our data
var responsibilities = [];

Now, we'll want a render function, that'll take our elements, and turn them into actual DOM objects. We'll spit it into two. First, we'll render the list item:

function renderResponsibility(rep,deleteClick){
    var el = document.createElement("li");
    var rem = document.createElement("a");
    rem.textContent = " Remove";
    rem.onclick = deleteClick;
    var cont = document.createElement("span");
    cont.textContent = rep.text;
    el.appendChild(cont);
    el.appendChild(rem);
    return el;
}

On renderResponsibility another alternative would be to use a templating engine, but you said no extra libraries so I'm keeping my word :)

Now, our general render:

//render our actual elements
function render(){
    respList.innerHTML = "";
    // note, foreach needs a modern browser but can easily be shimmed
    responsibilities.forEach(function(responsibility,i){
        var el = renderResponsibility(responsibility,function(){
            responsibilities.splice(i,1);//remove the element
            render();//re-render
        });
        respList.appendChild(el);
    });
    //update the text area;
    respTextArea.textContent = responsibilities.map(function(elem){
       return elem.text;//get the text.
    }).join("\n");
}

Finally, let's add the functionality to the Add button:

//events
addButton.onclick = function(e){
    var text = newResp.value;
    var resp = new Responsibility(text);
    responsibilities.push(resp);
    render();
    newResp.value = "";
}

And we're done :)

Just to tease:

Here is a solution in KnockoutJS
Here is a shorter solution in KnockoutJS without using an object for responsibility
Here is a solution in AngularJS

Also here is a more 'jQuery' version of the vanilla solution. As you can see a DOM manipulation library isn't very useful when building an application.


ICMP ping all hosts in a range I'm creating my own little toolbox for pinging IP interfaces (for the test, only in localhost). In the future I'm thinking of piping the output to a text file. But I'd like to focus on improving the performance for now.

For now, scanning a class A IP range is pretty fast, but for a B range (65025 addresses) it's very long. Taking in excess of 5 minutes.
Here is my code:

#!/usr/bin/python3

import threading
import os

# Define a function for the thread
def pingBox( threadName, host):
    response = os.system("ping -c 1 "+host)
    print('response for pinging %s is %s' %(threadName, response))
    if response == 0:
        print ('%s : is up' %host)
    else:
        print ('%s : is down' %host)
# Create one thread by ping
def createThread(host):
    try:
        tlist = []
        tlist.append(
            threading.Thread(
                target=pingBox,
                kwargs={
                    "threadName": "Thread:{0}".format(host),
                    "host":host
                }
        ))

        for t in tlist:
            t.start()

        for t in tlist:
            t.join()
    except Exception as e:
        print(e)

def rangeSelector(ipRange):
    if ipRange== 'A' or ipRange== 'a':
        for i in range (0,255):
            createThread('127.0.0.'+str(i))
    elif ipRange== 'B' or ipRange== 'b':
        for i in range (0,255):
            for j in range(0,255):
                createThread('127.0.'+str(i)+'.'+str(j))
    else:
        pass

if __name__ == "__main__":
    ipRange=input('Choose range A|B :')
    rangeSelector(ipRange)

Towards better design, functionality and performance

Start with good names: that means following Python naming conventions and give a meaningful names to your identifiers/functions/classes:

rangeSelector ---> ping_network_range
createThread ---> start_threads
pingBox ---> check_host

As @Peilonrayz already mentioned range(256) is the valid range for your case.

The former createThread function tried to create a single thread with start ing and join ing it at one step. But that undermines the benefit of using threading for parallelizing computations. The crucial mechanics is that all treads are initiated/started at once, then, we join them (awaiting for their finishing) at next separate phase.
On constructing a threading.Thread no need to pass custom thread name through kwargs={"threadName": ...} - the Thread constructor already accepts name argument for the thread name which is then accessed within target function via threading.current_thread().name.

The former pingBox function used os.system function (executes the command (a string) in a subshell) which is definitely not a good choice.
The more robust, powerful and flexible choice is subprocess module:

  The subprocess module allows you to spawn new processes, connect to
  their input/output/error pipes, and obtain their return codes. This
  module intends to replace several older modules and functions:
  os.system, os.spawn*
  ...
  The recommended approach to invoking
  subprocesses is to use the run() function for all use cases it can
  handle. For more advanced use cases, the underlying Popen interface
  can be used directly.

Some good, user-friendly description - on this SO link.

Furthermore, the ping command (OS network tool) can be speed up itself through adjusting specific options. The most significant ones in such context are:

-n (No attempt will be made to lookup symbolic names for host addresses. Allows to disable DNS lookup to speed up queries)
-W <number> (Time to wait for a response, in seconds. The option affects only timeout in absence of any responses, otherwise ping waits for two RTTs)
-i interval (Wait interval seconds between sending each packet. The default is to wait for one second between each packet normally, or not to wait in flood mode. Only super-user may set interval to values less than 0.2 seconds)

The difference would be more noticeable on sending more than one packet (-c option).In your case I'd apply -n and -q options. With -q option allows to quite the output since we'll get the returned code which itself indicates whether a particular host is reachable across an IP network. subprocess.run allows to access the returncode explicitly.

The 2 nested for loops within the former rangeSelector function is flexibly replaced with itertools.product routine to compose an efficient generator expression yielding formatted IP addresses:

from itertools import product
...
start_threads(f'127.0.{i}.{j}' for i, j in product(range(256), range(256)))

Designing command-line interface

As your program is planned to be used as part of pipeline, instead of hanging on interactive, blocking call of input('Choose range A|B :') - the more flexible and powerful way is using argparse module that allows building an extended and flexible command-line interfaces with variety of options of different types and actions.
For ex. the allowed network classes names can be supplied through choices:

...
parser = ArgumentParser(description='Ping network addresses by network class')
parser.add_argument('-c', '--nclass', choices=('A', 'B'), required=True, help='Choose class A or B')

The final optimized implementation:

import threading
from argparse import ArgumentParser
from itertools import product
import subprocess

def check_host(host: str):
    return_code = subprocess.run(["ping", "-c", "1", "-n", "-q", host],
                                 stdout=subprocess.DEVNULL,
                                 stderr=subprocess.DEVNULL).returncode
    print(f'response for pinging {threading.current_thread().name} is {return_code}')

    status = 'up' if return_code == 0 else 'down'
    print(f'{host} : is {status}')

def start_threads(addr_range):
    for addr in addr_range:
        t = threading.Thread(target=check_host, args=(addr,),
                             name=f'Thread:{addr}')
        t.start()
        yield t

def ping_network_range(net_class: str):
    net_class = net_class.upper()
    if net_class == 'A':
        threads = list(start_threads(f'127.0.0.{i}' for i in range(256)))
    elif net_class == 'B':
        threads = list(start_threads(f'127.0.{i}.{j}'
                                     for i, j in product(range(256), range(256))))
    else:
        raise ValueError(f'Wrong network class name {net_class}')

    for t in threads:
        t.join()

if __name__ == "__main__":
    parser = ArgumentParser(description='Ping network addresses by network class')
    parser.add_argument('-c', '--nclass', choices=('A', 'B'),
                        required=True, help='Choose class A or B')
    args = parser.parse_args()
    ping_network_range(args.nclass)

Sample usage (with timing, under Unix time command):

time python3 ping_network_range.py -c B > test.txt

real    2m4,165s
user    2m17,660s
sys     4m35,790s

Sample tail contents of resulting test.txt file:

$ tail test.txt
response for pinging Thread:127.0.255.250 is 0
127.0.255.250 : is up
response for pinging Thread:127.0.255.252 is 0
127.0.255.252 : is up
response for pinging Thread:127.0.255.253 is 0
127.0.255.253 : is up
response for pinging Thread:127.0.255.255 is 0
127.0.255.255 : is up
response for pinging Thread:127.0.255.254 is 0
127.0.255.254 : is up


Determine whether three sides form a valid triangle, and classify the triangle This is a my first Python program, and whilst I am new to Python I would like to keep in good practice

Below is a short program to work out what type a triangle is and if it makes a valid triangle

I have tried to use as little documention for this as possible to try and get used to how things work in Python so I can only imagine the mistakes I have made. However please do mention anything that should have be done better

# Determin if triangle is Scalene. Isosceles or equilateral
# Also works out if lengths can make a triangle

from decimal import *

getcontext().prec = 3
getcontext().rounding = ROUND_HALF_UP

#Needs to be divided to re-set decimal place I think
a = Decimal(input("Length of side a = ")) / 1
b = Decimal(input("Length of side b = ")) / 1
c = Decimal(input("Length of side c = ")) / 1

if a != b and b != c and a != c:
    print("This is a a Scalene triangle")
    triangle_type = 'Scalene'
elif a == b and c == b:
    print("This is an Equilateral triangle")
    triangle_type = 'Equilateral'
else:
    print("This is an Isosceles triangle")
    triangle_type = 'Isosceles'

def is_valid_triangle(a, b, c,triangle_type):
    if triangle_type == 'Equilateral':
        return True #all same lengths will be a valid triangle
    elif triangle_type == 'Isosceles' or triangle_type == 'Scalene':
        if a == b:
            return a + b > c
        elif b == c:
            return b + c > a
        elif a == c:
            return a + c > b
        else: #This will be the scalene triangle
            return a + b > c
    else:
        return False #Message is unclear as could be lengths are negitive or correct int type not used

print('Is this a valid triangle?', is_valid_triangle(a,b,c,triangle_type))

Add a __name__ == "__main__" guard, and move the logic into a function separate from the I/O:
def triangle_type(a, b, c):
    '''Return a string indicating the type of triangle
       (Equilateral, Isosceles, Scalene, Impossible)
    '''
    # implementation here...

def main():
    getcontext().prec = 3
    getcontext().rounding = ROUND_HALF_UP

    #Needs to be divided to re-set decimal place I think
    a = Decimal(input("Length of side a = ")) / 1
    b = Decimal(input("Length of side b = ")) / 1
    c = Decimal(input("Length of side c = ")) / 1

    print(f"This is a {triangle_type(a, b, c)} triangle")

if __name__ == "__main__":
    main()

In the implementation, we can save a lot of "or" tests by sorting the lengths before we start:
a, b, c = sorted([a, b, c])

if a + b <= c:
    # N.B. automatically catches a < 0, since b <= c
    return 'Impossible'
if a != b != c:
    return 'Scalene'
elif a == c:
    return 'Equilateral'
else:
    return 'Isosceles'

Modified code
def triangle_type(a, b, c):
    '''
    Return a string indicating the type of triangle
    (Equilateral, Isosceles, Scalene, Impossible)
    '''

    a, b, c = sorted([a, b, c])

    if a + b <= c:
        return 'Impossible'
    if a != b != c:
        return 'Scalene'
    if a == c:
        return 'Equilateral'
    return 'Isosceles'

def main():
    a = input("Length of side a: ")
    b = input("Length of side b: ")
    c = input("Length of side c: ")
    print(f"({a}, {b}, {c}) is a {triangle_type(a, b, c)} triangle")

if __name__ == "__main__":
    main()

Further improvement
Use the doctest module to write the tests:
def triangle_type(a, b, c):
    '''
    Return a string indicating the type of triangle
    (Equilateral, Isosceles, Scalene, Impossible)

    >>> triangle_type(1, 1, 2)
    'Impossible'
    >>> triangle_type(-1, -1, -1)
    'Impossible'
    >>> triangle_type(1, 1.0, 1)
    'Equilateral'
    >>> triangle_type(1, 2, 2)
    'Isosceles'
    >>> triangle_type(2, 3, 2)
    'Isosceles'
    >>> triangle_type(2, 3, 4)
    'Scalene'
    '''

    a, b, c = sorted([a, b, c])

    if a + b <= c:
        return 'Impossible'
    if a != b != c:
        return 'Scalene'
    if a == c:
        return 'Equilateral'
    return 'Isosceles'

if __name__ == "__main__":
    import doctest
    doctest.testmod()


Searching Combobox drop-down list I've created a Combobox GUI that allows the user to search through the Combobox by entering a letter on the keyboard. If user enter letter 'L' it will search through the Combobox drop-down list for the first occurrence of a word that starts with letter 'L'.

Please let me know if I could have made my findInBox method better. Do you know of a way to search through the drop-down list while it is open?

import tkinter
tk = tkinter

from tkinter import StringVar, Label, Button
from tkinter import font
from tkinter.ttk import Combobox

class Parking_Gui(tk.Frame):

    def __init__(self):
        """Sets up the window and widgets"""
        tk.Frame.__init__(self)

        self.myfont = font.Font(family="Calibri", size=11, weight="normal")

        self.master.geometry("315x125")
        self.master.title("MSU PARKING APP")
        self.master.rowconfigure(0, weight = 1)
        self.master.columnconfigure(0, weight = 1)
        self.grid(sticky = 'NW')

        # Label for the parking lots
        self.LotLabel = tk.Label(self, text = "MSU Parking",  font=self.myfont)
        self.LotLabel.grid(row = 0, column = 0)

        # Combo Box for parking lots
        self._ComboValue = tk.StringVar()
        self._LotCombo = Combobox(self, textvariable=self._ComboValue,
                                  state='readonly', height = '6',
                                  justify = 'center', font=self.myfont)

        # List of parking lots
        self._LotCombo['values']=('ARTX', 'BURG', 'CLAY_HALL', 'GLAB',
                                  'HAMH_HUTC', 'HHPA', 'JVIC', 'LIBR','PLSU',
                                  'POST',  'PROF', 'STEC', 'STRO_NORTH',
                                  'STRO_WEST', 'TROP')
        self._LotCombo.grid(row = 0, column = 1)

        # Button to open parking diagram
        self._button = tk.Button(self, text = "Open", font=self.myfont)
        self._button.bind('<Button-1>', self._runParkingLot)
        self._button.grid(row = 0, column = 2)

        # Press enter to open selected parking diagram
        self._LotCombo.bind("<Return>", self._runParkingLot)

        # Search Combobox with keyboard
        self._LotCombo.bind("<Key>", self.findInBox)

        self._LotCombo.focus()

    def _runParkingLot(self, event):
        """Event handler for the button. Will run the
           functinon associated with selected item"""

        parkingLot = self._LotCombo.get()

        """The 'globals' keyword, will return a dictionary of every function,
           variable, etc. currently defined within the global scope."""
        if parkingLot in globals():
            func = globals()[parkingLot]
            func()

    def findInBox(self, event):
        """findInBox method allows user to search through Combobox values
           by keyboard press"""

        alpha = ('A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J',
                 'K', 'L', 'M', 'N', 'O', 'P', 'Q', 'R', 'S', 'T',
                 'U', 'V', 'W', 'X', 'Y', 'Z')

        lot = ('ARTX', 'BURG', 'CLAY_HALL', 'GLAB', 'HAMH_HUTC', 'HHPA',
               'JVIC' ,'LIBR', 'PLSU', 'POST', 'PROF', 'STEC',
               'STRO_NORTH', 'STRO_WEST', 'TROP')

        keypress = event.char
        keypress = keypress.upper()

        # If key pressed is a letter in alphabet
        if keypress in alpha:

             # if user press 'A' on keyboard return first item in list
             if(keypress == lot[0][0]):
                    self._LotCombo.current(0)

             # Searches list for first occurrence of key entered by user
             else:
                 count = 1
                 while(keypress != lot[count][0] and count < len(lot)-1 ):
                     count +=1

                 self._LotCombo.current(count)
                 count = 1

def main():
    Parking_Gui().mainloop

main()

Imports

There is a more idiomatic way of doing

import tkinter
tk = tkinter

it is import tkinter as tk.

You’re also importing StringVar, Label and Button from tkinter but still calling them using the tk namespace. Either remove the import line or remove the tk. part when creating such objects.

Same for font, you import it from tkinter and use it only for font.Font. Either do from tkinter.font import Font and call it using only Font or call it with tk.font.Font. All in all, it’s more a matter of consistency than saving on typing.

ComboBox management

First of, you should define your values as a constant somewhere. Either at the top-level of the file or as a class attribute. That way you could re-use it in both __init__ and findInBox without having to define it twice.

Secondly, your alpha is pretty much string.ascii_uppercase. No need to redefine it again.

Lastly, you’d be better of using a combination of for and enumerate to iterate over your different values. Both because a for loop makes the intent of iteration clearer, and the index management is simpler using enumerate:

import tkinter as tk

from tk.font import Font
from tk.ttk import Combobox

from string import ascii_uppercase

class Parking_Gui(tk.Frame):

    PARKING_LOTS = ('ARTX', 'BURG', 'CLAY_HALL', 'GLAB', 'HAMH_HUTC',
                    'HHPA', 'JVIC', 'LIBR', 'PLSU', 'POST', 'PROF',
                    'STEC', 'STRO_NORTH', 'STRO_WEST', 'TROP')

    def __init__(self):
        ...
        self._LotCombo['values'] = Parking_Gui.PARKING_LOTS
        ...

    ...
    def findInBox(self, event):
        keypress = event.char.upper()

        if keypress in ascii_uppercase:
            for index, lot_name in enumerate(Parking_Gui.PARKING_LOTS):
                if lot_name[0] >= keypress:
                    self._LotCombo.current(index)
                    break

PEP8 & PEP257

Small improvements in readability but:

class names should be TitleCase not Title_Snake_Case
functions and variable names should be snake_case and not TitleCase or camelCase. (And constants like PARKING_LOTS in UPPER_SNAKE_CASE.)
new lines in docstring should be indented at the same level than the start of the """, not their end. Multi-line docstrings should also have the closing """ on their own line.


Insert dynamic parameters to sqlite database statements I want to insert parameters dynamically to statements at sqlite.
I now write as follows:

            tx.executeSql('SELECT Data from Table Where something = "'+ anyvariable+ '"',[],successFn, errorCB);

But I guess there is a better (cleaner) method to do it.. Any ideas?

I would create an object to model my dynamic parameters using the following interfaces:

{ //reduced format
    clause: string,
    params: an array of parameters if more than one is used
}
{ //simple entry format
    clause: string
    param: the parameter if only one is used
}
{ //or format
    or: an array of dynamic parameter interfaces to be joined as part of an or clause
}
{ //and format
    and: an array of dynamic parameter interfaces to be joined as part of an and clause
}

From here I could use the following functions:

function reduce(param) {
    var clause = [], params = [], nest, nestType;
    if (param.clause !== undefined && param.params) {
        //already in reduced format
        return param;
    }
    if (param.clause !== undefined && param.param !== undefined) {
        //convert simple format to reduced
        return { clause: param.clause, params: [param.param] };
    }
    if (param.clause) {
        //special case (clause without additional parameter)
        //parameters without clause would be done by using empty string
        return { clause: param.clause, params: [] };
    }
    //convert nested forms (and and or)
    if (param.and) {
        nest = param.and;
        nestType = ' AND ';
    } else if (param.or) {
        nest = param.or;
        nestType = ' OR ';
    } else {
        throw new Error('Invalid dynamic parameter found');
    }
    nest.forEach(function (p) {
        p = reduce(p);
        clause.push(p.clause);
        params.push.apply(params, p.params);
    });
    return {
        clause: '(' + clause.join(nestType) + ')',
        params: params
    };
}

function executeDynamicSql(tx, base, dynamicparameter, onSuccess, onError) {
    var reduction = reduce(dynamicparameter);
    tx.executeSql(base + reduction.clause, reduction.params, onSuccess, onError);
}

And call it (in your test case) like so:

executeDynamicSql(tx, 'SELECT Data from Table Where ', {
    clause: 'something = ?',
    param: anyvariable
}, successFn, errorCB);

What is nice about this function is that it can handle a far more complex where clause such as:

var dynamicwhere = {
        or: [
            {clause: 'something = ?', param: somethingid},
            {clause: 'somethingelse = ?', param: somethingelse},
            {clause: 'somedate between ? and ?', params: [d1, d2]},
            {
                and: [
                    {clause: 'amt > ?', param: min},
                    {clause: 'amt < ?', param: max},
                ]
            }
        ]
    };

executeDynamicSql(tx, 'SELECT Data from Table Where ', dynamicwhere, successFn, errorCB);

Ultimately executing:

SELECT Data from Table Where (something = ?
                           or somethingelse = ?
                           or somedate between ? and ?
                           or (amt > ? and amt < ?))

With the in order parameter list:

[somethingid, somethingelse, d1, d2, min, max]

You could do something more fancy using a regex replace or whatever, but I don't think it should be necessary.


Simplify jQuery animation with callback Looking for feedback on how to better improve the code below to simplify the code below.

var animateTop = $('body');
if ($('html').hasClass('lt-ie9')) {
    animateTop = $('html');
}
$('.var').on('click', function() {
    animateTop.animate({
        scrollTop: $('footer').offset().top
    }, {
    queue: false,
    duration: 1500,
    complete: function() {
        if(!$('.foo').hasClass('active')) {
            $('.foo').toggleClass('active');
            $('.bar').slideToggle();
        }
    }
});
return false;
});

Can't really tell what your code does in real life, thus the following optimization/organization might not work. However, following generic tips, here's what I got:

//we can reduce DOM fetching by fetching html first
//caching it as well as other frequently used elements
//assuming that they never change in the course of your page's life
var html = $('html')
  , footer = $('footer')
  , foo = $('.foo')
  , bar = $('.bar')
  , options = {
      queue: false,
      duration: 1500,
      complete: function () {
        if (!foo.hasClass('active')) {
          foo.toggleClass('active');
          bar.slideToggle();
        }
      }
    }
  ;

//then here, we fetch body only when it's not IE9+ thus reducing operations
html = html.hasClass('lt-ie9') ? html : $('body');

$('.var').on('click', function (e) {

  //i think the properties can't be cached since it may need current values
  //however, your options can be cached since they are static values
  html.animate({
    scrollTop: footer.offset().top
  }, options);

  //an answer as to when to use return false vs preventDefault
  //you can use either, depending on the situation
  //http://stackoverflow.com/a/1357151/575527
  return false;
});


Simple HTTP server that performs a regex test I'm sure that my server can work with better performance, but it's just a dream.

Parsing a very long strings is not so obvious task for me.
I have a server that handles a lot of clients requests and finds a substring of a string.

Server

package api

import (
    "net/http"
    "stackexchange/api/controllers"
)

func start() {
    mux := http.NewServeMux()
    mux.HandleFunc("/messages", Messages)
}

func Messages(res http.ResponseWriter, req *http.Request) {
    mc := controllers.NewMessageController()
    switch req.Method {
    case "POST":
        mc.ActionPost(res, req)
    }
}

Controller

package controllers

import (
    "net/http"
    "regexp"
)

type MessageController struct {
}

func NewMessageController() *MessageController {
    return &MessageController{}
}

func (mc *MessageController) ActionPost(res http.ResponseWriter, req *http.Request) {

    msgBody := req.URL.Query().Get("body")
    msgPattern := req.URL.Query().Get("pattern")

    isPatternFind := regexp.MatchString(msgPattern, msgBody)

    if isPatternFind {
        res.Write([]byte("find"))
    } else {
        res.Write([]byte("not find"))
    }
}

Test

package tests

import (
    "fmt"
    "log"
    "net/http"
    "net/http/httptest"
    "stackexchange/api"
    "testing"
)

var longString = `Knowing that millions of people around the world would be
                  watching in person and on television and expecting great
                  things from him — at least one more gold medal for America,
                  if not another world record — during this, his fourth and
                  surely his last appearance in the World Olympics, and realizing
                  that his legs could no longer carry him down the runway with
                  the same blazing speed and confidence in making a huge,
                  eye-popping leap that they were capable of a few years ago
                  when he set world records in the 100-meter dash and in the
                  400-meter relay and won a silver medal in the long jump,
                  the renowned sprinter and track-and-field personality
                  Carl Lewis, who had known pressure from fans and media before
                  but never, even as a professional runner, this kind of pressure,
                  made only a few appearances in races during the few months before
                  the Summer Olympics in Atlanta, Georgia, partly because he was
                  afraid of raising expectations even higher and he did not want
                  to be distracted by interviews and adoring fans who would follow
                  him into stores and restaurants demanding autographs and
                  photo-opportunities, but mostly because he wanted to conserve
                  his energies and concentrate, like a martial arts expert, on
                  the job at hand: winning his favorite competition, the long jump,
                  and bringing home another Gold Medal for the United States,
                  the most fitting conclusion to his brilliant career in track and field.`

var subString = `home`

func TestPostMessage(t *testing.T) {
    url := "http://localhost:8080/messages?"
    url = fmt.Sprintf("%sbody=%s", url, longString)
    url = fmt.Sprintf("%s&pattern=%s", url, subString)

    req, err := http.NewRequest("POST", url, nil)

    if err != nil {
        log.Fatal(err)
    }

    res := httptest.NewRecorder()
    api.Messages(res, req)

    expected := "find"
    actual := res.Body.String()

    if actual != expected {
        log.Fatalf("\nMethod: POST\nActual: %s \nExpected: %s \n", actual, expected)
    }
}

The call to regexp.MatchString is the only really expensive part of your code. Do you need full regexp support? If you just want to find substrings, you should use strings.Contains instead of the regexp package. Otherwise, there's no way around it. It might be worth looking at what each request looks like: you could call strings.Contains for the simple ones (that don't use any regexp features) and only use regexp.MatchString when needed.

Some comments on style:

Your NewMessageController method is useless. Just have your callers use new(controllers.MessageController).
Use t.Fatalf instead of log.Fatalf in your tests.
Build strings in a consistent manner:

var (
    longString = "your long string"
    subString = "home"
    host = "http://localhost:8080"
)
func TestPostMessage(t *testing.T) {
    url := fmt.Sprintf("%s/messages?body=%s&pattern=%s", host, longString, subString)
    …
}


Remove the parent element when user clicks on the remove icon

X
form 0

X
form 1

X
form 2

X
form 3

I would like to hide the parent div, when it clicks on the remove element.

   $(".remove-form-0").on("click", function(){
          $("#form-0").css("display", "none");
  });

I do not want to duplicate this for 4 times. What is the best solution for this ?

Any helps would be highly appreciated.

In general classes are used for collections of things that should exhibit the same behaviour. Your buttons here are perfect candidates for this, so you should give them the same class name - remove-form-btn

Then give your divs a class name of form and you can write just one handler:

$('.remove-form-btn').click((event) => {
  $(event.target).parents('.form').hide();
});


Exporting HTML table data to Excel using C# I have JSON data that I bind to a table:

Now I would like to export this data to Excel, so I have written this code that involves data formatting of an Excel spreadsheet.

private static MemoryStream JsonToStream<T>(string data, string html, string movieReportName)
{
    IEnumerable<T> serializedData = data.JsonDeserialize<T>().AsEnumerable<T>();
    List<SelectListItem> colOrder = ParseColumnsToList(html);
    DataTable dataTable = serializedData.ToDataTable<T>(colOrder);
    ExcelPackage epackage = new ExcelPackage();
    ExcelWorksheet excel = epackage.Workbook.Worksheets.Add(movieReportName);
    excel.Cells["A1"].LoadFromDataTable(dataTable, true);

    int rowCount = dataTable.Rows.Count;
    int columnCount = dataTable.Columns.Count;
    for (int i = 2; i < rowCount + 2; i++)
    {
        for (int j = 1; j <= columnCount; j++)
        {
           if (value.Contains("%"))
            {
                if ((dataTable.Columns[j - 1].ColumnName != "Movie Code") {

                    value = value.Replace("%", "");
                    if (value.Contains("."))
                    {
                        double number = 0.0;
                        double.TryParse(value, out number);
                        excel.Cells[i, j].Value = number / 100;
                        excel.Cells[i, j].Style.Numberformat.Format = "0.00##%";
                    }
                }
            }
         }
      }
      MemoryStream ms = new MemoryStream(epackage.GetAsByteArray());
      epackage.Dispose();
      return ms;
  }

This works fine but it runs very slowly. Do you think I have to move data formatting to a database?

Update
What are different better option to export large data from HTML to excel?

First off let's encapsulate what you are trying to do here.

Create a function that will format the DataTable.

private static DataTable FormatMovieReportTable(DataTable reportTable)
{
    foreach(DataColumn column in reportTable.Columns)
    {
        string colName = column.ColumnName;
        if(colName != "Movie Code")
        {
            foreach(DataRow row in reportTable.Rows)
            {
                string value = row[colName]
                if(value.Contains("%"))
                {
                    value = value.Replace("%", "");
                    if(value.Contains("."))
                    {
                        double number = 0.0;
                        double.TryParse(value, out number);
                        value = (number / 100).ToString("0.00##")
                    }
                    row[colName] = value;
                }
            }
        }
    }
    return reportTable;
}

Now create a function that will create the DataTable from the string values.

private DataTable JsonToTable<T>(string data, string html, string tableName)
{
    DataTable output = null;
    IEnumerable<T> serializedData = data.JsonDeserialize<T>().AsEnumerable<T>();
    List<SelectListItem> colOrder = ParseColumnsToList(html);
    output = serializedData.ToDataTable<T>(colOrder);
    if(output!= null)
    {
        output = FormatMovieReportTable(output);
        output.TableName = tableName;
    }
    return output
}

And now the excel output..

private void TableToExcel(DataTable table)
{
    ExcelPackage epackage = new ExcelPackage();
    // the ?? is just in case that the table name is null
    // "new sheet" is just some arbitrary default name
    ExcelWorksheet excel = epackage.Workbook.Worksheets.Add(table.TableName ?? "new sheet");
    excel.Cells["A1"].LoadFromDataTable(table, true);
}

From wherever you called the posted function you can separate the logic

private void ThisIsTheCallerMethod()
{
    string data, html, reportName;

    // Other things happen and variables are populated

    DataTable reportTable = JsonToTable<SomeType>(data, html, reportName);
    if(reportTable != null)
    {
        TableToExcel(reportTable);
    }

    // Create the MemoryStream, etc...
}


WGS84-coordinate encoded as Integers I want to store WGS84-coordinates in two integers for latitude and longitude instead of floats to get a higher precision than with floats.

Here's my code:

struct Coordinate
{
    private int latitude;
    private int longitude;

    const double latMin = -90.0f;
    const double latMax = 90.0f;
    const double lngMin = -180.0f;
    const double lngMax = 180.0f;

    public Coordinate(double latitude, double longitude)
    {
        this.latitude = mapDoubleToInt(latitude, latMin, latMax);
        this.longitude = mapDoubleToInt(longitude, lngMin, lngMax);
    }

    public double Latitude { get { return mapIntToDouble(latitude, latMin, latMax); } }
    public double Longitude { get { return mapIntToDouble(longitude, lngMin, lngMax); } }

    private static int mapDoubleToInt(double value, double min, double max)
    {
        return (int)(((long)int.MaxValue - int.MinValue) * (value - min) / (max - min) + int.MinValue);
    }

    private static double mapIntToDouble(int value, double min, double max)
    {
        return ((double)value - int.MinValue) / ((long)int.MaxValue - int.MinValue) * (max - min) + min;
    }
}

I'm not sure if there is a better to convert the data. There are a lot of casts and arithmetic operations in it -- any way to do this more elegantly?

you can avoid stuffing everything in a one-liner:

private static int mapDoubleToInt(double value, double min, double max)
{
    long intRange = 1L << 32; // ==((long)int.MaxValue - int.MinValue)
    double inputRange = max - min;
    return (int)(intRange * (value - min) / inputRange + int.MinValue);
}

private static double mapIntToDouble(int value, double min, double max)
{
    long intRange = 1L << 32; // ==((long)int.MaxValue - int.MinValue)
    double inputRange = max - min;
    return inputRange * ((double)value - int.MinValue) / intRange + min;
}

After reordering the operations it's now clearer that they are opposites. Especially after extracting the range as variables.


Returning the length of the File in B/KB/MB The method length() in File returns the length of a file in Bytes, this code transforms that into b/kb/mb as can be seen in the Windows Explorer

I ended up with the code below, let me know if it can be optimized or the logic can be improved.

public static String getFileSize(File file) {
    String modifiedFileSize = null;
    double fileSize = 0.0;
    if (file.isFile()) {
        fileSize = (double) file.length();//in Bytes

        if (fileSize < 1024) {
            modifiedFileSize = String.valueOf(fileSize).concat("B");
        } else if (fileSize > 1024 && fileSize < (1024 * 1024)) {
            modifiedFileSize = String.valueOf(Math.round((fileSize / 1024 * 100.0)) / 100.0).concat("KB");
        } else {
            modifiedFileSize = String.valueOf(Math.round((fileSize / (1024 * 1204) * 100.0)) / 100.0).concat("MB");
        }
    } else {
        modifiedFileSize = "Unknown";
    }

    return modifiedFileSize;
}

Logic

You can simplify your logic by testing for the biggest known extension, for example

if (fileSize > 1024 * 1024)
    doMiB();
} else if (fileSize > 1024) {
    doKiB();
} else {
    doByte();
}

Rounding

Don't use Math.round(x / 100.0) * 100. for rounding to two decimal places use the appropriate library function

Argument checking

Don't use default values ("Unknown") for invalid input. If the input to your getFileSize method is not a file, throw an exception

if (!file.isFile()) {
    throw new IllegalArgumentException("Expected argument to be a file");
}

Fast return

This is more personal taste but I prefer to return as soon as possible. If you know the result will be "100.31 KiB" why save it into a local variable instead of returning it immediately?

Conclusion

I would improve the code as followed

private static final DecimalFormat format = new DecimalFormat("#.##");
private static final long MiB = 1024 * 1024;
private static final long KiB = 1024;

public String getFileSize(File file) {

    if (!file.isFile()) {
        throw new IllegalArgumentException("Expected a file");
    }
    final double length = file.length();

    if (length > MiB) {
        return format.format(length / MiB) + " MiB";
    }
    if (length > KiB) {
        return format.format(length / KiB) + " KiB";
    }
    return format.format(length) + " B";
}


Reverse the order of a list in Racket I am using the SICP book. There is an exercise in which you need to create a function that will receive a list as an argument and return a list with the same elements in a reverse order.

I know there are more efficient solutions than the one I am going to present. However, I really learn by analyzing things that came up naturally from my mind.

I would like to improve my suboptimal solution in its own nature.

First, I created my own list-ref (I like building built-in functions as a matter of practice):

(define (list-ref lista n)
  (if (= n 1)
      (car lista)
      (list-ref (cdr lista) (- n 1))))

Second, I created my own function to count the length of a list:

(define (list-len lista)
  (if (null? lista)
      0
      (add1 (list-len (cdr lista)))))

After that, I created the core of the reverse process:

(define (reverse-exe lista n)
  (if (= n 0)
      empty
      (cons (list-ref lista n) (reverse-exe lista (- n 1)))))

On the code above, the parameter n needs to be the length of the list (considering the first element as 1, and not zero).

As the question asks us to build a function that will receive only a list as an input, I created another function which turns "n" as the length:

(define (reverse lista)
  (reverse-exe lista (list-len lista)))

Is there a better way to organize these procedures?

Your solution has a main problem: you treat a list almost like an array, by using list-ref to get the n-th element of the list, to rebuild the list by consing all the elements from the last to the first.

This is a problem for two reasons:

it makes your procedure of order of complexity n², instead of n, as we can obtain with other algorithms, since the list-ref has a cost of the order of n, the length of the list, and you call it a number of times which is equal to n;
if you use lists instead of arrays, you should try to use as much as possible the primitive operators on them, like car, cdr, cons, that can be considered as more “natural” for this kind of data structure, having a constant cost, and resort to operators like len and list-ref only when strictly necessary (otherwise you can use directly arrays, also availables in lisp languages).

So the question is: how can we implement an efficient list reverse operator by using the primitive operators on lists?

A solution can be found if we generalize a little bit the reverse function, and instead of defining a function that reverses a list, we define a function that takes two lists as parameters, and build a new list by reversing the elements of the first one consing them on the second. This can be done in this way:

(define reverse1
  (lambda (first-list second-list)
    (if (null? first-list)
        second-list
        (reverse1 (cdr first-list) (cons (car first-list) second-list)))))

The efficiency is given by the fact that we are using only the operators cons, car and cdr, and we scan only the first list, and only once, by adding each element to the second list.

And to obtain the reverse function, we need simply to call the previous function with a second list as the empty list, as this will produce as result the reverse of the first list:

(define reverse
  (lambda (ls)
    (reverse1 ls '())))

Actually, this technique is more commonly note as resorting to an accumulator to produce a result, and a solution like the above is more usually written in a single function in Racket:

(define reverse
  (lambda (ls [accumulator '()])
    (if (null? ls)
        accumulator
        (reverse (cdr ls) (cons (car ls) accumulator)))))

Here we use the possibility of defining a default value for a parameter, so that the first time we call the function with the list to be reversed, and the second parameter is automatically assigned the empty list, while in the recursive call it is passed explicitly by building the (partial) result.

A final important note: the recursion in this function is called “tail recursion”, since the last operation of the function is just a call to the function itself, and the result of this call is the result of the function. In Scheme, and Racket, as well as in other similar languages with good compilers, this kind of recursion is compiled as iteration, making the function more efficient in terms of space used.


Custom System.Web.Http.AuthorizeAttribute I've hacked together what feels like a mess for an authorize attribute to secure web api methods. The code appears to be functionally correct; passing the unit tests in place. The code has me worried because it will be executing with every call to any web api method and there is a lot going on, including string allocations and database access. I'm sure that a lot of my misgivings are due to premature optimization.

I am looking for a general review of the code; looking for obvious problems, possible performance bottlenecks etc.

The use of this attribute provides for three different scenarios. First, specifying the attribute with no users or roles means that the attribute should simply validate that the supplied user/password authenticate properly. Second, the attribute supports a comma separated list of usernames... so in addition to authenticating the user, it should also verify the user is in the permitted list. Finally, the attribute allows a comma separated list to denote allowed roles... this requires the user to authenticate and to be a member of one of the provided roles. The semantics are such with regard to user or role lists, passing either condition in additional to authenticating successfully should return a true result.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading;
using System.Web;
using System.Web.Http;
using System.Web.Http.Controllers;
using System.Web.Security;

using WebMatrix.WebData;

using Appsomniacs.Web.Filters;

namespace Appsomniacs.Web.Handlers
{
    public class BasicAuthorizeAttribute : AuthorizeAttribute
    {
        protected override bool IsAuthorized(HttpActionContext actionContext)
        {
            var identity = Thread.CurrentPrincipal.Identity;

            if (identity == null &&
                HttpContext.Current != null)
            {
                identity = HttpContext.Current.User.Identity;
            }

            if (identity != null &&
                identity.IsAuthenticated)
            {
                var basicAuth = identity as BasicAuthenticationIdentity;

                if (basicAuth != null)
                {
                    if (WebSecurity.Login(basicAuth.Name, basicAuth.Password))
                    {
                        if (this.Users.Length > 0)
                        {
                            var allowedUsers = this.Users.ToUpperInvariant()
                                .Split(',');

                            if (allowedUsers.Contains(basicAuth.Name.ToUpperInvariant()))
                            {
                                return true;
                            }
                        }

                        if (this.Roles.Length > 0)
                        {
                            var allowedRoles = Roles.Split(',');

                            var userRoles = System.Web.Security.Roles.GetRolesForUser(basicAuth.Name);

                            var matches = userRoles.Intersect(allowedRoles).ToArray();

                            if (matches.Length > 0)
                            {
                                return true;
                            }
                        }
                        else
                        {
                            return true;
                        }
                    }
                }
            }

            return false;
        }
    }
}

How about something like this.  It's pretty much what you have but all I have done is seperated a couple of the parts into methods

using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading;
using System.Web;
using System.Web.Http;
using System.Web.Http.Controllers;
using System.Web.Security;
using WebMatrix.WebData;

using Appsomniacs.Web.Filters;

namespace Appsomniacs.Web.Handlers
{
    public class BasicAuthorizeAttribute : AuthorizeAttribute
    {
        protected override bool IsAuthorized(HttpActionContext actionContext)
        {
            var identity = Thread.CurrentPrincipal.Identity;

            if (identity == null && HttpContext.Current != null)
            {
                identity = HttpContext.Current.User.Identity;
            }

            if (identity != null && identity.IsAuthenticated)
            {
                var basicAuth = identity as BasicAuthenticationIdentity;

                if (basicAuth != null)
                {
                    if (WebSecurity.Login(basicAuth.Name, basicAuth.Password))
                    {
                        return IsAllowedUser(basicAuth.Name) || IsAutenticatedInRole(basicAuth.Name);
                    }
                }
            }

            return false;
        }

        private bool IsAllowedUser(string name)
        {
            if(this.Users.Length == 0) return false;

            var allowedUsers = this.Users.ToUpperInvariant()
                .Split(',');

            return allowedUsers.Contains(basicAuth.Name.ToUpperInvariant());
        }

        private bool IsAutenticatedInRole(string name)
        {
            if (this.Roles.Length > 0)
            {
                var userRoles = System.Web.Security.Roles.GetRolesForUser(basicAuth.Name);

                var allowedRoles = Roles.Split(',');
                var matches = userRoles.Intersect(allowedRoles).ToArray();

                return matches.Length > 0;
            }

            return true;
        }
    }
}


First Schema for Location and Countries This is my first MYSQL schema for caching location data (co-ordinates to place names) and referencing it with a country.

I was wondering what everyones feedback was on it, did I do a good job?

CREATE TABLE location (
id INT AUTO_INCREMENT PRIMARY KEY,
locality VARCHAR(20),
administrative_area_level_1 VARCHAR(20),
administrative_area_level_2 VARCHAR(20),
administrative_area_level_3 VARCHAR(20),
loc VARCHAR (17) NOT NULL,
rad VARCHAR (17),
updated TIMESTAMP ON UPDATE CURRENT_TIMESTAMP
);

CREATE TABLE country (
id INT AUTO_INCREMENT PRIMARY KEY,
name VARCHAR(20),
iso VARCHAR(20),
loc VARCHAR (17) NOT NULL,
rad VARCHAR (17),
updated TIMESTAMP ON UPDATE CURRENT_TIMESTAMP
);

For all it's worth, and this is pretty old but for the sake of clearing unanswered reviews, here is my take:

id INT AUTO_INCREMENT PRIMARY KEY,

I find that being more specific in column names makes the code easier to work with, especially when you are using JOIN and table aliases. Suppose the following:

SELECT l.*, c.*
FROM location AS l
LEFT JOIN country AS c
ON l.loc = c.loc

The result set would have column headers as l.id and c.id unless you took the time to make column aliases for everything. If your columns were instead named location_id and country_id the result set would be much easier to read to someone who is not a database programmer.
name VARCHAR(20)

I think 20 may be a bit short for country name. For instance:

SELECT CHAR_LENGTH('United States of America');

Result: 24

Other than that I think it looks fine.


Switch statement decreases adaptability of a function Below you can see a class and a function. The class is just an dummy entity. The functon has a switch statement. That switch statement calculates the tax by to a predefined hardcoded string, and I don't like that. What's next, this switch statement violates the Open/Closed principle.
The class entity:
class Product {
    private int $id;
    private string $name;
    private int $price;
    private string $category;

    public function __construct(int $id, string $name, int $price, string $category) {
        $this->id = $id;
        $this->name = $name;
        $this->price = $price;
        $this->category = $category;
    }
    public function getId(): int
    {
        return $this->id;
    }
    public function getName(): string
    {
        return $this->name;
    }
    public function getPrice(): int
    {
        return $this->price;
    }
    public function getCategory(): string
    {
        return $this->category;
    }
}

The client code:
//$product = new Product(200, 'Bread', 200, 'Food'); // A different product category
$product = new Product(100, 'Chair', 5000, 'Furniture');

$taxForChair = calculateTax($product);
var_dump($taxForChair); // float 1050
exit;

function calculateTax(Product $product): float {
    // The switch statement below decreases adaptablity.
    // The function also violates the Open/Closed principle.
    // At this point, it has 2 cases.
    // When the Business Rule change by create a new product category, then this function needs to change too.
    switch($product->getCategory()) {
        case 'Furniture':
            $tax = $product->getPrice() / 100 * 21;
            break;
        case 'Food':
            $tax = $product->getPrice() / 100 * 9;
            break;
        default:
            $tax = 0;
            break;
    }

    return (float)$tax;
}

Would you be so kind and write a better calculateTax function?

I fixed it by myself by introducing abstract classes, see below:
class Product {
    private int $id;
    private string $name;
    private int $price;
    private Category $category;

    public function __construct(int $id, string $name, int $price, Category $category) {
        $this->id = $id;
        $this->name = $name;
        $this->price = $price;
        $this->category = $category;
    }
    public function getId(): int
    {
        return $this->id;
    }
    public function getName(): string
    {
        return $this->name;
    }
    public function getPrice(): int
    {
        return $this->price;
    }
    public function getCategory(): Category
    {
        return $this->category;
    }
}

here it goes
abstract class Category {
    abstract public function calculateTax(int $price): float;
}
class CategoryFurniture extends Category {
    public function calculateTax(int $price): float {
        return (float)($price / 100 * 21);
    }
}
class CategoryFood extends Category {
    public function calculateTax(int $price): float {
        return (float)($price / 100 * 9);
    }
}

//$product = new Product(200, 'Bread', 200, new CategoryFood);
$product = new Product(100, 'Chair', 5000, new CategoryFurniture);

$taxForChair = calculateTax($product);
var_dump($taxForChair); // float 1050
exit;

function calculateTax(Product $product): float {

    $price = $product->getPrice();

    $category = $product->getCategory();
    return $category->calculateTax($price);
}


LeetCode 753: Cracking the Safe I'm posting my code for a LeetCode problem copied here. If you have time and would like to review, please do so. Thank you!
Problem

There is a box protected by a password. The password is a sequence of n digits where each digit can be one of the first k digits 0, 1,
..., k-1.

While entering a password, the last n digits entered will automatically be matched against the correct password.

For example, assuming the correct password is "345", if you type "012345", the box will open because the correct password matches the
suffix of the entered password.

Return any password of minimum length that is guaranteed to open the box at some point of entering it.

Accepted Code
#include <vector>
#include <string>

class Solution {
    int n;
    int k;
    int value;
    std::vector<std::vector<bool>> visited;
    std::string base_sequence;

public:
    std::string crackSafe(int n, int k) {
        if (k == 1) {
            return std::string(n, '0');
        }

        this->n = n;
        this->k = k;
        value = 1;

        for (int index = 0; index < n - 1; index++) {
            value *= k;
        }

        visited.resize(value, std::vector<bool>(k, false));
        depth_first_search(0);
        return base_sequence + base_sequence.substr(0, n - 1);
    }

private:
    // Depth first search the neighbor value
    void depth_first_search(int neighbor) {
        for (int index = 0; index < k; index++) {
            if (!visited[neighbor][index]) {
                visited[neighbor][index] = true;
                depth_first_search((neighbor * k + index) % value);
                base_sequence.push_back('0' + index);
            }
        }
    }
};

Reference
LeetCode has a template for answering questions. There is usually a class named Solution with one or more public functions which we are not allowed to rename.

Problem

Discuss

Bellman Ford algorithm

Constant members
n and k can be constant, so long as

you remove them from being parameters to crackSafe
you add them as parameters to a constructor
the constructor uses inline initialization syntax, i.e. n(n)

Basically, your two member functions should - in their current form - be bare functions outside of a class, since the class member variables only really have transient meaning. For this to make sense as a class, n and k would only make sense as "permanent" attributes per instance.


Google CodeJam 2016 - Ruby - Counting Sheep I just wrote a small Ruby class to solve the first of the challenges in the G CodeJam. I am not strong at this sort thing so very sure it can be simplified massively! The program is correct for both the small and large inputs.

Challenge

  Problem

  Bleatrix Trotter the sheep has devised a strategy that helps her fall
  asleep faster. First, she picks a number N. Then she starts naming N,
  2 × N, 3 × N, and so on. Whenever she names a number, she thinks about
  all of the digits in that number. She keeps track of which digits (0,
  1, 2, 3, 4, 5, 6, 7, 8, and 9) she has seen at least once so far as
  part of any number she has named. Once she has seen each of the ten
  digits at least once, she will fall asleep.

  Bleatrix must start with N and must always name (i + 1) × N directly
  after i × N. For example, suppose that Bleatrix picks N = 1692. She
  would count as follows:

  N = 1692. Now she has seen the digits 1, 2, 6, and 9. 2N = 3384. Now
  she has seen the digits 1, 2, 3, 4, 6, 8, and 9. 3N = 5076. Now she
  has seen all ten digits, and falls asleep. What is the last number
  that she will name before falling asleep? If she will count forever,
  print INSOMNIA instead.

  Input

  The first line of the input gives the number of test cases, T. T test
  cases follow. Each consists of one line with a single integer N, the
  number Bleatrix has chosen.

  Output

  For each test case, output one line containing Case #x: y, where x is
  the test case number (starting from 1) and y is the last number that
  Bleatrix will name before falling asleep, according to the rules
  described in the statement.

class CountingSheep
  def solve(input)
    return "INSOMNIA" if input == 0
    current = input
    seen_numbers = []

    loop do
      seen_numbers.concat(current.to_s.split(//))
      break if seen_numbers.uniq.size == 10
      current += input
    end
    current
  end

  def get_inputs(data_size)
    array = []
    path = "~/Desktop/counting_sheep/A-#{data_size}-practice.in"

    File.open(File.expand_path(path), 'rb') do |f|
      f.each_line do |line|
        array << line.strip
      end
    end
    array.shift
    array
  end

  #'small' or 'large' data_size input
  def output(data_size: "small")
    count = 1
    get_inputs(data_size).each do |input|
      output = solve(input.to_i)
      puts "Case ##{count}: #{output}"
      count += 1
    end
  end
end

In no particular order:

I'd prefer .zero? rather than == 0
You could use a hash instead of an array and remove the need to call uniq. That method quickly gets pretty expensive, especially since you're calling it on an ever-growing array
I'd like the solve method to return either an integer or nil, instead of mixing string and number return types.

As for getting digits from a number, I'd suggest using #divmod. Coverting to a string and splitting always irks me a little, even if it is a pretty straightforward solution. But like the return type from solve, I see no reason to involve strings when trying to get numbers from a number.

Basically, you can do something like this:

require 'set'

def unique_digits(number)
  digits = Set.new
  remainder = number
  while remainder > 0
    remainder, digit = remainder.divmod(10)
    digits << digit
  end
  digits
end

This is using a Set (part of the stdlib) since it guarantees uniqueness - no need to call uniq on an array.

As for the core of the task:

def count_sheep(n)
  return nil if n.zero?
  digits = Set.new
  current = 0
  while digits.size < 10
    current += n
    digits += unique_digits(current)
  end
  current
end

All you need then is the method to run it. I'd suggest something like:

def read_input(io)
  cases = io.gets.strip.to_i
  cases.times do |i|
    n = io.gets.strip.to_i
    puts "Case ##{i+1}: #{count_sheep(n) || "INSOMNIA"}"
  end
end

Where io is a file handle or stdin. That way you can run your script in two ways; piping things to stdin, or reading from a file. All it needs is an IO object. E.g.:

read_input(ARGV[0] ? File.open(ARGV[0]) : STDIN)

should let run either $ ruby sheep.rb <file> or $ cat <file> | ruby sheep.rb

In all:

require 'set'

def unique_digits(number)
  digits = Set.new
  remainder = number
  while remainder > 0
    remainder, digit = remainder.divmod(10)
    digits << digit
  end
  digits
end

def count_sheep(n)
  return nil if n.zero?
  digits = Set.new
  current = 0
  while digits.size < 10
    current += n
    digits += unique_digits(current)
  end
  current
end

def read_input(io)
  cases = io.gets.strip.to_i
  cases.times do |i|
    n = io.gets.strip.to_i
    puts "Case ##{i+1}: #{count_sheep(n) || "INSOMNIA"}"
  end
end

read_input(ARGV[0] ? File.open(ARGV[0]) : STDIN)


Calculating calendar year for a student based on DOB I've written this brief function for calculating which calendar year a student would be in Year 1 based on their DOB (date of birth). In 1998 there was a change in the DOB cut points for starting Year 1, hence the need for a function.

year1 <- function(x){
    if(x <= as.Date("1996-12-31")) {out <- as.numeric(format(x, "%Y")) +6}
    if(x >= as.Date("1997-07-01")) {ifelse(as.numeric(format(x, "%m")) >=07 , out <- as.numeric(format(as.Date(x), "%Y")) + 7, out <- as.numeric(format(x, "%Y")) + 6)}
    if(x >= as.Date("1997-01-01") & x <= as.Date("1997-06-30")) {out <- 2003}
    return(out)
}

An example of the results:

> year1(as.Date("1996-02-15"))
[1] 2002
> year1(as.Date("1999-02-15"))
[1] 2006

I plan to run it like this:

> test <- c(as.Date("1985-09-15"),as.Date("1999-02-15"),as.Date("2004-08-15"))
> sapply(test,year1)
[1] 1991 2006 2011

I have a lot of data to run this over, and the use of so many ifs and assigning the result to out just to write it seems pretty redundant, but I couldn't think of a better way to do it.

I think this is equivalent to your function.

year1 <- function(dob) as.numeric(format(dob, "%Y")) + 6 +
    (as.Date("1997-07-01") <= dob & 7 <= as.numeric(format(dob, "%m")))

You would run it like this.

> test <- c(as.Date("1985-09-15"),as.Date("1999-02-15"),as.Date("2004-08-15"))
> year1(test)


Find length of largest substring with unique characters The below code scans a string and returns the length of the largest substring containing all unique characters.

I am iterating over the input string's characters and analyzing the longest unique charactered substring ending at the current character. By caching some info about the longest substring ending at the PREVIOUS characters, we can quickly make statements about this one.

I am not too concerned about the logic or runtime of this code, as I am only starting to get back into my interview prep. I am more concerned this code does not look "javascript" enough for more seasoned javascript developers. Coming from an OOP (Java, C#, typescript) background, I might be trying to force certain structures onto the code that might not be suitable.

let LongestSubstringNoRepeats = function (s) {
    let currSubstring = {
        length: 1,
        substring: s[0],
    }

    currSubstring.chars = {};
    currSubstring.chars[s[0]] = true;

    let currMax = 1;
    for (let i = 1; i < s.length; ++i) {

        if (currSubstring.chars[s[i]]) {
            //repeat character
            let substring = currSubstring.substring;
            let index = substring.indexOf(s[i]);
            substring = substring.slice(index + 1);
            newChars = {} ;
            substring.split("").map((char) => {
                newChars[char] = true;
            });
            currSubstring.substring = substring;
            currSubstring.chars = newChars;
            currSubstring.length = substring.length;

        } else {
            currSubstring.length++;
            currSubstring.substring = currSubstring.substring.concat(s[i]);
            currSubstring.chars[s[i]] = true;
        }

        if (currSubstring.length > currMax){
            currMax = currSubstring.length;
        }
    }
    return currMax;
}

I am more concerned this code does not look "javascript" enough

There's no such thing as "javascript enough". If that meant anything, for me it would be keeping it simple. The best code is one that's written in a way that you never have to run to debug. Errors will show themselves voluntarily.

  returns the length of the largest substring

But your function isn't named that way. It's named LongestSubstringNoRepeats which sounds like it returns the substring, not the length. Always name appropriately. Do not give ambiguous or confusing names. It may be popular among devs that naming is one of the two hardest problems in computer science, but that only happens because it's a detail easily missed.

substring.split("").map((char) => {
  newChars[char] = true;
});

Never use functions outside their intended purpose. array.map is intended to create another array based on the values of an existing array, a 1:1 transform. In this case, you used it as a loop. If you intend to do that, use array.forEach instead.

Also, avoid unnecessary creation of objects. In this case string.split creates an array, array.map also creates another. This operation is probably possible using only string manipulation.

let currSubstring = {
    length: 1,
    substring: s[0],
}

currSubstring.chars = {};
currSubstring.chars[s[0]] = true;

// to

let currentSubString = {
  length: 1,
  substring: s[0],
  chars: { [s[0]]: true }
}

Can be shortened by using computed property names.

One way to check for repeats is to use Set. It's like an array, but will only hold unique values. Once you add in something that already exists, Set never changes size - something you can check if a character already repeated.

Always name your variables fully and explicitly. Abbreviations should be avoided as it will only cause headaches down the road. JS can be compressed with tools so code size is never an issue. Write for a human, not for the machine.

Always provide test cases. In this case, just looking at the code doesn't give me the impression that it works. It may not work at all or has edge cases you never thought of. TDD is always a good start, especially for API-ish code/code that doesn't involve anything else other than a function that returns a value.


Rust idiomatic option handling Is there a neater way of handling the option response from req.cookie() in this code block (line 3 onwards)? Id like to avoid the nested if statements, and multiple returns if at all possible. req.cookie() returns an Option<Cookie> and the Cookie struct has a value function. If the cookie is present but has the wrong value, i want to return a bad request, however i also want to return a bad request if the cookie is not present at all. If the cookie value is OK, i want to continue and do further work:
#[get("/callback")]
async fn callback(req: HttpRequest, client: web::Data<BasicClient>, web::Query(cb): web::Query<Callback>) -> Result<HttpResponse> {
    if let Some(state_cookie) = req.cookie(STATE_COOKIE_NAME) {
        if state_cookie.value() != cb.state {
            return Ok(HttpResponse::BadRequest().finish())
        }
    } else {
        return Ok(HttpResponse::BadRequest().finish())
    }

    let token_result = client.exchange_code(AuthorizationCode::new(cb.code)).request(http_client);

    return match token_result {
        Ok(token_details) => Ok(handle_success(token_details)),
        Err(token_error) => Ok(handle_error(token_error))
    }
}

There is a way without multiple returns and nested if statements.
You have three control flow pathways in your program, related to three states of state_cookie respectively:

cookie is Some and state_cookie.value() == cb.state
cookie is Some and state_cookie.value() != cb.state
cookie is None

For the first, we proceed, whereas for second and third we branch to do the same return. Do the 2. and 3. have anything in common? They cover all except one value.
You may be coming from a language where data types lack expressiveness. Many languages don't have Option at all. In Rust, we have deep operations on Algebraic Data Types like Option.
To postpone comparison and branching until we have all information, we do a map on the Option: req.cookie(STATE_COOKIE_NAME).map(|sc| sc.value()). Then, a simple comparison divides our function into two control flow paths. As an extra, for self-documenting code, we make a short-lived binding for state_cookie_value:
#[get("/callback")]
async fn callback(req: HttpRequest, client: web::Data<BasicClient>, web::Query(cb): web::Query<Callback>) -> Result<HttpResponse> {
    let state_cookie_value = req.cookie(STATE_COOKIE_NAME).map(|sc| sc.value());
    if state_cookie_value != Some(cb.state) {
        return Ok(HttpResponse::BadRequest().finish());
    }

    let token_result = client.exchange_code(AuthorizationCode::new(cb.code)).request(http_client);

    match token_result {
        Ok(token_details) => Ok(handle_success(token_details)),
        Err(token_error) => Ok(handle_error(token_error))
    }
}

This is basically all in this code.
I believe this may obscure the code's meaning for the reader, but you can as well write the match statement as:
Ok(token_result.map_or_else(handle_error, handle_success))


Type / Constructor testing utilities My argument sanitization lib has a byproduct which does some typechecking and constructor investigation. I'm unsure if I'm using the fastest / most efficient approach here.

What are your opinions? Total rubbish or does it have a right to exist? ;)

// Return the type of an object aka safe typeof
function typeOf(o) {
    return Object.prototype.toString.call(o).match(/(\w+)\]/)[1];
}

// Return the name of a function aka class name
function nameOf(o) {
    return typeOf(o) == "Function"
        ? Function.prototype.toString.call(o).match(/function\s?(\w*)\(/)[1]
        : false;
}

// Return the expected arguments of a function
function argumentsOf(o) {
    if(typeOf(o) == "Function") {
        var args = Function.prototype.toString.call(o).match(/\((.+)\)\s*{/);
        if(!!args && !!args[1]) return args[1].replace(/\s+/g, "").split(",");
        else return [];
    } else return false;
}

Fun question,

I believe the only reliable way of getting the name and parameters of a function is to indeed parse the toString. However, you can figure out whether the parameter is a function without a regex. I would propose from here :

function isType(object, type){
     return object != null && type ? object.constructor.name === type.name : object === type;
}

// Return the name of a function aka class name
function nameOf(o) {
    return isType( o , Function )
        ? Function.prototype.toString.call(o).match(/function\s?(\w*)\(/)[1]
        : false;
}

There are still a few things that bother me here, why is the parameter name o, which is the Spartan convention for Object whereas we expect a function ? Also, why name the function nameOf, which creates an expectation of being to name anything whereas it really only can give the name for a function, maybe it ought to be called nameOfFunction or functionName(). Finally, as per the comments, making a function with name return false is making the name lie, just return ''. Something like this then:

// Return the name of a function aka class name
function functionName(f) {
    return isType( f , Function )
        ? Function.prototype.toString.call(f).match(/function\s?(\w*)\(/)[1]
        : '';
}

Finally, for argumentsOf, I sincerely dislike dropping the newline in if statements, removing curly braces is fine, but no newlines makes it too hard to read.


Increasing speed of Project Euler #14 - Longest Collatz Sequence I'm solving Project Euler problems and uploading my solutions to GitHub. Some of my solutions are just based on math and are thanks to that very fast, but #14 is way too slow, and I have no idea how to optimize it.

"""
Longest Collatz sequence
------------------------

The following iterative sequence is defined for the set of positive integers:
n  n/2 (n is even)
n  3n + 1 (n is odd)
Using the rule above and starting with 13, we generate the following sequence:
13  40  20  10  5  16  8  4  2  1
It can be seen that this sequence (starting at 13 and finishing at 1) contains 10 terms. Although it has not been proved yet (Collatz Problem), it is thought that all starting numbers finish at 1.
Which starting number, under one million, produces the longest chain?
NOTE: Once the chain starts the terms are allowed to go above one million.

Approach:

A function `collatz` which returns the number of terms in the chain, given it's starting position.
It goes trough the sequence, but It also remembers previous ones using a global dictionary.
Get the maximum one.
"""

collatz_counts = {}

def collatz(n):
    count = 1
    start = n
    while n!=1:
        if n in collatz_counts:
            count += collatz_counts[n]
            break
        if n%2==0: n/=2
        else: n = 3*n+1
        count += 1

    collatz_counts[start] = count
    return count

answer = max(((collatz(i),i) for i in range(1,1000000)))
print("The starting position generating the longest sequence is {0}".format(answer[1]))

You have a bug. Try calling collatz(2) a few times and you'll see the result increment by one each time.

The source of trouble is initializing count to 1. It means you always add at least 1 to the memoized value. Note however that initializing count to 0 instead would give incorrect results when the loop terminates by the n != 1 condition.

You can avoid this bug and simplify your code at the same time: if you initialize collatz_counts to {1: 1} you can loop while n not in collatz_counts: and eliminate the if ... break.

Note also that an odd number is always followed by an even number in the sequence, so you can take two steps with  n = (n * 3 + 1) // 2

I suppose you are on Python 3 because you are using print(). Note that n/=2 is float division. n becomes a float which slows down subsequent calculations. Use n //= 2 instead.


Build Hash with 2 input variables I have a case that looks like this :

John has 120 candies, He needs some plastic bags and marks each plastic bag with the number and puts 50 candies in each bag. So John can only fill up to 3 plastic bags :
plastic bag 1 = 50 candies
plastic bag 2 = 50 candies
plastic bag 3 = 20 candies

I want to create a Hash from the above case. The first variable is 120 candies and second variable is 50 candies for each plastic bag. I have written a method to create a Hash like that :
def make_a_hash(candies, each_candies)
  mark_number = 1
  arry = []

  begin
    put_candy = candies / each_candy > 0 ? each_candy : candies
    candies = candies - put_candy
    arry << [mark_number, put_candy]
    mark_number += 1
  end while candies > 0
  arry.to_h
end

#=> make_a_hash(120, 50)
#=> {1=>50, 2=>50, 3=>20}

That method is working, and I would like to know if there is another best practice instead my method.

The first remark I'd like to make about your function is that the names of things are not very clear as to what it is supposed to do. Imagine presenting only the code to someone, who doesn't know what problem you are trying to solve. Do you think it would be very clear?

We can make it more general, so that it can be applied to other problems. Instead of candies and plastic bags, let's use units and containers. That way, we can also calculate practically any "number of \$x\$ in \$y\$..." problems that involve integers:

Number of boxes in delivery trucks
Number of passengers in train carts
Etc.

Since we are looking to get as many full containers as we can, and then the remainder in another container, let's name the function that way as well. Here is what it looks like so far, with just the names changed:

def get_full_containers_and_remainder(units, container_capacity)
  container_number = 1
  containers = []

  begin
    units_in_this_container = units / container_capacity > 0 ? container_capacity : units
    units -= units_in_this_container
    containers << [container_number, units_in_this_container]
    container_number += 1
  end while units > 0
  containers.to_h
end

get_full_containers_and_remainder(120, 50)
# {1=>50, 2=>50, 3=>20}

So far, so good. The logic is identical, but the function is easier to understand with more general names.

The logic can be simplified to not having to rely on manually adding units one container at a time in a loop. For such small numbers this is trivial, but if you had to divide up really big numbers this would take very long.

We can instead use a bit of simple math to calculate the remainder as well as the number of containers without a loop.

Using the modulo operator we can get the remainder:

remainder = units % container_capacity # remainder of 120 ÷ 50 = 20

Then we can calculate the number of full containers by subtracting the remainder from the total units, and then dividing by the container size.

count_full_containers = (units - remainder) / container_capacity # (120 - 20) ÷ 50 = 2

From there it's simply a matter of adding that many full containers, and then one more container with the remainder. Note that I added a check at the beginning to return an empty hash early if either of the values is zero, to avoid division-by-zero errors.

Working demo on repl.it

def get_full_containers_and_remainder(units, container_capacity)
  if units == 0 || container_capacity == 0
    return [].to_h
  end

  remainder = units % container_capacity
  count_full_containers = (units - remainder) / container_capacity
  container_number = 1
  containers = []

  begin
    containers << [container_number, container_capacity]
    container_number += 1
  end while container_number <= count_full_containers
  containers << [container_number, remainder]
  return containers.to_h
end

puts get_full_containers_and_remainder(120, 50) # {1=>50, 2=>50, 3=>20}
puts get_full_containers_and_remainder(0, 50)   # {}
puts get_full_containers_and_remainder(120, 0)  # {}


Returning a byte array from a method public abstract class Element
{
    // Removed for verbosity ...

    protected abstract byte[] GetBytes();
}

I'm attempting to create an engine that can create a document in a very specific format. The document itself is composed of various elements, and each element has a very specific binary representation.

I originally thought that perhaps I should have an abstract method that returns a byte[] of the byte data will be written to a stream (file, memory, whatever). For example, one element would be formatted as so:

public sealed class ElementX : Element
{
    // Removed for verbosity ...

    public override byte[] GetBytes()
    {
        var nl      = Encoding.GetBytes("\r\n");
        var opening = Encoding.GetBytes("begin-elem-x");
        var closing = Encoding.GetBytes("end-elem-x");

        var data   = Encoding.GetBytes(Data);
        var result = Encoding.GetBytes(Result);

        return Util.BlockCopy(new[] { opening, nl, data, nl, closing, nl, result });
    }
}

The problem is that it just doesn't feel right to have a method that is meant for serialization. I thought about creating a custom serializer, but since each element is serialized in a very specific format (too much variation from element to element), I think that each element needs to own the code that allows it to determine the binary representation of itself; I don't know if a class exclusively for serialization of this format will work, as it would simply consist of a large if-else/switch branch and it would force me to expose private members as internal/protected/public so that this serializer could properly serialize the element.

I'm also not sure if there are any interfaces that specifically deal with this that are part of the Portable Class Libraries (ISerializable is not).

Additionally, I know there is a Code Analysis rule stating not to return a byte[] as a public property getter (CA1819: Properties should not return arrays). I don't think this necessarily applies to methods though.

I was thinking about using ImmutableArray from the Immutable Collections NuGet instead of byte[]. This seems to make sense because the value returned is immutable at the time; the value returned will only be different if a property value on the class is changed.

I could be wrong, but I haven't seen other designs of serialization than :

Serializer with reflection (customizable through attributes/converter/mapper).

This design is more complicated, in a sense that you have to plan ahead every the possible details of serialization. It is also harder to wire everything up, while leaving room for customization.

  it would simply consist of a large if-else/switch branch

This really shouldn't happen unless you plan to do this : switch(element.GetType().Name). You should use reflection to pull every member or property(pick one), and use Attribute to filter amd format them. The only switch you might need to have is opposibly for the premitive types : string, int, enum, etc.

  it would force me to expose private members as internal/protected/public

You don't have to, as you can obtain information on any member of a class, be protected or private.
Serializable by exposing a serialize method

This design is more straight-forward where each element serializes its own stuffs and delegates the rest to the respective element.

I'm going to assume the document is composed of hierarchical elements. If that is case, copying and merging the bytes array could get very costly, more as we get closer to the root element. Since you mentionned that you're working with a "very specific binary representation" and writing to a "stream", it would be best to write directly on the stream, rather than copying and merging the bytes, and holding them around.

public abstract class Element
{
    // Removed for verbosity ...

    protected abstract void Serialize(BinaryWriter writer);
}

public class ElementX : Element
{
    public string Result { get; set; }
    public Element Data { get; set; }

    protected override void Serialize(BinaryWriter writer)
    {
        const string NewLine = "\r\n";
        const string Opening = "begin-elem-x";
        const string Closing = "end-elem-x";

        writer.Write(Opening);
        writer.Write(NewLine);
        writer.Write(Data);
        writer.Write(NewLine);
        writer.Write(Closing);
        writer.Write(Result);
    }
}

internal static class SerializationExtensions
{
    public static void Write(this BinaryWriter writer, Element element)
    {
        element.Serialize(writer);
    }
}


Suggestions on how to improve this .htaccess file for page load speed Here is my standard .htaccess file, any suggestions on how I can improve it with regards to page load speed? Would gZip instead of DEFLATE make any sort of difference?

ExpiresActive On
RewriteEngine on
RewriteBase /

# compress text, html, javascript, css, xml:
AddOutputFilterByType DEFLATE text/plain
AddOutputFilterByType DEFLATE text/javascript
AddOutputFilterByType DEFLATE text/html
AddOutputFilterByType DEFLATE text/xml
AddOutputFilterByType DEFLATE text/css
AddOutputFilterByType DEFLATE application/xml
AddOutputFilterByType DEFLATE application/xhtml+xml
AddOutputFilterByType DEFLATE application/rss+xml
AddOutputFilterByType DEFLATE application/javascript
AddOutputFilterByType DEFLATE application/x-javascript

# Expires 1 month after file is first accessed
ExpiresByType image/jpeg A2592000
ExpiresByType image/jpg A2592000
ExpiresByType image/gif A2592000
ExpiresByType image/png A2592000
ExpiresByType image/x-icon A2592000
ExpiresByType text/plain A2592000

# Expires 1 month after file is modified
ExpiresByType application/x-javascript M2592000
ExpiresByType text/css M2592000
ExpiresByType text/javascript M2592000
ExpiresByType text/html M2592000

Honestly, this looks OK, though you could clean it up a little bit.

You have two ways to make this code better:

group related mime types.
use human-readable syntax

Regarding point #2, here's part of the config I created after watching Illya Grigorik's presentation "Breaking the 1000ms Time to Glass Mobile Barrier":

<IfModule mod_deflate.c>
# these are known to be safe with MSIE 6
AddOutputFilterByType DEFLATE text/html text/plain text/xml

# everything else may cause problems with MSIE 6
AddOutputFilterByType DEFLATE text/css
AddOutputFilterByType DEFLATE application/x-javascript application/javascript application/ecmascript
AddOutputFilterByType DEFLATE application/rss+xml

ExpiresActive On
ExpiresDefault "access plus 15 minutes"

ExpiresByType text/html "access"
ExpiresByType application/json "access"

ExpiresByType text/css "access plus 1 week"
ExpiresByType image/gif "access plus 1 week"
ExpiresByType image/png "access plus 1 week"
ExpiresByType image/jpeg "access plus 1 week"
ExpiresByType application/javascript "access plus 1 week"
</IfModule>

You might also look into Google's mod_pagespeed for Apache.


Displaying IP addresses in Go using Stringer interface I am trying out some Go code examples (while coding a private project) to get more "in-depth" knowledge about the language.

I have come across an exercise from the Go Tour website about displaying correctly an IP number, making use of type method and the Stringer interface.

I found two ways to achieve the goal but I am wondering if there is no other clean way of doing it.

I strongly believe that a method with fewer code lines is always better - even though, looking around some Go OpenSource projects differs from that!

Example 1 with range.

package main

import "fmt"

type IPAddr [4]byte

// TODO: Add a "String() string" method to IPAddr.
func (ip IPAddr) String() string {
    rs := ""

    for k, v := range ip {
        if k == 0 {
            rs += fmt.Sprintf("%v", v)
            continue
        }

        rs += fmt.Sprintf(".%v", v)
    }

    return rs
}

func main() {
    hosts := map[string]IPAddr{
        "loopback":  {127, 0, 0, 1},
        "googleDNS": {8, 8, 8, 8},
    }
    for name, ip := range hosts {
        fmt.Printf("%v: %v\n", name, ip)
    }
}

Example 2 with a simple fmt.Sprintf

package main

import "fmt"

type IPAddr [4]byte

// TODO: Add a "String() string" method to IPAddr.
func (ip IPAddr) String() string {
    return fmt.Sprintf("%v.%v.%v.%v", ip[0], ip[1], ip[2], ip[3])
}

func main() {
    hosts := map[string]IPAddr{
        "loopback":  {127, 0, 0, 1},
        "googleDNS": {8, 8, 8, 8},
    }
    for name, ip := range hosts {
        fmt.Printf("%v: %v\n", name, ip)
    }
}

What do you guys suggest?

Your first variant is unnecessary detailed and hard to read:

an IPv4 address consists of exactly 4 digit groups
4 is a small number, maybe it's worth to inline the for loop
the for loop treats the case k == 0 specially
the += operator for strings allocates a new string each time
the code is much longer than the description in the task "join the four numbers with dots"

Your second variant is short, easy to grasp and probably efficient enough for all practical cases.

func (ip IPAddr) String() string {
    return fmt.Sprintf("%v.%v.%v.%v", ip[0], ip[1], ip[2], ip[3])
}

You can replace the %v with %d to make it more specific, since the arguments are integer values.

The main reason for choosing this variant is readability. The format string shows at a glance how the formatted string will look like.

If you want to write the fastest possible code, you should probably allocate a byte array of size 3+1+3+1+3+1+3 and append each number and dot individually, without using a loop at all.


PDO Is this registration secure and well coded? i finally have been moved to PDO (from MySQLi) and i m now trying to make simple login and registration page (for training purposes).

This code does work! But i want to know if it is secure?

Also witch way is better for prepared statements to pass values with ? or :username?
Is the way i use PDO is simple or should i change something?

// Define variables and initialize with empty values
$username = $password = $confirm_password = "";
$username_err = $password_err = $confirm_password_err = "";

// Processing form data when form is submitted
if($_SERVER["REQUEST_METHOD"] == "POST"){

    // Validate username
    if(empty(trim($_POST["username"]))){
        $username_err = "Please enter a username.";
    } else{
        // Prepare a select statement
        $sql = "SELECT id FROM users WHERE username = :username";

        if($stmt = $pdo->prepare($sql)){
            // Bind variables to the prepared statement as parameters
            $stmt->bindParam(":username", $param_username, PDO::PARAM_STR);

            // Set parameters
            $param_username = trim($_POST["username"]);

            // Attempt to execute the prepared statement
            if($stmt->execute()){
                if($stmt->rowCount() == 1){
                    $username_err = "This username is already taken.";
                } else{
                    $username = trim($_POST["username"]);
                    $email = trim($_POST["email"]);
                }
            } else{
                echo "Oops! Something went wrong. Please try again later.";
            }
        }

        // Close statement
        unset($stmt);
    }

    // Validate password
    if(empty(trim($_POST["password"]))){
        $password_err = "Please enter a password.";
    } elseif(strlen(trim($_POST["password"])) < 6){
        $password_err = "Password must have atleast 6 characters.";
    } else{
        $password = trim($_POST["password"]);
    }

    // Validate confirm password
    if(empty(trim($_POST["confirm_password"]))){
        $confirm_password_err = "Please confirm password.";
    } else{
        $confirm_password = trim($_POST["confirm_password"]);
        if(empty($password_err) && ($password != $confirm_password)){
            $confirm_password_err = "Password did not match.";
        }
    }

    // Check input errors before inserting in database
    if(empty($username_err) && empty($password_err) && empty($confirm_password_err)){

        // Prepare an insert statement
        $sql = "INSERT INTO users (username, email,  password) VALUES (:username, :email, :password)";

        if($stmt = $pdo->prepare($sql)){
            // Bind variables to the prepared statement as parameters
            $stmt->bindParam(":username", $param_username, PDO::PARAM_STR);
            $stmt->bindParam(":email", $param_email, PDO::PARAM_STR);
            $stmt->bindParam(":password", $param_password, PDO::PARAM_STR);

            // Set parameters
            $param_username = $username;
            $param_email = $email;
            $param_password = password_hash($password, PASSWORD_DEFAULT); // Creates a password hash

            // Attempt to execute the prepared statement
            if($stmt->execute()){
                // Redirect to login page
                header("location: login");
                die();
            } else{
                echo "Something went wrong. Please try again later.";
            }
        }

        // Close statement
        unset($stmt);
    }

    // Close connection
    unset($pdo);
}

Thanks in advance!

I don't really like the kind of a review that is popular on this site which only states that your code has some problems but doesn't really offer any solution or improvement.

Given for some problems listed it's really hard to find a plausible solution, I would prefer an answer where author at least tries to provide a solution, so it will be sort of a sanity check for the problem stated.

Now to your code. It is pretty secure but not well coded. For me, the main problem here is the code repetition. For some reason each value is accessed and duplicated through many different variables. Why not to keep just one variable initialized at the top?

Also, empty() is never needed when a variable is deliberately set. You can always use a variable itself for the purpose.

The PDO code could be also improved, in two ways

Positional placeholders are less verbose and there is no real use fro the named placeholder of only one variable is  going to be used.
Such a manual error handling (i.e. if($stmt = $pdo->prepare($sql))) is useless and harmful at the same time. An error message like "Oops! Something went wrong." would help noone. Please read my exhaustive explanation on the PHP error reporting for the details.

So here is the 1st tier refactoring, just to make your code tidy and meaningful:

<?php
// Processing form data when form is submitted
if($_SERVER["REQUEST_METHOD"] == "POST"){

    // why not to define a variable already with a value?
    $username = trim($_POST["username"]);
    $email = trim($_POST["email"]);
    $password = trim($_POST["password"]);
    $confirm_password = trim($_POST["confirm_password"]);

    // I would rather make errors an array than separate variables;
    $error = [];

    // Validate username
    if(!$username){
        $error['username'] = "Please enter a username.";
    } else{
        $sql = "SELECT id FROM users WHERE username = ?";
        $stmt = $pdo->prepare($sql);
        $stmt->execute([$username]);
        if($stmt->rowCount()){
            $error['username'] = "This username is already taken.";
        }
    }

    // Validate password
    if(!$password){
        $error['password'] = "Please enter a password.";
    } elseif(strlen($password) < 6) {
        $error['password'] = "Password must have at least 6 characters.";
    }

    // Validate confirm password
    if(!$confirm_password){
        $error['confirm_password'] = "Please enter confirm password.";
    } elseif ($password != $confirm_password){
        $error['confirm_password'] = "Password did not match.";
    }

    // Check input errors before inserting in database
    if(!$error){
        $param_password = password_hash($password, PASSWORD_DEFAULT); // Creates a password hash

        // Prepare an insert statement
        $sql = "INSERT INTO users (username, email,  password) VALUES (:username, :email, :password)";

        $stmt = $pdo->prepare($sql);
        // Bind variables to the prepared statement as parameters
        $stmt->bindParam(":username", $username);
        $stmt->bindParam(":email", $email);
        $stmt->bindParam(":password", $param_password);

        $stmt->execute();
        header("location: login");
        die;
    }
}

Just keep in mind that refactoring is an endless process, you can improve any code. but better to do it gradually, in order for you to understand every change as opposed to adding some cargo cult code mindlessly.