Function to determine the effect of a lightning bolt on a game character using "abilities" I'm writing my first game and would love some advice on how to get away from all this hardcoding I'm using...
Each Character has "abilities" (all individual functions) but I can't seem to figure out how to streamline this. Example:

def lightning_bolt(self):
    if self.is_stunned:
        print("Stunned...ability unsuccessful.")
    elif self.is_confused:
        confused_test = roll_d_x(2)
        if confused_test == 1:
            print("Confused...ability unsuccessful.")
    else:
        defender.mobility = (defender.mobility / 2)
        success = self.success_check()
        defender.mobility = (defender.mobility * 2)
        if success:
            volatile_roll = roll_d_x(10)
            if volatile_roll == 1:
                self.volatile_magic()
            else:
                defender.lose_health(((self.energy * 0.5) + 10))
                self.lose_energy(25)
                stun_test = roll_d_x(4)
                if stun_test == 1:
                    defender.is_stunned = True
        else:
            print("Ability failed.")

Note: all abilities must pass this "stunned and confused test" and a "volatile test". I will be defining a function for those. *I wanted to make "class Ability:", but I want each character to have their own individual abilities. Is there a way to nest classes or something? Any ideas to streamline aspects like "cost" and "damage dealt", etc.?

I have asked that you provide more code, and I hope that you do.

Until then, I suggest you have a look at the Visitor Pattern and its implementation using multiple dispatch.

You have a lightning attack. You are applying certain effects, including some effects that are common to all attacks.

You have not yet reached the point where some defenders will be immune to lightning, or take reduced damage, or immune to stun, but you will.

It makes more sense to Tell, don't Ask in thise situation. You tell the defender that they were attacked by lightning, and let the defender handle things.

The default behavior would be substantially what you have shown. But you could then create a defender subclass that was immune to lightning, or suffered reduced damage, and the logic would be located with that particular defender, which improves your code organization and maintainability.

Beyond that, since your defender seems to be a class already (based on the code you have shown), it should be easy to encode the "standard attack checks" into the base class in a single method. You can then raise an exception or return a sentinel value to indicate that the attack is blocked due to stun or confusion, or just print out the results (not recommended).


Expanding compressed string I wrote this for a problem in one of the online contest at hackerearth. Problem statement is:

  Input:

    The first line contains the compressed string in form a2b4h5a2

    Expanding it means aabbbbhhhhhaa.

    You have to sort the expansion in increasing order(a-z). ie. aaaabbbbhhhhh

    This is followed by Q that indicates the number of queries.

    Follows Q lines each line contain an integer K.

  Output:

    For every question output the Kth character if it exists else print -1 .

It works correctly as intended, but help me make it faster as it exceeds time limit for that portal.
Any constructive criticism and advice is appreciated. :)

The idea is that, I made a integer array of size 26 to store the respective frequency of characters, thus sorting step is done automatically. Then just started subtracting the frequency value from the query until its zero or negative, that index is the required alphabet.

#include<stdio.h>

int main()
{

   // made a array to store frequency of alphabets

    int ar[26] = {0};
    int n=0, i=0, c=0, Q=0, K=0, m=0, chint=0;
    char str[10000];                   // use to store the compressed format string
    scanf("%10000[^\n]", str);

    for(i=0;str[i];i=i++)
    {
        if((str[i]>='a' && str[i]<='z'))
         {
             ar[m] += n;
             m = str[i]-97;
             n=0;
         }
        else
        {
            chint=(int)str[i]-48;
            n=(n*10)+chint;
        }
    }
    ar[m] += n;

    scanf("%d",&Q);
    while(Q--)
    {
        scanf("%d",&K);
        for(i = 0; i<26; i++)
        {
            K = K - ar[i];
            if(K<=0)
            {
                printf("%c\n", (i+97));
                break;
            }
        }
        if(K>0)
            printf("-1\n");
    }

    return 0;
}

Warning

Enabling your compiler's warnings would tell you that c is unused.

Variable declarations

Following previous comments, I consider it good practice to declare your variable in the smallest possible scope and as late as possible. This makes things clearer (and prevent you from having unused variables). Using a "recent" version of C allows you to declare your variable in the for loop.

While/for

Your loop over Q is written as a while loop but could be written as a for loop.

i=i++

This is pretty weird code and according to my compiler warnings, might be undefined behavior.

  expand.c:10:25: warning: operation on ‘i’ may be undefined [-Wsequence-point]

More about this here.

Ascii values

You are mixing literal characters (like 'a') that are easy to understand and ascii values (like '97') for no obvious reason. I'd rather have the literal characters used everywhere. It makes clearer the fact that we try to convert chars to indices in once case and to numbers in the other case.

Using temp variable

It would probably make sense to use a temporary variable for str[i] in order to make things clearer.

Variable names

The variable names you are using could probably be improved. Among other things m seems pretty cryptic to me.

Draft

At this stage, I have the following code:

#include<stdio.h>

int main()
{
    int ar[26] = {0};
    char str[10000];
    scanf("%10000[^\n]", str);

    int last_char = 0;
    int nb_occ = 0;

    for(int i=0;str[i];i=i++)
    {
        int c = str[i];
        if((c>='a' && c<='z'))
        {
            ar[last_char] += nb_occ;
            last_char = c-'a';
            nb_occ=0;
        }
        else
        {
            int chint=(int)c-'0';
            nb_occ=(nb_occ*10)+chint;
        }
    }
    ar[last_char] += nb_occ;

    int Q = 0;
    scanf("%d",&Q);
    while(Q--)
    {
        int K = 0;
        scanf("%d",&K);
        for(int i = 0; i<26; i++)
        {
            K = K - ar[i];
            if(K<=0)
            {
                printf("%c\n", (i+97));
                break;
            }
        }
        if(K>0)
            printf("-1\n");
    }

    return 0;
}

Code organisation

On this kind of task, it is usually worth trying to split the input/output logic from the actual computing logic. Here, you could define 2 functions : one to initialise some object like structure from a string, one to use the structure to get the Kth character (K being the argument to the method).

This would allow you to write unit tests which would help you improve your algorithm.

Algorithm

You have used the correct ideas : use the proper data structure to store the different values used.

An improvement you could implement is to define a structure with the cumulated numbers that you compute once so that then you can get the kth element with a simple array lookup (actually as suggested by ChrisWue, maybe you'd need a binary search and not a single lookup). When multiple queries are performed (which is I think what is happening in your case), this should make things much faster.

The idea of doing some ("long") preprocessing once to make (multiple) queries much faster is a pretty classic solution in real life and in programming challenges.


How to deal with elimination of duplicate logic vs. cost of complexity increase? I just wrote some code that is very representative of a recurring theme (in my coding world lately): repeated logic leads to an instinct to eliminate duplication which results in something that is more complex the tradeoff seems wrong to me (the examples of the negative side aren't worth posting - but this is probably the 20th console utility I've written in the past 12 months).

I'm curious if I'm missing some techniques or if this is really just on of those "experience tells you when to do what" type of issues.

Here's the code... I'm tempted to leave it as is, even though there will be about 20 of those if-blocks when I'm done.

static void Main(string[] sargs)
{
    try
    {
        var urls = new DirectTrackRestUrls();
        var restCall = new DirectTrackRestCall();
        var logger = new ConsoleLogger();
        Args args = (Args)Enum.Parse(typeof(Args), string.Join(",", sargs));
        if (args.HasFlag(Args.Campaigns))
        {
            var getter = new ResourceGetter(logger, urls.ListAdvertisers, restCall);
            restCall.UriVariables.Add("access_id", 1);
            getter.GotResource += new ResourceGetter.GotResourceEventHandler(getter_GotResource);
            getter.GetResources();
            SaveResources();
        }
        if (args.HasFlag(Args.Advertisers))
        {
            var getter = new ResourceGetter(logger, urls.ListAdvertisers, restCall);
            restCall.UriVariables.Add("access_id", 1);
            getter.GotResource += new ResourceGetter.GotResourceEventHandler(getter_GotResource);
            getter.GetResources();
            SaveResources();
        }
        if (args.HasFlag(Args.CampaignGroups))
        {
            var getter = new ResourceGetter(logger, urls.ListCampaignGroups, restCall);
            getter.GotResource += new ResourceGetter.GotResourceEventHandler(getter_GotResource);
            getter.GetResources();
            SaveResources();
        }
    }
    catch (Exception e)
    {
        Console.WriteLine(e.InnerException);
        Console.WriteLine(e.StackTrace);
    }

Why not something like...

static void Main(string[] sargs)
{
    try
    {
        var urls = new DirectTrackRestUrls();
        var restCall = new DirectTrackRestCall();
        var logger = new ConsoleLogger();
        Args args = (Args)Enum.Parse(typeof(Args), string.Join(",", sargs));
        if (args.HasFlag(Args.Campaigns))
        {
            GetAndSaveResources(urls.ListAdvertisers, true);
        }
        if (args.HasFlag(Args.Advertisers))
        {
            GetAndSaveResources(urls.ListAdvertisers, true);
        }
        if (args.HasFlag(Args.CampaignGroups))
        {
            GetAndSaveResources(urls.ListCampaignGroups, false);
        }
    }
    catch (Exception e)
    {
        Console.WriteLine(e.InnerException);
        Console.WriteLine(e.StackTrace);
    }
}

void GetAndSaveResources(IList list, bool setAccessId) {
    var getter = new ResourceGetter(logger, list, restCall);
    if (setAccessId)
        restCall.UriVariables.Add("access_id", 1);
    getter.GotResource += new ResourceGetter.GotResourceEventHandler(getter_GotResource);
    getter.GetResources();
    SaveResources();
}


Alternately taking elements from an array Given the task of having to return an array that alternately takes elements from two arrays, what would be most Ruby way of completing the task? As of now, I have this code, but it doesn't feel very elegant.

def alternateTake (a, b)
  raise ArgumentError, "Unequal length" if a.length != b.length
  build = []
  (0..a.length-1).each do |x|
    build << (x.even? ? a[x] : b[x])
  end
  build
end

Example input & output:

method([1, 4, 3, 7], [1, 4, 2, 33]) = [1, 4, 3, 33]
method([98, 12, 41], [35, 22, 14]) = [98, 22, 41]
method([12, 33], [66, 45, 3]) = ERROR

This one-liner should be what you're looking for:

def alternate_take(xs, ys)
  raise ArgumentError, "Unequal length" if xs.length != ys.length
  xs.zip(ys).map.with_index{|(x,y),i| i.even? ? x : y}
end

Follow Up

Tokland's answer to OP's followup about a solution without indexes is really clever.  Here's another variation, which I don't like as much as the index version, but it answers the question:

def alternate_take(xs, ys)
  raise ArgumentError, "Unequal length" if xs.length != ys.length
  xs.zip(ys).reduce([]){|m,(x,y)| m + (m.size.even? ? [x] : [y])}
end


Immutable employee class I have created an employee object to be immutable.

package com.immutable.ex01;

import java.io.Externalizable;
import java.io.IOException;
import java.io.ObjectInput;
import java.io.ObjectOutput;
import java.text.DateFormat;
import java.text.SimpleDateFormat;
import java.util.ArrayList;
import java.util.Calendar;
import java.util.Date;
import java.util.List;

public final class Employee implements Cloneable, Externalizable{
    private final Integer age;
    private final String name;
    private final List<String> companies;
    private final Date dob;

    public Employee(Integer age, String name,List<String> companies, Date dob) {
        this.age = age;
        this.name = name;
        this.companies = companies;
        this.dob = dob;
    }

    public Integer getAge() {
        return this.age;
    }

    public String getName() {
        return this.name;
    }

    public Date getDob() {
        Calendar cal = Calendar.getInstance();
        cal.setTime(this.dob);
        return cal.getTime();
    }

    public List<String> getCompanies() {
        List<String> clone = new ArrayList<String>(this.companies.size());
        for(String companyName : this.companies) {
            clone.add(companyName);
        }
        return clone;
    }

    @Override
    public String toString() {
        StringBuffer strb = new StringBuffer();
        strb.append("AGE").append(" ").append(this.age)
            .append(", NAME ").append(this.name)
            .append(", DOB ").append(this.getDateString())
            .append(", COMPANIES WORKED IN ");
        for(String companyName : this.companies) {
            strb.append(companyName).append(",");
        }

        strb.deleteCharAt(strb.length() - 1);
        return strb.toString();
    }

    @Override
    public Object clone() throws CloneNotSupportedException{
        throw new CloneNotSupportedException("Cannot be cloned");
    }

    @Override
    public void writeExternal(ObjectOutput out) throws IOException {
        throw new IOException("This operation is not supported");
    }

    @Override
    public void readExternal(ObjectInput in) throws IOException {
        throw new IOException("This operation is not supported");
    }

    private String getDateString() {
        String date = null;
        DateFormat df = null;
        try {
            df = new SimpleDateFormat("dd MMM yyyy");
            date = df.format(this.dob);
        }catch(Exception e) {
            e.printStackTrace();
        }
        return date;
    }
}

My main method class

package com.immutable.ex01;

import java.text.DateFormat;
import java.text.SimpleDateFormat;
import java.util.ArrayList;
import java.util.Date;
import java.util.List;

public class MainTest01 {
    public static void main(String [] args) {
        Employee emp = null;
        List<String> companyList = null;
        Date date = null;
        try {
            companyList = new ArrayList<String>();
            companyList.add("IBM");
            companyList.add("Google");
            companyList.add("Norton");
            companyList.add("Seable");
            companyList.add("Nekki");
            date = toDate("28 Oct 1981");
            emp = new Employee(23, "John Molkovich", companyList, date);

            date = emp.getDob();

            date = new Date();

            companyList = emp.getCompanies();
            companyList.add("Toyo Corp");
            System.out.println(emp);
        }catch(Exception e) {
            e.printStackTrace();
        }
    }

    private static Date toDate(String dateString) throws Exception{
        Date d = null;
        DateFormat df = new SimpleDateFormat("dd MMM yyyy");
        d = df.parse(dateString);
        return d;
    }
}

Please review my code and check if I have achieved immutability for my employee class.

Your Employee is frozen in time; they can never get any older!

Age should be calculated from date-of-birth and “today”, not stored in an immutable field.

The getCompanies() method could be written simply as:

return new ArrayList<>(this.companies);

Or perhaps:

return Collections.unmodifiableList(this.companies);


Toggling a CSS animation I'm trying to toggle a dropdown box that will appear and disappear upon clicking the button.

var clickState = false;

$("#show").on("click", function() {
  if (!clickState) {
    $(".animated").removeClass("off");
    renewElement($(".animated"));
    $(".animated").addClass("on");
    clickState = true;
  } else if (clickState) {
    $(".animated").removeClass("on");
    renewElement($(".animated"));
    $(".animated").addClass("off");
    clickState = false;
  }
});

function renewElement(e) {
  var newElement = e.clone(true);
  e.remove();
  $(".container").append(newElement);
}
.controls {
  text-align: center;
}
button {
  display: inline-block;
}
.textbox {
  width: 280px;
  margin: 0 auto;
  background: tomato;
  padding: 10px;
}
.animated {
  max-height: 0;
  overflow: hidden;
  animation-name: none;
  animation-duration: 0.5s;
  animation-fill-mode: forwards;
}
.triangle {
  content: " ";
  width: 0;
  height: 0;
  border: 10px solid;
  border-color: transparent transparent tomato transparent;
  position: relative;
  left: 50%;
  top: -9px;
  margin-left: -7px;
}
.on {
  animation-name: slideOpen;
  animation-direction: normal;
}
.off {
  animation-name: slideOpen;
  animation-direction: reverse;
}
@keyframes slideOpen {
  0% {
    max-height: 0px;
    padding: 0px;
  }
  100% {
    max-height: 150px;
  }
}

  <button id="show">Show div</button>

text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text

I'm using a clickState boolean to keep track of the on and off presses, creating the toggle.
I'm also cloning the entire div and recreating it so that the animation will refresh properly. It seems that without this, the animation will only run once.
The animation is connected to the element with the .addClass() method.

I'm new to animation, but I can't help but feel I'm writing some really hideous code here. For starters, the code is clearly not reusable. I would also like to know if there is a better way of repeating the animation without cloning and then appending the element.

Feel free to fork my pen on CodePen to test any changes.

This is what I've come up with, which is nicer than the if/else if statement I was originally using.

$("#show").on("click", function() {
  renewElement($(".animated"));
  var $animated = $(".animated");
  var shown = $animated.hasClass('on');
  $animated.toggleClass('on', !shown).toggleClass('off', shown);
});

function renewElement(e) {
  var newElement = e.clone(true);
  e.remove();
  $(".container").append(newElement);
}
.controls {
  text-align: center;
}

button {
  display: inline-block;
}

.textbox {
  width: 280px;
  margin: 0 auto;
  background: tomato;
  padding: 10px;
}

.animated {
  max-height: 0;
  overflow: hidden;
  animation-name: none;
  animation-duration: 0.5s;
  animation-fill-mode: forwards;
}
.triangle {
  content: " ";
  width: 0;
  height: 0;
  border: 10px solid;
  border-color: transparent transparent tomato transparent;
  position: relative;
  left: 50%;
  top: -9px;
  margin-left: -7px;
}

.on {
  animation-name: slideOpen;
  animation-direction: normal;
}

.off {
  animation-name: slideOpen;
  animation-direction: reverse;
}

@keyframes slideOpen {
  0% {
    max-height: 0px;
    padding: 0px;
  }
  100% {
    max-height: 150px;
  }
}

  <button id="show">Show div</button>

text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text

As you can see, I've kept the renewElement() function. After reading this article it seems that there isn't much you can do.


excel-vba function to filter an array and return an array I have a function to filter a 2-d array.
It works. But I am not sure if the following mechanism is a sensible concept.
The idea is:

loop over the input array which is received as input_arr(y,x)

store the values in temp_arr(x,y), since in order to resize the y-dimension it
needs to be last dimension, so I have to transpose the values as I
store them

once the loop is done, transpose temp_arr(x,y) which
becomes the return value
Public Function filter_arr(input_arr As Variant, col_index As Long, filter_value As Variant) As Variant

 'the input_arr might be indexed starting from 0 or 1
 Dim n1 As Long, n2 As Long
 n1 = LBound(input_arr, 2)
 n2 = UBound(input_arr, 2)

 Dim temp_arr() As Variant
 Dim y As Long, x As Long, count As Long
 count = 0

 If (LBound(input_arr, 1) = 0) Then
  For y = LBound(input_arr, 1) To UBound(input_arr, 1)
   If (input_arr(y, col_index) = filter_value) Then

    ReDim Preserve temp_arr(n1 To n2, 0 To count)
    For x = n1 To n2
     temp_arr(x, count) = input_arr(y, x)
    Next x
    count = count + 1

  End If
 Next y

 Else

 'if LBound(input_arr, 1) = 1
 For y = LBound(input_arr, 1) To UBound(input_arr, 1)

  If (input_arr(y, col_index) = filter_value) Then

   count = count + 1
   ReDim Preserve temp_arr(n1 To n2, 1 To count)
   For x = n1 To n2
    temp_arr(x, count) = input_arr(y, x)
   Next x

   End If
  Next y

 End If

 filter_arr = Application.Transpose(temp_arr)

End Function

Edit: adding some context
Imagine that I have a ws in an .xlam file that contains product information which I've added as an add-in.
I have a function like =getProductData() which returns an array containing that data
So excel has a SORT function which works great with this kind of thing. I can do =SORT(getProductData(), 2, 1) to return my product data sorted by the 2nd column
But excel's FILTER function needs me to specify the data itself as its second parameter, not just its index, like in SORT
I can use INDEX to do something like =SORT(FILTER(getProductData(),INDEX(getProductData(),,3)="red",""),2,1) to filter my product data by the 3rd column if the value = red, then sort by the 2nd column of the resulting array
But I was hoping to do something like
=SORT(filter_arr(getProductData(),3,"red"),2,1) to do the same
Naturally I'm still just playing around to determine what I can pull off - maybe using INDEX is the way to go and I'm trying to reinvent the wheel

ReDim Preserve and Transpose are not free.  I use this pattern when I need to filter an array:

Declare a 1-D array (Indices) with the same number of rows as the 2D array
When a match is found add the matches index to the Indices array and increment the counter
If there is matching data, declare a 2-D array (Results) with the same dimensions of the Input array but a row count equal to the counter
Iterate over the indices in the Indices array and add the matching values to the Results
Return the Results
If there are no matches return an empty array

Quick Filter
Public Function QuickFilter(input_arr As Variant, col_index As Long, filter_value As Variant) As Variant
    Dim r As Long
    Dim Count As Long
    Dim Indices As Variant
    ReDim Indices(LBound(input_arr, 1) To UBound(input_arr, 1))
    Count = LBound(input_arr, 1) - 1
    For r = LBound(input_arr, 1) To UBound(input_arr, 1)
        If input_arr(r, col_index) = filter_value Then
            Count = Count + 1
            Indices(Count) = r
        End If
    Next

    Dim Results As Variant
    Dim c As Long
    Dim index As Long
    If Count >= LBound(input_arr, 1) Then
        ``` we want results to hold the entire row
        ReDim Results(LBound(input_arr, 1) To Count, LBound(input_arr, 2) To UBound(input_arr, 2))
        For r = LBound(input_arr, 1) To Count
            index = Indices(r)
            For c = LBound(input_arr, 2) To UBound(input_arr, 2)
                Results(r, c) = input_arr(index, c)
            Next
        Next
        QuickFilter = Results
    Else
        QuickFilter = Array()
    End If
End Function


Represent a person who has different roles for different offices People have different types of roles depending on the office they are working at. For example, a person has two roles (cashier,cleaner) at OfficeMax and five roles(manager,security,etc) at OfficeLess.  Typescript is used.

interface Person {
    name : string,
    roles : OfficeRoles
}

interface OfficeRoles {
    roles : OfficeRole[],
    office : Office
}

interface OfficeRole {
    getType() : RoleType
}

interface Office {
    name : string
}

enum RoleType {
    CLERK,CUSTODIAN,SECURITY;
}

The following Person object would represent John, who is a Clerk and Security at OfficeMax:

let office : Office = { name : "OfficeMax" };
let roles : Role[] = [ new ClerkRole( "John" ) , new SecurityRole(1) ];
let person : Person = { name : "John" , roles : roles };

class ClerkRole implements Role {
    private name : string;
    constructor( name : string ) {
        this.name = name;
    }
    getType() { return RoleType.CLERK; }
}

class SecurityRole implements Role {
    private id : number;
    constructor( id : number ) {
        this.id = id;
    }
    getType() { return RoleType.SECURITY; }
}

Is the above a good representation of the requirements?  Specifically, I had trouble with the OfficeRole interface as the different office roles do not share any commonality.

Here's my comments on your code.

interface Person {
    name : string,
    /**
     * Because you can have an employee working at multiple offices, they cannot have only one OfficeRoles object.
     * The roles relationship should reflect the potential for multiple offices.
     */
    roles : OfficeRoles
}

interface OfficeRoles {
    roles : OfficeRole[]
    office : Office
}

interface OfficeRole {
    /**
     * This doesn't need to be a function, you could implement a readonly property in the class that's assigned
     * when the OfficeRole is created.
     */
    getType() : RoleType
}

/**
 * The Office object does not include the roles it has, or the employees who currently hold those roles.
 */
interface Office {
    name : string
}

/**
 * I'd recommend against going with an Enum if the type of roles could be office specific.
 */
enum RoleType {
    CLERK,CUSTODIAN,SECURITY
}

let office : Office = { name : "OfficeMax" };
// Does not compile, there is no Role class or interface.
let roles : Role[] = [ new ClerkRole( "John" ) , new SecurityRole(1) ];
let person : Person = { name : "John" , roles : roles };

/**
 * No Role interface exists.
 * Also, you shouldn't subclass OfficeRole just to provide a different value for {@link getType}, that can be done
 * in a constructor.
  */
class ClerkRole implements Role {
    private name : string;
    constructor( name : string ) {
        this.name = name;
    }
    getType() { return RoleType.CLERK; }
}

class SecurityRole implements Role {
    private id : number;
    constructor( id : number ) {
        this.id = id;
    }
    getType() { return RoleType.SECURITY; }
}

Here's the direction I'd take it in. This does not include any methods you'd normally have to manage the relationships between employees and roles, nor does it include the classes (they're straightforward). It also doesn't include any methods you'd need to maintain the integrity of your object graph.

/**
 * Represents an employee.
 * An employee can work at different offices and does not have a relationship with an office beyond having a
 * role there, therefore office is not modeled on the employee directly.
 */
interface IEmployee {
    name : string
    roleAssignments: IAssignedRole[];
}

/**
 * An employee can work at multiple offices with different roles, which means a N:N relationship between
 * {@link IEmployee} and {@link IRole}.
 */
interface IAssignedRole {
    employee: IEmployee;
    role: IRole;
}

/**
 * Assumes there can be any number of roles for a given office. Each role needs a name and exists for only one office.
 */
interface IRole {
    name: string;
    office: IOffice;
}

/**
 * Represents an office.
 */
interface IOffice {
    name: string
    /**
     * Defines roles permitted for this office.
     */
    roles : IRole[];
    /**
     * Captures the roles assignments for this office.
     */
    roleAssignments : IAssignedRole[];
    /**
     * Provides list of employees by iterating through the role assignments.
     */
    readonly employees: ReadonlyArray<IEmployee>;
}

Let me know if you have any questions :)


Custom scrollable div I'm trying to create a custom div scroller.  I got it working, but it's still a little rough around the edges. Please take a look..

DEMO

<%@ Page Language="C#" AutoEventWireup="true" CodeFile="Default.aspx.cs" Inherits="_Default" %>

<!DOCTYPE html>

    <title></title>

    <form id="form1" runat="server">

    </form>

Just focusing on the Javascript:

I feel a urge to to reference a story about a monk. Instead I'll just say that descriptive names will help when you come back to your code or when others try to read it.
for example:

var obj = document.getElementById(id);
obj.style.position = "absolute";
obj.onmousedown = function () {
    scr = obj;
}

Here it would be better to name obj something like draggableElement or some such. this was I know instantly what we are referring to.
You are testing e and pageY and assigning a variable in two different way. Pick one way and be consistent about it. It really matters not what you choose so much as sticking to it.

Here are a couple of different ways you could do it.

if

if (!e) e = window.event;
var y = e.pageY;
if(!y) y = e.clientY;

ternary

e = e ? e : window.event;
var y = e.pageY ? e.pageY : e.clientY;

or

e = e || window.event;
var y = e.pageY || e.clientY;

I personally favour this last way.


Mapping from string "aa=bb" to Dictionary I am converting a input like :

aa=bb
Fruit1=Potato
Fruite2=Banana

Into a Dictionary, using this code.

string tmp = "\r\naa=bb\r\nFruit1=Potato\r\nFruite2=Banana";
Dictionary<string, string> tempValueList = new Dictionary<string, string>();

var lines = tmp.Split(new[] { "\r\n", "\n" }, StringSplitOptions.RemoveEmptyEntries);
foreach (var line in lines)
{
    var keyValue = line.Split(new[] { '=' }, StringSplitOptions.RemoveEmptyEntries);
    tempValueList.Add(keyValue[0],keyValue[1]);
}

tempIniObject.Key = key;
tempIniObject.ValueList = tempValueList;

This is simple, but I believe there is a mutch classier way to do it.

Why not to use regular expression? Your task can be solved with this code:

var dictionary = Regex.Matches(tmp, "(?<key>.+)=(?<value>.+)")
                      .OfType<Match>()
                      .Where(m => m.Success)
                      .ToDictionary(m => m.Groups["key"].Value.Trim(),
                                    m => m.Groups["value"].Value.Trim());


How is my injection protection? What do I need to do to prevent injections?  In addition, if I have any statement/parameter errors, please tell me.

 <?
 $username  = 'username';
 $pass      = 'pass';
 $cpass     = 'cpass';
 $email     = 'email';
 $gender    = 'gender';
 $firstname = 'firstname';
 $lastname  = 'lastname';
 $phone     = 'phone';
 $address   = 'address';

 $username    = htmlspecialchars($username, ENT_QUOTES);
 $pass        = htmlspecialchars($pass, ENT_QUOTES);
 $cpass       = htmlspecialchars($cpass, ENT_QUOTES);
 $email       = htmlspecialchars($email, ENT_QUOTES);
 $gender      = htmlspecialchars($gender, ENT_QUOTES);
 $firstname   = htmlspecialchars($firstname, ENT_QUOTES);
 $lastname    = htmlspecialchars($lastname, ENT_QUOTES);
 $phone       = htmlspecialchars($phone, ENT_QUOTES);
 $address     = htmlspecialchars($address, ENT_QUOTES);

 $uLength     = strlen($username);
 $pLength     = strlen($pass);
 $cpLength    = strlen($cpass);
 $emailLength = strlen($email);
 if ($uLength >= 6 && $uLength <= 35) {

} else {

$errorMessage = $errorMessage . "Username must be between 6 and 35 characters" . "
";

}
if ($emailLength >= 3 && $emailLength <= 42) {

} else {

$errorMessage = $errorMessage . "Email must be between 3 and 42 characters" . "
";

}

if ($pLength >= 8 && $pLength <= 64) {

} else {

$errorMessage = $errorMessage . "Password must be between 8 and 64 characters" . "
";

}
if ($cpLength >= 8 && $cpLength <= 64) {

} else {

$errorMessage = $errorMessage . "Confirmation password must be between 8 and 64 characters" . "
";
}
$user_name    = 'u904609109_dev';
$pass_word    = '_____';
$database     = 'u904609109_users';
$server       = 'mysql.2freehosting.com';

$con = mysqli_connect($server, $user_name, $pass_word, $database);
if (mysqli_connect_error()) {
    die('Connect Error: (' . mysqli_connect_errno() . ') ' . mysqli_connect_error());
}

echo 'Connect Error:' . $mysqli->host_info . "\n";

$result = mysqli_query($con, $database);
if (!$result) {
    die("Failed to load " . mysqli_error($con));
}

$dblink = "SELECT * FROM users WHERE (username= '" . $username . "')";
$dblink->bind_param('username', $username);
if ($dblink) {
}

$result2  = mysqli_query($result, ($dblink));
$num_rows = mysqli_num_rows($result);

if ($num_rows > 0) {

    $errorMessage = "Username already taken";

} else {
}
$dblink = $mysqli_query("SELECT * FROM users WHERE (email = '" . $email . "')");
$dblink->bind_param('username', $username);
if ($dblink) {
}
$result2  = mysqli_query($result, $dblink);
$num_rows = mysqli_num_rows($result);

if ($num_rows > 0) {

    $errorMessage = "Email already taken";

} else {
}
$username = mysqli_real_escape_string($username);
$pass = mysqli_real_escape_string($pass);
$cpass = mysqli_real_escape_string($cpass);
$gender = mysqli_real_escape_string($gender);
$email = mysqli_real_escape_string($email);
$firstname = mysqli_real_escape_string($firstname);
$lastname = mysqli_real_escape_string($lastname);
$address = mysqli_real_escape_string($address);
$phone = mysqli_real_escape_string($phone);

$username= $_POST[($username)];
$pass= $_POST[($pass)];
$cpass= $_POST[($cpass)];
$gender= $_POST[($gender)];
$email= $_POST[($email)];
$firstname= $_POST[($firstname)];
$lastname= $_POST[($lastname)];
$address= $_POST[($address)];
$phone= $_POST[($phone)];

mysqli_select_db($database, $con);

$sql = "INSERT INTO users(username, pass, cpass, gender, email, firstname, lastname, address, phone) VALUES ('" . $username . "', '" . $pass . "', '" . $cpass . "', '" . $gender . "', '" . $email . "', '" . $firstname . "',  '" . $lastname . "', '" . $address . "', '" . $phone . "')";

$result1 = mysqli_query($con, $sql);
if (!$result1 || mysqli_query($con, $sql)) {
    die('Invalid query: ' . mysqli_error());
}
echo htmlentities($sql);

header("Location: finished.html"); {
    mysqli_close($con);
}
}
?>

Give me parameterized SQL or give me death!
Seriously. Use prepared statements! Don't use mysqli_query as that makes your code vulnerable to SQL Injection. By using prepared statements, you won't need the mysqli_real_escape_string calls anymore.

This code looks nasty:
$result1 = mysqli_query($con, $sql);
if (!$result1 || mysqli_query($con, $sql)) {
    die('Invalid query: ' . mysqli_error());
}

First you perform the query. If the query fails (i.e. it returns a false-ish value), you die, but if the query works then you call the query again. And if the second query is successful, then you call die.
THAT MAKES NO SENSE!

This code can't possibly be working:
$dblink = "SELECT * FROM users WHERE (username= '" . $username . "')";
$dblink->bind_param('username', $username);

And again, don't concatenate SQL queries! Use prepared statements!

Use better variable names!
What is $result1 and what is $result2? Describe what the result is for in the variable name.

You have a whole bunch of empty blocks, such as this:
if ($emailLength >= 3 && $emailLength <= 42) {

} else {

$errorMessage = $errorMessage . "Email must be between 3 and 42 characters" . "
";

}

Also work on your indentation. Fixing the indentation, the if-logic and the string concatenation, and I suggest that you write your HTML tags with lowercase, this can be:
if ($emailLength < 3 || $emailLength > 42) {
    $errorMessage .= "Email must be between 3 and 42 characters
";
}


Prime numbers, Prime Factors and LCM I am trying to generate LCM by prime factorization. I have already done the other way around to generate LCM using GCD but I am trying to achieve it using prime factorization.

I am getting right prime factors but the problem is that to generate LCM we need to multiply each factor the greatest number of times it occurs in either number's factors. Reference

Now I don't like the code I come up with to achieve this. The logic is to retrieve factors of both numbers and then create a hash/counter for each factor. Finally multiply the factor with the result so far and the greatest number of times in each hash counter list.

I would like to request to review my all methods IsPrime, PrimeFactors, LeastCommonMultipleByPrimeFactorization etc. In particular the main LeastCommonMultipleByPrimeFactorization method. Please feel free to pass your comments and suggestions. I would be happy to see some simplest way to achieve this.

Code snippet:

    public static bool IsPrime(int n)
    {
        for (int i = 2; i <= n; i++)
        {
            if (n % i == 0)
            {
                if (i == n)
                    return true;
                else
                    return false;
            }
        }
        return false;
    }

    public static int[] PrimeFactors(int n)
    {
        var factors = new List<int>();
        for (int i = 2; i <= n; i++)
        {
            while (n % i == 0 && IsPrime(i))
            {
                factors.Add(i);
                n = n / i;
            }
        }
        return factors.ToArray();
    }

    public static int LeastCommonMultipleByPrimeFactorization(int m, int n)
    {
        //retrieve prime factors for both numbers
        int[] mFactors = PrimeFactors(m);
        int[] nFactors = PrimeFactors(n);

        //generate hash code to get counter for each factor
        var mFactorsCountHash = CreateCounterHash(mFactors);
        var nFactorsCountHash = CreateCounterHash(nFactors);

        var primeFactors = new List<int>();
        primeFactors.AddRange(mFactors);
        primeFactors.AddRange(nFactors);

        int result = 1;

        //On each distinct factor... check either which number factors
        foreach (int factor in primeFactors.Distinct())
        {
            int mfactorCount = 0;
            int nfactorCount = 0;
            if (mFactorsCountHash.ContainsKey(factor))
            {
                mfactorCount = mFactorsCountHash[factor];
            }

            if (nFactorsCountHash.ContainsKey(factor))
            {
                nfactorCount = nFactorsCountHash[factor];
            }

            int numberOfCount = mfactorCount > nfactorCount ? mfactorCount : nfactorCount;
            result = factor * result * numberOfCount;
        }

        return result;
    }

    private static Dictionary<int, int> CreateCounterHash(IEnumerable<int> mFactors)
    {
        Dictionary<int, int> hash = new Dictionary<int, int>();
        foreach (var factor in mFactors)
        {
            if (hash.ContainsKey(factor))
                hash[factor]++;
            else
                hash.Add(factor, 1);
        }
        return hash;
    }

To test method, I am using following code

[Test]
    public void LeastCommonMultipleByPrimeFactorizationPositiveTest()
    {
        Assert.AreEqual(42, ArthmeticProblems.LeastCommonMultipleByPrimeFactorization(21, 6));

        Assert.AreEqual(12, ArthmeticProblems.LeastCommonMultipleByPrimeFactorization(4, 6));
    }

  Note: This is just for fun and revision of my basic concepts

IsPrime

public static bool IsPrime(int n)
{
    for (int i = 2; i <= n; i++)
    {
        if (n % i == 0)
        {
            if (i == n)
                return true;
            else
                return false;
        }
    }
    return false;
}

Under what circumstances can the final return false be reached? Why not handle those special cases explicitly, and then simplify the loop body by reducing it?

public static bool IsPrime(int n)
{
    if (n < 1) throw new ArgumentException("n");
    if (n == 1) return false;

    for (int i = 2; i < n; i++)
    {
        if (n % i == 0) return false;
    }
    return true;
}

Of course, there are better ways of testing primality than trial division, but that's beside the point.

PrimeFactors

public static int[] PrimeFactors(int n)
{
    var factors = new List<int>();
    for (int i = 2; i <= n; i++)
    {
        while (n % i == 0 && IsPrime(i))
        {
            factors.Add(i);
            n = n / i;
        }
    }
    return factors.ToArray();
}

Any particular reason for returning int[] instead of IEnumerable<int>?

What's the purpose of the IsPrime call there? It's easy to prove that it always returns true (and so the IsPrime method isn't needed at all).

As a point of style, I prefer op= methods, but this is really subjective.

The loop guard might be more intelligible as n > 1.

public static IEnumerable<int> PrimeFactors(int n)
{
    var factors = new List<int>();
    for (int i = 2; n > 1; i++)
    {
        while (n % i == 0)
        {
            factors.Add(i);
            n /= i;
        }
    }
    return factors;
}

CreateCounterHash

private static Dictionary<int, int> CreateCounterHash(IEnumerable<int> mFactors)
{
    Dictionary<int, int> hash = new Dictionary<int, int>();
    foreach (var factor in mFactors)
    {
        if (hash.ContainsKey(factor))
            hash[factor]++;
        else
            hash.Add(factor, 1);
    }
    return hash;
}

Why does the return type use the implementation Dictionary rather than the interface IDictionary?

Why hard-code the type? You don't do anything with it other than equality checking, so it could be IDictionary<T, int> CreateCounterHash<T>(IEnumerable<T> elts) (and then could potentially be an extension method of IEnumerable<T>).

ContainsKey followed by a get is a minor inefficiency. Since TryGetValue will give default(int) (i.e. 0) if the key isn't, found, this can be fixed:

private static IDictionary<T, int> CreateCounterHash<T>(IEnumerable<T> elts)
{
    Dictionary<T, int> hash = new Dictionary<T, int>();
    foreach (var elt in elts)
    {
        int currCount;
        hash.TryGetValue(elt, out currCount);
        hash[elt] = currCount + 1;
    }
    return hash;
}

LCM

public static int LeastCommonMultipleByPrimeFactorization(int m, int n)
{
    //retrieve prime factors for both numbers
    int[] mFactors = PrimeFactors(m);
    int[] nFactors = PrimeFactors(n);

    //generate hash code to get counter for each factor
    var mFactorsCountHash = CreateCounterHash(mFactors);
    var nFactorsCountHash = CreateCounterHash(nFactors);

    var primeFactors = new List<int>();
    primeFactors.AddRange(mFactors);
    primeFactors.AddRange(nFactors);

    int result = 1;

    //On each distinct factor... check either which number factors
    foreach (int factor in primeFactors.Distinct())
    {
        int mfactorCount = 0;
        int nfactorCount = 0;
        if (mFactorsCountHash.ContainsKey(factor))
        {
            mfactorCount = mFactorsCountHash[factor];
        }

        if (nFactorsCountHash.ContainsKey(factor))
        {
            nfactorCount = nFactorsCountHash[factor];
        }

        int numberOfCount = mfactorCount > nfactorCount ? mfactorCount : nfactorCount;
        result = factor * result * numberOfCount;
    }

    return result;
}

Looks buggy: factor * numberOfCount will only be correct when numberOfCount == 1 or factor == 2 && numberOfCount == 2.

Also a tad repetitive. Can't you merge the two counts earlier?

public static IDictionary<K, V> MergeDictionaries<K, V>(IDictionary<K, V> d1,
                                                        IDictionary<K, V> d2,
                                                        Func<V, V, V> mergeFunc)
{
    // Left as an exercise
    // Only call mergeFunc when both dictionaries contain the key
}

You can also use a bit of Linq to do the aggregate, so the LCM simplifies to

public static int LeastCommonMultipleByPrimeFactorization(int m, int n)
{
    //retrieve prime factors for both numbers
    int[] mFactors = PrimeFactors(m);
    int[] nFactors = PrimeFactors(n);

    //generate hash code to get counter for each factor
    var mFactorsCountHash = CreateCounterHash(mFactors);
    var nFactorsCountHash = CreateCounterHash(nFactors);

    var combinedCounts = MergeDictionaries(mFactorsCountHash, nFactorsCountHash, Math.Max);
    return combinedCounts.Aggregate(1, (prod, primeWithMult) => prod * pow(primeWithMult.Key, primeWithMult.Value));
}


Bush Wanderer - code intended for teaching I am teaching computing to an AS class and this was their assignment:

  Bush Wanderer

  Task 1

  Create a 5x5 grid of hashes(#) where the player is an x on the
  board

  Prompt the player to move in either N, E, S or W direction and then
  reprint the board to show the new position.

  Example:

  x # # # #
# # # # #
# # # # #
# # # # #
# # # # #
Which way would you like to move?(N,E,S or W)

  Task 2

  Ensure the player does not move off of the board
  Place a hidden chest somewhere on the board at random
  End the game when the chest is found
  Tell the player how many moves it took to find the chest
  Add a visible (but blind) dragon that can teleport around the board    at random and will eat you and end your game if it gets you before
  you get the chest

This was my exemplar solution at the end and I wanted to get some feedback on what the professional community thought of it. How could this be improved?

import random

grid = 5

moves = 0

x = 0
y = 0

cx = random.randint(0,(grid-1))
cy = random.randint(0,(grid-1))

while(True):

  dx = random.randint(0,(grid-1))
  dy = random.randint(0,(grid-1))

  for i in range(grid):
      if i == dy and i == y:
        for u in range(grid):
          if u == dx:
            print('@ ', end='')
          elif u == x:
            print('x ', end='')
          else:
            print('# ', end='')
        print()
      elif i == y:
          print('# '*x, end="")
          print('x ', end="")
          print('# '*((grid-x)-1))
      elif i == dy:
          print('# '*dx, end="")
          print('@ ', end="")
          print('# '*((grid-dx)-1))
      else:
        for i in range(grid):
          print('# ', end="")
        print()

  if dx == x and dy == y:
    if moves == 1:
      print('You died in', moves,'move!')
    elif moves > 1 or moves == 0:
      print('You died in', moves,'moves!')
    break
  elif cx == x and cy == y:
    if moves == 1:
      print('You won in', moves,'move!')
    elif moves > 1 or moves == 0:
      print('You won in', moves,'moves!')
    break

  m = input('Which way would you like to move?(N,E,S or W)').lower()

  if m == 'n' and y > 0:
      y = y-1
  elif m == 'e' and x < (grid-1):
      x = x+1
  elif m == 's' and y < (grid-1):
      y = y+1
  elif m == 'w' and x >0:
      x = x-1
  else:
      print('Invalid move')

  moves = moves + 1

Because the code you write is to be used for teaching, the importance is not on performance, but on correctness and readability and understandability (comphrensiveness?).

So basically...

Add comments! Explain each step, so that the kids are gonna see the code, think "what does it do", and they can read it right there!
Don't use dx and dy, use more explanatory variables!

Let's combine them:

  if dx == x and dy == y:
    if moves == 1:
      print('You died in', moves,'move!')
    elif moves > 1 or moves == 0:
      print('You died in', moves,'moves!')
    break
  elif cx == x and cy == y:
    if moves == 1:
      print('You won in', moves,'move!')
    elif moves > 1 or moves == 0:
      print('You won in', moves,'moves!')
    break

This is hard to understand. dx and dy were hard to grasp at first - when doing games programming, usually dx and dy are "delta-x" and "delta-y", to calculate object velocity.

But it's dragon x y and chest x y here!

So just call it that.

  if dragon_x == x and dragon_y == y:
    if moves == 1:
      print('You died in', moves,'move!')
    elif moves > 1 or moves == 0:
      print('You died in', moves,'moves!')
    break
  elif chest_x == x and chest_y == y:
    if moves == 1:
      print('You won in', moves,'move!')
    elif moves > 1 or moves == 0:
      print('You won in', moves,'moves!')
    break

Next, I'd usually tell you to split this up into functions. But maybe these kids don't know functions yet, and it's remarkably frustrating to be shown "the solution" when "the solution" consists of "this magic trick I didn't teach you yet, haha".

Use comments instead, and be sure to teach them what a comment is and what its for. If you have to review homework, a comment might be all you get to understand their code.

  #check for loss by comparing dragon and player location
  if dragon_x == x and dragon_y == y:
    if moves == 1:
      print('You died in', moves,'move!')
    elif moves > 1 or moves == 0:
      print('You died in', moves,'moves!')
    break
  #else, check for victory by comparing chest and player location
  elif chest_x == x and chest_y == y:
    if moves == 1:
      print('You won in', moves,'move!')
    elif moves > 1 or moves == 0:
      print('You won in', moves,'moves!')
    break

Note that function names make good comments. Instead of checkForVictory() we just put #check for victory. I recommend you start comments with why, and then with how. How-comments should eventually be removed (you can read the code), and why-comments should stay, but when learning how to program, having the explanation next to the code just helps. It also promotes micro problem-solution flow (in my opinion), which is what you need later if you want to split up code into smaller chunks or problems.

And this loop here...

  for i in range(grid):
      if i == dy and i == y:
        for u in range(grid):
          if u == dx:
            print('@ ', end='')
          elif u == x:
            print('x ', end='')
          else:
            print('# ', end='')
        print()
      elif i == y:
          print('# '*x, end="")
          print('x ', end="")
          print('# '*((grid-x)-1))
      elif i == dy:
          print('# '*dx, end="")
          print('@ ', end="")
          print('# '*((grid-dx)-1))
      else:
        for i in range(grid):
          print('# ', end="")
        print()

Is mighty confusing. There is no easily discernible pattern to it. If you had to explain the thought process that went into each step, what would you say? (Teacher, I don't understand, how did you create this?)

Here's mine:

per tile:
  print dragon if dragon is on tile
  print player if player is on tile
  print empty tile if nobody is on tile

Those are the requirements, next up is realizing that if we have a grid, we ought to do it by row and columns:

for each row
  for each column
    if the dragon is on that tile
     print dragon
    else if the player is on that tile
     print player
    else
     print empty tile
  print new line, to go to next row

And then translate to python (untested)

for i in range(grid)
  for u in range(grid)
    if i == dragon_y and u == dragon_x:
     print('@ ', end='')
    elif i == player_y and u == player_x:
     print('x ', end='')
    else:
     print('# ', end='')
  print()

Another reason for this is that if you were to get a question like, "how do we add mountains, where the dragon can wander, but the player can't?", you'd be stumped. Or at least I would be, if I had to add mountains into your display code. It's all about clever tricks with multiplying strings. You'd have to alter ALL your cases. Or if there were two dragons, because, you know, hard mode. Again, it would take you a major overhaul.

I'd just make a simple change:

for each row
  for each column
    if there is a dragon on that tile
     print dragon
    else if the player is on that tile
     print player
    else if that tile is a mountain
     print mountain
    else
     print empty tile
  print new line, to go to next row

and it supports mountains and multiple dragons!

Possibly the best thing you could do is take another look at your code, and remove the python element from it. Start in English, and convert that to python. I bet that's what the kids will be doing.

Using an approach like this, the code is going to be a lot shorter and easy to understand. Yes, it's slower by doing all the checks all the time. But for these kids, it's probably going to be more important that they understand the code, than that it runs three times as fast. Computers these days are forgiving, and you can always teach them better optimizations later.

Plus, if they were to shy away from programming because the code is too complex, that'd be a darn shame.

And get comfortable with your code before the lesson. It would be best if you could do live editing in the classroom.

As for industry standards...

  for i in range(grid):
      if i == dy and i == y:
        for u in range(grid):
          if u == dx:
            print('@ ', end='')
          elif u == x:
            print('x ', end='')
          else:
            print('# ', end='')
        print()
      elif i == y:
          print('# '*x, end="")
          print('x ', end="")
          print('# '*((grid-x)-1))
      elif i == dy:
          print('# '*dx, end="")
          print('@ ', end="")
          print('# '*((grid-dx)-1))
      else:
        for i in range(grid):
          print('# ', end="")
        print()

Extract to printGrid.

  m = input('Which way would you like to move?(N,E,S or W)').lower()

  if m == 'n' and y > 0:
      y = y-1
  elif m == 'e' and x < (grid-1):
      x = x+1
  elif m == 's' and y < (grid-1):
      y = y+1
  elif m == 'w' and x >0:
      x = x-1
  else:
      print('Invalid move')

  moves = moves + 1

Extract to pickMove, and use a while loop to make sure that the move counter (moves) doesn't go up if you make a bad move.

Encapsulate conditionals by putting checks like dy == y and dx == x into either a function isDragonEatingPlayer (naming is hard) or just a temporary variable of some sorts.


Critique my Stack Overflow schema I'm learning DB design.  How would you change the following schema which tries to replicate Stack Overflow's functionality:

create_table "questions", :force => true do |t|
    t.text      "question",         :null => false
    t.text      "description",  :null => false
    t.integer       "up_votes",         :null => false, :default => 0
    t.integer       "down_votes",       :null => false, :default => 0
  t.timestamps
end

create_table "answers", :force => true do |t|
    t.integer       "question_id",  :null => false
    t.text      "answer",               :null => false
    t.integer       "up_votes",         :null => false, :default => 0
    t.integer       "down_votes",       :null => false, :default => 0
  t.timestamps
end

create_table "question_comments", :force => true do |t|
    t.integer   "question_id",  :null => false
    t.text      "comment",          :null => false
  t.timestamps
end

create_table "answer_comments", :force => true do |t|
    t.integer   "answer_id",    :null => false
    t.text      "comment",      :null => false
  t.timestamps
end

create_table "tags", :force => true do |t|
    t.string        "tag",  :null => false, :limit => 100
  t.timestamps
end
add_index "tags", ["tag"], :name => "tag_UNIQUE", :unique => true

create_table "question_tags", :force => true do |t|
  t.integer     "question_id",  :null => false
  t.integer     "tag_id",               :null => false
  t.timestamps
end

create_table "users", :force => true do |t|
    t.string        "first_name",   :limit => 45,       :null => false
    t.string        "last_name",    :limit => 45,       :null => false
    t.string        "email",            :limit => 100,  :null => false
    t.string        "password",                                     :null => false
    t.string        "salt",                                             :null => false
  t.timestamps
end

create_table "user_votes", :force => true do |t|
    t.integer       "user_id",  :null => false
    t.integer       "question_id"
    t.integer       "answer_id"
    t.boolean       "is_positive_vote"
    t.timestamps
end

create_table "user_favorites", :force => true do |t|
    t.integer       "user_id",  :null => false
    t.integer       "question_id"
    t.integer       "answer_id"
    t.timestamps
end

create_table "question_flags", :force => true do |t|
    t.integer       "user_id",  :null => false
    t.integer       "question_id"
    t.timestamps
end

create_table "answer_flags", :force => true do |t|
    t.integer       "user_id",  :null => false
    t.integer       "question_id"
    t.timestamps
end

create_table "related_questions", :force => true do |t|
    t.integer       "question_id",                  :null => false
    t.integer       "related_question_id",  :null => false
    t.timestamps
end

Your answers table doesn't actually have an answerID, which is later referenced in your answer comments.

Your link tables for comments, I would have a primary key assigned that is separate from the answerid, since you don't want a composite key between answerID and comment (Think about updating/removing post logic). Apply the same to question comments.

Consider storing users against questions/answer submissions, otherwise, how will you know who posted it? Think about edit functionality too, do you want to audit changes?


ZigZag order of a tree traversal Please let me know your thoughts on the code below, please be brutal. Here is the question I solved:

Given a binary tree, return the zigzag level order traversal of its nodes' values. (ie, from left to right, then right to left for the next level and alternate between).

For example:
Given binary tree {3,9,20,#,#,15,7},

    3
   / \
  9  20
    /  \
   15   7

return its zigzag level order traversal as:
[
  [3],
  [20,9],
  [15,7]
]

   public ArrayList<ArrayList<Integer>> zigzagLevelOrder(TreeNode root) {
        ArrayList<ArrayList<Integer>> res = new ArrayList<>();
        Queue<TreeNode> queue = new LinkedList<>();
        if(root==null){
            return res;
        }
        ArrayList<Integer> level = new ArrayList<Integer>();
        level.add(root.val);
        res.add(level);
        int depth=1;
        queue.add(root);
        TreeNode empty = new TreeNode(2);
        queue.add(null);
        level = new ArrayList<Integer>();
        while(!queue.isEmpty()){
            TreeNode curr = queue.poll();
            if(curr==null){
                if(!queue.isEmpty()){
                    queue.add(null);
                }
                else{
                    break;
                }
                res.add(level);
                level = new ArrayList<Integer>();

                depth++;
            }
            else{
                if(depth%2==0){
                    if(curr.left!=null){
                        level.add(curr.left.val);
                        queue.add(curr.left);
                    }
                    if(curr.right!=null){
                        level.add(curr.right.val);
                        queue.add(curr.right);

                    }
                }
                else{
                    if(curr.right!=null){
                        level.add(curr.right.val);
                        queue.add(curr.right);
                    }
                    if(curr.left!=null){
                        level.add(curr.left.val);
                        queue.add(curr.left);
                    }
                }
            }
        }
        return res;
    }

Branching to change behavior is a code smell.  A cleaner design would be to use the State pattern, and let the state toggle each time you descend to the next layer of the tree.

Also, I think you've made things less clear by tangling your traversal logic, with your output logic.  Which is to say, instead of doing something with the child, and something with the child values, instead of working with the parent value and adding the children to the traversal tree.

In pseudo code, I would expect the logic to look like

currentNodes = [ root ]
while( currentNodes.notEmpty ) {
    state = state.switch
    valueList = report.nextValues

    nextNodes = []

    for ( node : currentNodes ) {
        valueList.add(node.value)

        nextNodes.add( state.order(node.left, node.right) )
    }
    currentNodes = nextNodes.reverse;
}

return report


Bit manipulator (reader / writer) Please take a review of my simple bit manipulator:
#ifndef BIT_STREAM_H
#define BIT_STREAM_H

#include <cstdint>

class BitStream {
public:
    explicit BitStream(uint8_t* buffer, size_t buffer_size) : m_Buffer(buffer), m_BufferSize(buffer_size) {

    }

    virtual ~BitStream() = default;

public:
    inline uint64_t Mask(uint64_t offset, size_t size) const {
        return ((1 << size) - 1) << offset;
    }

public:
    inline size_t GetBufferSize() const {
        return m_BufferSize;
    }

    inline size_t GetOffset() const {
        return m_Offset;
    }

    inline bool GetErrorState() const {
        return m_ErrorState;
    }

protected:
    uint8_t* m_Buffer = nullptr;
    size_t m_BufferSize = 0;
    size_t m_Offset = 0;
    bool m_ErrorState = false;
};

class BitReader : public BitStream {
public:
    explicit BitReader(uint8_t* buffer, size_t buffer_size) : BitStream(buffer, buffer_size) {

    }

    template <size_t buffer_size>
    explicit BitReader(uint8_t(&buffer)[buffer_size]) : BitStream(buffer, buffer_size) {

    }

public:
    template<const size_t bit_size, typename value_t = uint64_t>
    inline value_t Read(value_t df = 0) {
        const size_t bpb = 8;
        const size_t byte_offset = m_Offset / bpb;
        const size_t relative_offset = m_Offset - (byte_offset * bpb);

        if (m_ErrorState) {
            return df;
        }

        if (((m_Offset + bit_size) / bpb) >= m_BufferSize) {
            m_ErrorState = true;
            return df;
        }

        m_Offset += bit_size;

        return (*(value_t*)&m_Buffer[byte_offset] & Mask(relative_offset, bit_size)) >> relative_offset;
    }
};

class BitWriter : public BitStream {
public:
    explicit BitWriter(uint8_t* buffer, size_t buffer_size) : BitStream(buffer, buffer_size) {

    }

    template <size_t buffer_size>
    explicit BitWriter(uint8_t(&buffer)[buffer_size]) : BitStream(buffer, buffer_size) {

    }

public:
    template<const size_t bit_size, typename value_t = uint64_t>
    inline void Write(value_t value) {
        const size_t bpb = 8;
        const size_t byte_offset = m_Offset / bpb;
        const size_t relative_offset = m_Offset - (byte_offset * bpb);

        if (m_ErrorState) {
            return;
        }

        if (((m_Offset + bit_size) / bpb) >= m_BufferSize) {
            m_ErrorState = true;
            return;
        }

        m_Offset += bit_size;

        *(value_t*)&m_Buffer[byte_offset] = (*(value_t*)&m_Buffer[byte_offset] & ~Mask(relative_offset, bit_size)) | (value << relative_offset);
    }
};

#endif // BIT_STREAM_H

Example of usage:
#include "BitStream.h"

#include <iostream>
#include <cassert>

int main() {
    uint8_t buffer[2];
    memset(buffer, 0, sizeof(buffer));

    BitWriter writer(buffer);

    writer.Write<4>(15);
    writer.Write<2>(0);
    writer.Write<6>(63);

    assert(!writer.GetErrorState());

    BitReader reader(buffer);

    std::cout << reader.Read<4>() << std::endl;
    std::cout << reader.Read<2>() << std::endl;
    std::cout << reader.Read<6>() << std::endl;

    assert(!reader.GetErrorState());

    std::cin.ignore();
}

Later in plan add exceptions, to string converter method.

Inline isn't
This pops up on CodeReview regularly. inline is effectively ignored by most modern compilers, and even if it weren't, it isn't a good idea to force the matter -- programmers do not know what's best for their program when compared to the compiler when full optimization is enabled. It's safe to omit this.
Redundant initial values
uint8_t* m_Buffer = nullptr;
size_t m_BufferSize = 0;

These are initialized by your constructor, so there's no point in pre-initializing them here.
Type punning
This:
*(value_t*)&m_Buffer[byte_offset]

is a little gross. There are better ways to do inline pointer casts, particularly in C++. Here, since you're punning from uint8_t* to a pointer of arbitrary type, you'll probably want reinterpret_cast.
I guess bit_size should be 64 if you pass uint64_t for value_t, but I don't see why it's passed separately. You can move bpb up in scope; then bit_size would equal bpb * sizeof(value_t). Perhaps you want to support bit_size smaller than the value type, which is fine; but you should at least assert that bit_size is less than or equal to the number of bits in value_t.
There are subtler risks in the way that you're packing bits - from what I can tell, it assumes little-endian, which will not be universally true.


Python tokenizer in Haskell + Parsec I wrote a tokenizer/lexer (difference?) for Python in Haskell: this is my code on GitHub.

I already tested it out with some of CPython's standard library scripts, and it doesn't fail. However, it probably outputs non-conforming output. But I don't care about this right now.

What I'm looking for is a general (functional) style review. This is my first time using Parsec and Haskell for something relatively big, so I expect my functional style to be suboptimal.

Significant code snippets:

parseTokens :: LexemeParser [PositionedToken]
parseTokens = do
  P.skipMany ignorable
  tokens <- concat <$> P.many parseLogicalLine
  P.eof
  endIndents <- length . filter (>0) <$> getIndentLevels
  dedents <- mapM position $ replicate endIndents Dedent
  endMarker <- position EndMarker
  return (tokens ++ dedents ++ [endMarker])

parseIndentation :: LexemeParser [PositionedToken]
parseIndentation = mapM position =<< go
  where
    go = do
      curr <- length <$> P.many (P.char ' ')
      prev <- getIndentLevel
      case curr `compare` prev of
        EQ -> return []
        GT -> return [Indent] <* pushIndentLevel curr
        LT -> do
          levels <- getIndentLevels
          when (not $ elem curr levels) (fail $ "indentation must be: " ++ show levels)
          let (pop, levels') = span (> curr) levels
          putIndentLevels levels'
          return $ replicate (length pop) Dedent

parseLogicalLine :: LexemeParser [PositionedToken]
parseLogicalLine = do
  P.skipMany ignorable
  indentation <- parseIndentation
  (indentation ++) <$> begin
  where
    begin = do
      tokens <- P.many1 parsePositionedToken
      let continue = (tokens ++) <$> begin

      implicitJoin <- (> 0) <$> getOpenBraces
      if implicitJoin then do
        whitespace
        parseComment <|> (P.optional backslash *> eol)
        P.skipMany ignorable
        whitespace
        continue
      else do
        whitespace
        explicitJoin <- P.optionMaybe (backslash *> eol)
        case explicitJoin of
          Nothing   -> ((\t -> tokens ++ [t]) <$> position NewLine) <* P.optional eol
          otherwise -> whitespace *> continue

(I was largely inspired by PureScript's lexer, though I probably ended up with something a lot worse.)

for comparison

Here are some other ways I've found to parse indentation sensitive language:

Use Text.Parsec.Indent Here is a blog post containing an example: (link)
Have a look at the language-python package. It's lexer is built using alex: (link)
Purescript handles indentation via the mark, checkIndentation, indented and same functions: (link) Grep the rest of the source code for these functions to see how they are used. Its lexer defines a Indent token (link) but from what I can tell it is never returned, i.e. see (link). The indentation level is kept track of by the parser (link) via the mark function.

returning a list of tokens?

I'm not sure there is a lot of value in parsers which return [PositionedToken] except, perhaps, for testing purposes.

Most of the time you want to run a token parser incrementally, but parseTokens has to consume the entire file before it can return anything.

There should be a function which just returns the next token. This will come in handy in conjunction with the next comment I have...

handling dedents at eof

I would find another way to handle implicit dedents at the end of the file. I'm sure this can be handled at the grammar level. The code to add them is extremely messy - don't you agree?

Have a look at how the language-python package does it:

https://github.com/bjpop/language-python/blob/master/src/Language/Python/Version2/Parser/Lexer.x#L229-L242

The function lexToken returns just a single token.
The lexer has state, so when it reaches EOF it checks the current indentation level. If the indentation level is > 1, it decrements it and returns a dedentToken. Otherwise it returns an EOF token.


Simple JavaScript autosuggest component Any suggestions for improving this? I'm hoping for some JavaScript object structure / code integrity type recommendations rather than style or CSS.

var list = [
    {name: 'Afghanistan', code: 'AF'},
    {name: 'Åland Islands', code: 'AX'},
    {name: 'Albania', code: 'AL'},
    {name: 'Algeria', code: 'DZ'},
    {name: 'American Samoa', code: 'AS'},
    {name: 'AndorrA', code: 'AD'},
    {name: 'Angola', code: 'AO'},
    {name: 'Anguilla', code: 'AI'},
    {name: 'Antarctica', code: 'AQ'}
];

function Autocomplete(selector){

    this.list = list;

    this.box = document.getElementById(selector);

    this.dropdown = null;

    this.init = function(){
        this.box.addEventListener("keyup", this.boxKeyUp);
        this.dropdown = document.createElement("ul");
        this.dropdown.setAttribute('class','drop');
        document.body.appendChild(this.dropdown);
        this.updateList();
    };

    this.updateList = function(){
        this.dropdown.innerHTML = "";
        var value = this.box.value;
        for(var i=0; i < this.list.length; i++){
            var name = list[i].name.toLowerCase();
            var idx = name.indexOf(value);
            if(idx===0){
                var item = document.createElement('li');
                item.appendChild(document.createTextNode(list[i].name));
                this.dropdown.appendChild(item);
            }
        }
    };

    this.boxKeyUp = function(){
        if(this.box.value===""){
            this.dropdown.style.display = "none";
            return true;
        }
        this.updateList();
        this.dropdown.style.display = "block";
    }.bind(this);

    this.init();

    return this;
}
var ac = new Autocomplete("inputbox");
input {
    width: 200px;
}

input, .drop {
    margin: 0;
    padding: 0;
    padding: 6px;
    font-size: 16px;
    font-family: "helvetica neue", helvetica, sans-serif;
}

.drop {
    display: none;
    list-style-type: none;
    list-style-position: inside;
    border: 1px solid #eee;
}
<input type="text" name="inputbox" id="inputbox">

You should give your drop down some limitations, so that it isn't taking up all sorts of space that it shouldn't be, I started with adding a width of 200 pixels to it, but I suggest you change it to something that works for you.

I also added a background color, because I would want my dropdown to be different from the surrounding page so that I knew it was an extension of another element on the page, again you should alter this according to your needs, but don't leave it white on a white background.

I also removed the top border for the drop down.  I am not much into the graphics of things. so mine doesn't look very pretty either, but you should make it so that the drop down looks like it is a drop down. or an Auto Suggest component

I assume that you are doing this just to learn JavaScript and not for an actual production site.

Just in the case that you are doing this for a production site I would recommend that you use jQuery and depending on the source of your suggestions AJAX as well.

AutoComplete Widget from jQuery-UI

in your JavaScript, give your Curly braces some space after the function declaration or the for loop declaration or the if declaration, you gave them space in your CSS, show the JavaScript braces the same courtesy.

Here are the changes that I made to your code.  all I did was alter the CSS and add spaces before the opening braces in your JavaScript.

You should also stick you either single quotes or double quotes, it runs like this, but it is often confusing to read.

var list = [
    {name: 'Afghanistan', code: 'AF'},
    {name: 'Åland Islands', code: 'AX'},
    {name: 'Albania', code: 'AL'},
    {name: 'Algeria', code: 'DZ'},
    {name: 'American Samoa', code: 'AS'},
    {name: 'AndorrA', code: 'AD'},
    {name: 'Angola', code: 'AO'},
    {name: 'Anguilla', code: 'AI'},
    {name: 'Antarctica', code: 'AQ'}
];

function Autocomplete(selector) {

    this.list = list;

    this.box = document.getElementById(selector);

    this.dropdown = null;

    this.init = function() {
        this.box.addEventListener('keyup', this.boxKeyUp);
        this.dropdown = document.createElement('ul');
        this.dropdown.setAttribute('class','drop');
        document.body.appendChild(this.dropdown);
        this.updateList();
    };

    this.updateList = function() {
        this.dropdown.innerHTML = '';
        var value = this.box.value;
        for(var i=0; i < this.list.length; i++) {
            var name = list[i].name.toLowerCase();
            var idx = name.indexOf(value);
            if(idx===0) {
                var item = document.createElement('li');
                item.appendChild(document.createTextNode(list[i].name));
                this.dropdown.appendChild(item);
            }
        }
    };

    this.boxKeyUp = function() {
        if(this.box.value===''){
            this.dropdown.style.display = 'none';
            return true;
        }
        this.updateList();
        this.dropdown.style.display = 'block';
    }.bind(this);

    this.init();

    return this;
}
var ac = new Autocomplete('inputbox');
input {
    width: 200px;
}

input, .drop {
    margin: 0;
    padding: 0;
    padding: 6px;
    font-size: 16px;
    font-family: "helvetica neue", helvetica, sans-serif;
}

.drop {
    display: none;
    list-style-type: none;
    list-style-position: inside;
    border-style: none groove groove groove;
    border-color: silver;
    border-width:5px;
    width: 195px;
    background-color: lightgray;
}
<input type="text" name="inputbox" id="inputbox">


Are these tables concepted correctly? I want to store image (details) in my database, and want for each image the possibility to have from 1 to N versions.

I want a single file (src) to be associated with a single version. There must not be more than one version for an single image with the same dimensions and format (no point having a version with 1920x1080 jpg and 1920x1080 jpg, whereas 1920x1080 jpg and 1920x1080 png would be ok).

I have come out with the following tables :

-- ---------------------------------------------------------------------------------
-- IMAGE TABLE
-- ---------------------------------------------------------------------------------

DROP TABLE IF EXISTS "image_e";

CREATE TABLE IF NOT EXISTS "image_e"
(
    "id"    int unsigned NOT NULL AUTO_INCREMENT,
    "title" varchar(32)  NOT NULL,
    "alt"   varchar(16)  NOT NULL,

    PRIMARY KEY ("id")

)   ENGINE = MyISAM DEFAULT CHARSET = utf8 COLLATE = utf8_bin;

-- ---------------------------------------------------------------------------------
-- IMAGE VERSION TABLE
-- ---------------------------------------------------------------------------------

DROP TABLE IF EXISTS "image_version_e";

CREATE TABLE IF NOT EXISTS "image_version_e"
(
    "id"      int unsigned NOT NULL AUTO_INCREMENT,
    "imageId" int unsigned NOT NULL,
    "format"  varchar(4)   NOT NULL,
    "size"    int unsigned NOT NULL,
    "width"   int unsigned NOT NULL,
    "height"  int unsigned NOT NULL,
    "src"     varchar(64)  NOT NULL,

    PRIMARY KEY ("id")

)   ENGINE = MyISAM DEFAULT CHARSET = utf8 COLLATE = utf8_bin;

ALTER TABLE "image_version_e" ADD UNIQUE KEY ("imageId", "format", "width", "height");
ALTER TABLE "image_version_e" ADD UNIQUE KEY ("src");

image_version_e.imageId references image_e.id

This version is MyISAM, i will make a second version for InnoDB with this foreign key.

I don't care about insert / update / delete speed, but i do care about select speed as i will use this for a wallpaper gallery with a search function.

Are my unique keys done correctly ? Isn't it too much or not enough ? Is the schema well thought ? Is there a way to optimize it ?

Thanks for reading and for your help :)

The unique keys will enforce the requirements you specified, but unless your data entry program either traps error codes returned by MySQL and prompts the user for correction or checks for uniqueness before saving the record, you will probably experience a lot of record rejections where you won't understand why.

I would also use a much larger VARCHAR maximum length for the image_version_e.src column. Remember that VARCHAR columns only require as much storage space as needed for the actual data with trailing spaces stripped (usually). VARCHAR(255) or more is not unreasonable as it will accommodate those really long directory paths or URLs. Your data entry form field for it should be scrollable but much shorter... probably a multiline text field.

See more details in the MySQL documentation of CHAR and VARCHAR.


Is this a good PHP strategy pattern? I am a very beginning in PHP and Design Patterns. I have been studying the amazing Head First Design patterns. So, I have been trying to translate the original into PHP. The class is working well, however, I would like to ask:

Is this considered good PHP?
Someone mentioned that this is not a Strategy Pattern. Head First's definition: "Strategy lets the algorithm vary independently from clients that use it".  Is this not correct?

<?php

interface FlyBehavior{
    public function fly();
}

class FlyWithWings implements FlyBehavior{
    public function fly(){
        echo "I am flying!
";
    }
}

class FlyNoWay implements FlyBehavior{
    public function fly(){
        echo "I cannot fly!";
    }
}

interface QuackBehavior{
    public function quack();
}

class QuackQuack implements QuackBehavior{
    public function quack(){
        echo "Quack
";
    }
}

class MuteQuack implements QuackBehavior{
    public function quack(){
        echo "Silence";
    }
}

class Squeak implements QuackBehavior{
    public function quack(){
        echo "Squeak";
    }
}

abstract class Duck{
    protected $quackBehavior;
    protected $flyBehavior;

    public function performQuack(){
        $this->quackBehavior->quack();
    }

    public function performFly(){
        $this->flyBehavior->fly();
    }

    abstract public function display();

    public function swim(){
        echo "All ducks float, even decoy";
    }

}

class MallardDuck extends Duck{
    public function __construct(){
        $this->quackBehavior=new QuackQuack();
        $this->flyBehavior=new FlyWithWings();
    }
    public function display(){
        echo "I am real Mallard duck!";
    }
}

$mallard=new MallardDuck();
$mallard->performQuack();
$mallard->performFly();

?>

Syntax errors aside, there's a major pitfall in your design choices that you need to watch out for. Namely, if you want to figure out if your animal is capable of some behaviour, you'll need to add an identifier function for every animal. This will limit you down the road.

For example, say you are iterating over a bunch of different types of animals, you'd need something like:

foreach ($noahs_ark->animals() as $animal) {
    if ($animal->canFly()) {
        $animal->performFly();
    }
}

The problem here is that if you want to be able to check that an animal can fly, every single animal will need a "canFly()" method otherwise you'll end up with an error. If you're dealing only with birds, this probably isn't a big deal. But when you add dogs to the mix, then you have to add a ->canFly() method for every dog object too!

The solution to this is to implement your interfaces directly on your animal classes. For example:

// Assuming all birds classes extend Avian
abstract class Avian implements Flier, Quacker { ... }

foreach ($noahs_ark->animals() as $animal) {
    if ($animal instanceof Flier) {
        $animal->performFly();
    }
]

This way you can use reflection to determine types, and along with it, make assumptions that if it possesses(implements) a certain behaviour(interface), your animal can perform that action.

I'd still recommend that you make your modules implement interfaces as well to ensure consistency across specialized modules though. It'll help in cases where you might want to create factories for creating animals or in other cases as well. Hope that helps!


Implementing an alternative to scanf Since almost all C functions designed to get data from stdin are bad / flawed:

gets - the less said about this the better
scanf - no checks for buffer overflow and '\n' constantly remaining in stdin, screwing up next scanfs
scanf_s - pretty much the same, but with buffer overflow checks
fgets appends a '\n' to the string
gets_s doesn't have the previous problem, but useless for other streams

I decided to write my own function that would at least be usable to read numbers from stdin:

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
void *scano(char mode);

int main()
{

  // int *num = (int *) scano(sData, 'd');
   float *num = (float *)scano('f');
   printf("Half: %f", *(num)/2);

    return 0;
}

void *scano(char mode){

    char sData[20];
    fgets(sData, 20, stdin);
    *(sData + strlen(sData) - 1) = '\0'; //get rid of the '\n' before the '\0'
   switch(mode){
       case 'd':{
           int *dataI = (int *)malloc(sizeof(int));
           *dataI = atoi(sData);
           return dataI;
       }
       case 'f':{
           float *dataF = (float *)malloc(sizeof(float));
           *dataF = atof(sData);
           return dataF;
       }
       case 'D':{
           //double...
       }
   }

}

The function is of type void * because that's the only convenient way to have it return different data types.

The function is obviously unfinished for other data types, but I have some questions first:

How can the algorithm of the function be improved?
Don't I need to free() in every case? I'm aware allocated memory needs to be freed, but when working with lists, free() was only used to delete Nodes, when creating Nodes, no free() was called after a malloc().
Is it fully safe? If not, how can it be made safe?

Q: How can the algorithm of the function be improved?

A: I'm not sure what you mean by algorithm. There are several things relating to functionality that you should improve on, however:

Make an enum to represent what modes are accepted by your function, and use that instead of taking a char. This will greatly reduce the potential for bugs and makes you less reliant on magic "numbers" (or characters, if you will).
You might want to remove the biggest drawback your function has compared to scanf and friends, which is lack of flexibility. The great strength of scanf is that its format string is extremely variable and can be used to extract data from a vast variety of formats. You approach currently limits you to a single char worth of format specifier, and is thus much more rigid and less adaptable. Depending on what you want from your function, this might be acceptable or even desired, but if you really want to contest with scanf, you should come up with a good idea of two on how to improve flexibility.
Remove the hard limit on length. Why do you arbitrarily only read 20 chars? This is just barely sufficient for 64bit integers, but will quickly prove as too little as soon as you start reading different types of data (especially strings).
Add some error reporting facility to your function. What if you tried to read an int when there was none? You might want to tell your users about this (actually, you absolutely must tell your users about this, or your user interface is quickly going to take a turn into an unpleasant direction).
You state in you question that gets_s is useless for streams different from stdin, but that criticism also applies to your function. Consider taking a stream handle as your first argument and reading from that instead. You can still provide another function which has stdin implied, for convenience's sake.

Q: Don't I need to free() in every case?

A: Yes, you absolutely do. Every buffer that allocated via malloc and friends should be freed at some point, otherwise you've got yourself a memory leak. Actually, you might not even want to allocate things on the heap at all. If all I want to do is read a single integer, I usually don't want to incur the cost of calling out to the OS to reserve some memory for me, when I could just read the value into a local variable. How do you do this? Easy enough, instead of taking care of allocation yourself, hand that task over to the user and just accept a pointer instead, i.e.

void scano(char mode, void* buffer);

This has the added benefit of opening up the return value slot, so now you can actually use that to indicate whether an error occured:

int scano(char mode, void* buffer);

or even better

err_code scano(char mode, void* buffer);

where err_code is an enum containing all the possible error values. This pattern of returning an error indicator is really common throughout the standard library and most C applications in general.

Q: Is it fully safe? If not, how can it be made safe?

A: That depends on your definition of "fully safe". If you mean "has no undefined behavior", then the answer is no, sadly. atoi causes such undefined behavior in the case that the integer it is trying to parse is out of range for the int type. The same is true for atof, so you would need to brace against those cases (by not using those two functions, usually). Another problem is that you don't check for malloc failure - if allocation fails, you get a null pointer and subsequently try to write the result of atoi or atof to it, which also causes undefined behavior. Another point of insecurity is your call to fgets: If reading fails completely, your array remains uninitialized. The subsequent call to strlen is then undefined behavior.

Other things that aren't quite right

You don't need to cast the return value of malloc, or any other function returning void*. In fact, this can lead to subtle bugs when making changes to your code later on.
Don't rely on magic numbers. Instead of typing fgets(sData, 20, stdin), pull that 20 out to its own constant, or, at the very least, do something like fgets(sData, sizeof buf, stdin). That way, you're way less prone to buffer overflow bugs arising from you forgetting to change one of the two 20s when you go back and edit your code.
*(sData + strlen(sData) - 1) = '\0'; //get rid of the '\n' before the '\0' ...and now you've overwritten part of your data! There might not actually be an '\n' there, if the line you are reading from has still 19 or more characters left. Check for those kinds of things before writing into your string!


C Garbage Heap aka simplest memory management tool The idea is to init garbage heap in the beginning of main, use gmalloc, gcalloc and grealloc wrappers and free all allocated memory in the end of main. It helps to avoid memory leaks when writing programs for ejudge systems. Would like to hear general feedback on the idea and advices on how to improve the code (beginner in C).

main.c

/*
Example of usage of garbage heap.
No free() call but no memory leak.
*/

#include "garbage_heap.h"

int main() {
    init_garbage_heap();
    for (int i = 0; i < 1000; ++i)
    {
        char* str = gmalloc(1000*sizeof(char));
        str = grealloc(str, 2000*sizeof(char));
    }
    free_garbage_heap();
    return 0;
}

garbage_heap.h

#ifndef GARBAGE_HEAP_H
#define GARBAGE_HEAP_H

#include <stddef.h>

void init_garbage_heap (void);
void free_garbage_heap (void);
void* gmalloc (size_t sizemem);
void* gcalloc (size_t number, size_t size);
void* grealloc (void* ptrmem, size_t sizemem);

#endif // GARBAGE_HEAP_H

garbage_heap.c

#include <stdlib.h>

#include "garbage_heap.h"

typedef struct
{
    void** buffer;
    int len;
    int capacity;
}
GarbageHeap;

static GarbageHeap garbage_heap;

void init_garbage_heap(void)
{
    int len = 32;
    garbage_heap.len = 0;
    garbage_heap.capacity = len;
    garbage_heap.buffer = (void**)malloc(sizeof(void*)*len);
    for (int i = 0; i < len; ++i)
        garbage_heap.buffer[i] = NULL;
}

void free_garbage_heap(void)
{
    for (int i = 0; i < garbage_heap.len; ++i)
        if (garbage_heap.buffer[i] != NULL)
            free(garbage_heap.buffer[i]);
    free(garbage_heap.buffer);
}

void resize_garbage_heap(void)
{
    garbage_heap.capacity *= 2;
    garbage_heap.buffer = (void**)realloc(garbage_heap.buffer, sizeof(void*) * garbage_heap.capacity);
    for (int i = garbage_heap.len; i < garbage_heap.capacity; ++i)
        garbage_heap.buffer[i] = NULL;
}

void* gmalloc(size_t sizemem)
{
    if (garbage_heap.len == garbage_heap.capacity)
        resize_garbage_heap();
    garbage_heap.buffer[garbage_heap.len] = malloc(sizemem);
    ++garbage_heap.len;
    return garbage_heap.buffer[garbage_heap.len - 1];
}

void* gcalloc(size_t number, size_t size)
{
    if (garbage_heap.len == garbage_heap.capacity)
        resize_garbage_heap();
    garbage_heap.buffer[garbage_heap.len] = calloc(number, size);
    ++garbage_heap.len;
    return garbage_heap.buffer[garbage_heap.len - 1];
}

void* grealloc(void* ptrmem, size_t sizemem)
{
    if (ptrmem == NULL)
        return gmalloc(sizemem);
    int i = 0;
    while (garbage_heap.buffer[i] != ptrmem)
        ++i;
    void* tmp_ptr = realloc(ptrmem, sizemem);
    if (tmp_ptr != NULL)
        garbage_heap.buffer[i] = tmp_ptr;
    return tmp_ptr;
}

Interesting concept, I'm going to guess that C is not your first programming language, and your first language did not require explicit memory management.

The code is pretty good, but here are some things to consider:

A Good Programming Practice
In most of the if statements and loops the code in the question is hard to maintain because it doesn't provide complex statements for each path through the code. A complex statement is a code block encapsulated in the C language by { and }.

An example of a complex statement is:
        {
            STATEMENT;
            STATEMENT;
        }

An example of the code lacking this good programming practice is

void free_garbage_heap(void)
{
    for (int i = 0; i < garbage_heap.len; ++i)
        if (garbage_heap.buffer[i] != NULL)
            free(garbage_heap.buffer[i]);
    free(garbage_heap.buffer);
}

Let's say that someone maintaining the code has to add a statement to the if statement in the loop and they are looking to do it quickly or they come from a programming language that is positional rather than using braces to denote a complex statement. If they add it immediately below the free(garbage_heap.buffer[i]); without adding the braces, not only is the new statement not in the if statement, the new statement isn't even in the for loop.

In languages such as C and C++ the following code is a safer programming practice:

void free_garbage_heap(void)
{
    for (int i = 0; i < garbage_heap.len; ++i)
    {
        if (garbage_heap.buffer[i] != NULL)
        {
            free(garbage_heap.buffer[i]);
        }
    }
    free(garbage_heap.buffer);
}

Where to add a new statement becomes much clearer.

Possible Improvements for the Function init_garbage_heap
The first thing to note in init_garbage_heap() is that there is no error checking after the call to void* malloc(int number_of_bytes_to_allocate). The call to malloc() may fail, and if it does the value returned is NULL. If the code continues to process if malloc() returned an error, then there would be a memory access error in the for loop for garbage_heap.buffer[i] = NULL; that will probably cause the program to crash (exit improperly). In C++ if new() fails it throws an exception, the C programming language does not have exceptions so the test is required.

The second thing to note in the following code is that the cast to (void**) is not necessary in C99 or later versions of C, in the original version of C malloc() return char* and required casting, now that it returns void* it does not.

void init_garbage_heap(void)
{
    int len = 32;
    garbage_heap.len = 0;
    garbage_heap.capacity = len;
    garbage_heap.buffer = (void**)malloc(sizeof(void*)*len);
    for (int i = 0; i < len; ++i)
        garbage_heap.buffer[i] = NULL;
}

The third thing to note is that the type of garbage_heap.buffer can change so it might be better if sizeof(*garbage_heap.buffer) * len was used rather than explicity stating sizeof(void*)*len. That way if the type of buffer changes the code only needs to change where buffer is declared rather than in multiple places.

The for loop to initialize the contents of garbage_heap.buffer to NULL might be replaced with a call to void * memset ( void * ptr, int value, size_t num );:

    memset(garbage_heap.buffer, 0, sizeof(*garbage_heap.buffer) * len);

This may possibly perform better speed wise than the for loop depending on how memset() is written. It is also written in the terms of the rest of the code.

If the call to malloc() is replaced by a call to calloc() neither the for loop or a call to memset() is required because calloc() will set the contents of the memory being returned to NULL.

Possibly improved version of `init_garbage_heap(void):

void init_garbage_heap(void)
{
    int len = 32;

    garbage_heap.len = 0;
    garbage_heap.capacity = len;
    garbage_heap.buffer = calloc(sizeof(*garbage_heap.buffer), len);
    if (!garbage_heap.buffer)
    {
        fprintf(stderr, "Unable to allocate memory for garbage_heap.buffer, exiting program");
        exit(EXIT_FAILURE);
    }
}

It is not clear why len is being assigned 32, it might be better to use a symbolic constant instead of 32. It might also be nice if the user of these routines could set the initial allocation capacity. It would be better if the initial buffer size was larger than 32 to reduce the need for resizing.

Spacing
The code in garbage_heap.c might be more readable if there was vertical spacing inside the functions.


Multiplication Grid function I've made a small function to output a multiplication grid (those things from primary school! :P) to an inputted number.

def make_grid(max):
    max += 1  # so the for loops go up to the desired number, inclusive
    print('X\t' + '\t'.join([str(_) for _ in range(max)]) + '\n')  # print all the numbers hoizontally
    for i in range(max):
        print(i, end="\t")  # print the vertical column of base numbers
        for x in range(max):
            print(str(i*x), end="\t")  # multiply the two numbers
        print('\n')

if __name__ == "__main__":
    try:
        max = int(input("Max Number? "))
        make_grid(max)
    except ValueError:
        print("Please enter a number!")

Choosing a max number of 5 (make_grid(5)) would output:

X 0   1   2   3   4   5

0 0   0   0   0   0   0

1 0   1   2   3   4   5

2 0   2   4   6   8   10

3 0   3   6   9   12  15

4 0   4   8   12  16  20

5 0   5   10  15  20  25

I'm looking for any improvements I could make, especially the \t's - as I use them to line up the numbers, but the gap gets pretty big, unnecessarily.

make_grid() isn't quite the right name for this function, as it is printing a grid rather than returning a grid.  I think you would be better off returning a long string to be printed.

In your list comprehension, _ is a bad choice for the iteration variable name, since _ indicates a throwaway value, by convention.  If you then call str(_), you should pick a different name.

Besides the excessively wide spacing as you have observed, using tabs is problematic because the output would be better with the entries right-aligned.  You also have a superfluous tab at the end of each line (except the header row).

Suggested solution

Use '{0:>{width}}'.format(…) to print the entries right-aligned with a fixed width.

def multiplication_grid(max):
    max += 1    # Inclusive upper bound for ranges
    grid = [['X'] + [j for j in range(max)]] + \
             [[i] + [i * j for j in range(max)] for i in range(max)]

    # Use fixed-width right alignment for every entry in the 2D grid
    width = 2 + len(str(max * max))
    fmt = lambda n: '{0:>{width}}'.format(n, width=width)
    return '\n\n'.join(''.join(fmt(entry) for entry in row) for row in grid)

[['X'] + [j for j in range(max)]] creates the header row.

[[i] + [i * j for j in range(max)] for i in range(max)] is a nested list comprehension to create the rest of the rows, where i is the row number and j is the column number.

lambda is a way to define very simple functions that do nothing more than returning an expression.  The lambda could also have been written as:

    def fmt(n):
        return '{0:>{width}}'.format(n, width=width)

The last line applies fmt() to every entry in the two-dimensional grid and joins everything into a single string.


F# method to insert, update, or delete events in a dictionary I have the following code:

    member this.ProcessEvent(sourceStream: OrderResponse) =
       match sourceStream.Action with
        | Action.Partial
        | Action.Insert ->
            sourceStream.Data
            |> Seq.iter (fun x -> this.orderState.[x.OrderId] <- this.ConvertFullOrder x)
        | Action.Update ->
            sourceStream.Data
            |> Seq.iter (fun x -> this.orderState.[x.OrderId] <- this.UpdateOrder x this.orderState.[x.OrderId])
        | Action.Delete ->
            sourceStream.Data
            |> Seq.iter (fun x -> this.orderState.Remove(x.OrderId) |> ignore)
        | _ -> ()

It is processing a list of events and an action. The data is processed and stored in a dictionary. The action tells to insert, update or delete some of the records and applies to all the events received in the same message.

For each case, I iterate through the sourceStream.Data list.

Is there a better / more readable way where I can declare I am going to process that list and then, based on the Action, have different code? this would allow to remove the multiple sourceStream.Data |> bits and specify it only once.

You have probably found a good solution long time ago, but here is my suggestion:
member this.ProcessEvent(sourceStream: OrderResponse) =
    let handler: (Order -> unit) =
        match sourceStream.Action with
        | Action.Partial
        | Action.Insert -> (fun x -> this.orderState.[x.OrderId] <- this.ConvertFullOrder x)
        | Action.Update -> (fun x -> this.orderState.[x.OrderId] <- this.UpdateOrder x this.orderState.[x.OrderId])
        | Action.Delete -> (fun x -> this.orderState.Remove(x.OrderId) |> ignore)
        | _ -> (fun x -> ())

    sourceStream.Data |> Seq.iter handler

I'm not sure. what type x has, so for the illustration I just call it Order in the handler definition.