How to refactoring function when I've to do many things in one function I always want to improve my code by refactoring but when I've to work with complicated function that have many thing to do . How can I refactoring them.

Here is my data for example I get data from request

My DB structure look like this

 DB
    id  PK
    blog int
    date_end string
    date_start string
    employee_id FK [ by table employee ]
    total_time_in_hr int
    value int

//my data
req = {
            data: [
                {
                    blog: 1,
                    date_end: "19-02-2020",
                    date_start: "19-02-2020",
                    employee_id: [2290,3],
                    work_id: 1,
                    total_time_in_hr: 10,
                    value: 5100
                }
            ]
    };

First I've to loop my data then I've to loop again in my employee_id and insert them into DB

//looop data
async store({ request, response }) {
    const data = req.data
    await Promise.all(
        data.map(async (getData, i) => {
            const empId = getData.employee_id;
            const workId = getData.work_id;
            let date_start = getData.date_start;
            let date_end = getData.date_end;
            const value = getData.value;
            date_start = moment(date_start, "DD-MM-YYYY").format(
                "YYYY-MM-DD"
            );
            date_start = moment(date_end, "DD-MM-YYYY").format(
                "YYYY-MM-DD"
            );
            await Promise.all(
                empId.map(async (empId, i) => {
//insert to my DB
                    const storeData = new MyDB();
                    storeData.employee_id = empId;
                    storeData.work_id = workId;
                    storeData.value = value;
                    storeData.date_start = date_start;
                    storeData.date_end = date_end;
                    storeData.status = 3;
                    storeData.group_name = groupName;
                    await storeData.save();
// insert to my Log DB
                    const createLog = new MyDBLog();
                    createLog.my_db_id = storeData.id;
                    createLog.value = value;
                    createLog.msg = 'somthing';
                    createLog.status = 3;
                    createLog.total_time = 0;
                    await createLog.save();

                    //this part in my work I've to make json and insert it to another DB
                    const data = {
                        work_id: workId,
                        blog: getData.blog,
                        total_time: getData.total_time_in_hr,
                        my_db_id: storeData.id
                    };
                    const store = await AnotherDB.create(data);
                })
            );
            return { workId, value };
        })
    ).then(async data => {
//update last Value
        await Promise.all(
            data.map(async ({ workId, value }) => {
                const updateTotalAssign = await AssignWork.query()
                    .where("work_id", workId)
                    .update({ total_value: value });
            })
        );
    });
    return response.status(200).json({
        status: 200,
        success: true,
        message: "success"
    });
}

As you see my code are very mess up How to refactoring function and clean code this kind of function

A low-hanging fruit will be defining some interfaces of datastores such as MyDB so that some details can be hidden from the controller code. For instance:

 const storeData = new MyDB();
 storeData.employee_id = empId;
 storeData.work_id = workId;
 storeData.value = value;
 storeData.date_start = date_start;
 storeData.date_end = date_end;
 storeData.status = 3;
 storeData.group_name = groupName;
 await storeData.save();

can be refactored to something like:

 MyDBBuilder
     .with(getData)
     .withStatus(3)
     .withGroupName(groupName)
     .save();

The complexity comes from the fact that it takes 4 steps to complete this business transaction. Unless there is a way to redefine data schema, all we can do here is to encapsulate the complexity and not expose it in the controller code.
In some cases, it might be appropriate to have all the db operations in some stored procedure, if you are not concerned with portability.


A way to handle integer input I'm working on simple code that is to read input where:

The first line is a number of cases to consider.
The next lines are integers separated by a space that will be tested lately.

Possible input:

2
3 4
4 5

Possible output (just some multiplication example):

12
20

Questions:

I would like to know if there are 'better' ways to handle this kind of input.
What could be done to make this code more universal? For example, there is a variable int lineLength=3 and I have to modify It for other similar input (where lineLength is bigger):

2
3 4 5
4 5 6

What validation should be implemented (if someone gives bad input, maybe it could be done in X ways (the handling))?
Do I break any good-programming rules? (I intended to write this only in class Main)

import java.io.IOException;
import java.util.Scanner;
import java.util.Arrays;

public class Main {

public static void main(String[] args) throws IOException {

/** Reading number of tests (NO VALIDATION) **/
/** nextInt() doesnt read the last newline character of input **/
Scanner read = new Scanner(System.in);
int numberOfTests = read.nextInt();

/** Consuming newline character**/
read.nextLine();

String line[] = new String[numberOfTests];

/** Reading line excluding line separator at the end (NO VALIDATION) **/
for (int i = 0; i < numberOfTests; i++)
    line[i] = read.nextLine();

/** Printing array of String(lines) **/
System.out.print(Arrays.toString(line));

/** Creating the array of ints (NO VALIDATION) **/
int lineLength = 3;
int num[][] = new int[numberOfTests][lineLength];

/** Extracting integers from line **/
for (int i = 0; i < numberOfTests; i++) {
    int k = 0;
    for (int j = 0; j < lineLength; j++) {
        if (Character.isSpaceChar((line[i].charAt(j))) != true) {
            num[i][k++] = Character.getNumericValue(line[i].charAt(j));
        }
    }
}

/** Printing array of integers to test **/
int numToTests = 2;
for (int i = 0; i < numberOfTests; i++) {
    System.out.println();
    for (int j = 0; j < numToTests; j++) {
        System.out.print(num[i][j]+" ");
    }
}
}
}

Behaviour

I assume that you are writing this code in preparation for some programming challenges involving an online judge.  In that case, it's unconventional that you are producing a matrix of digits.  That is, each element is a single digit, no negative numbers, and no numbers larger than 9.

Usually, each line represents an independent test case.  You should be able to compute a result for each line of input without waiting for all of the input to be read first.

Abstraction

If you want to focus on input-handling as a problem to be solved, then the "proper" solution would be to provide a convenient way for your computation code to fetch its input.  The goal would be to enable simple code like:

public class Multiply {
    public static int product(int[] numbers) {
        int product = 1;
        for (int n : numbers) {
            product *= n;
        }
        return product;
    }

    public static void main(String[] args) {
        IntTestCaseInput input = new IntTestCaseInput(System.in);
        for (int[] multProblem : input) {
            System.out.println(product(multProblem));
        }
    }
}

Here is a class that could achieve that:

import java.io.*;
import java.util.*;

public class IntTestCaseInput implements Iterable<int[]> {
    private class Iter implements Iterator<int[]> {
        public int cases = -1;

        @Override
        public void remove() {
            throw new UnsupportedOperationException();
        }

        @Override
        public boolean hasNext() {
            try {
                maybeInitialize();
                return cases > 0;
            } catch (IOException e) {
                return false;
            }
        }

        @Override
        public int[] next() {
            try {
                maybeInitialize();
                if (cases == 0) {
                    throw new NoSuchElementException();
                }
                cases--;

                String[] strings = IntTestCaseInput.this.in.readLine().split("\\s+");
                int[] numbers = new int[strings.length];
                for (int i = 0; i < strings.length; i++) {
                    numbers[i] = Integer.parseInt(strings[i]);
                }
                return numbers;
            } catch (IOException e) {
                throw new NoSuchElementException(e.getMessage());
            }
        }

        /**
         * On first use, find out how many test cases to expect.
         */
        private void maybeInitialize() throws IOException {
            if (cases < 0) {
                cases = Integer.parseInt(IntTestCaseInput.this.in.readLine());
            }
        }
    }

    private BufferedReader in;

    public IntTestCaseInput(InputStream in) {
        this(new InputStreamReader(in));
    }

    public IntTestCaseInput(Reader in) {
        this.in = (in instanceof BufferedReader) ?
            (BufferedReader)in : new BufferedReader(in);
    }

    @Override
    public Iterator<int[]> iterator() {
        return new Iter();
    }
}

In total, this results in a lot more code than your in your original question.  However, once the calculations start getting more complicated, you may find that getting the input routines out of the way is worthwhile.


Decode Cæsar Cypher by Checking all Keyspaces This function produces a decoded string, decoded, for all keyspaces against a given cæsar cipher ciphertext. Punctuation is not changed and string.upper() is used to make working with ord() easier.

I am looking for a more Pythonic way to work with my decoded variable. I keep finding myself assigning empty strings or None or 0 to use as a placeholder and I think it looks ugly. I'd also like to know if there is a better way to avoid writing c = ord(c).

def testCaesar(ciphertext):
    for testkey in range(0,25):
        decoded = ''   # <--- uglyness
        for c in ciphertext.upper():
            if not c.isalpha():
                decoded += c
                continue
            c = ord(c) # this is here to improve readability of line below. Is there a better way?
            decoded += chr(c + testkey if c + testkey < 90 else c - testkey)
        print('{0} : Rot{1} Caesar').format(decoded, str(testkey))
        checkMatch(decoded, testkey, 'Caesar')

You could use else instead of continue to mark the 2 potential results of your test. You get something like :

def testCaesar(ciphertext):
    for testkey in range(0,25):
        decoded = ''   # <--- uglyness
        for c in ciphertext.upper():
            if not c.isalpha():
                decoded += c
            else:
                c = ord(c) # this is here to improve readability of line below. Is there a better way?
                decoded += chr(c + testkey if c + testkey < 90 else c - testkey)
        print('{0} : Rot{1} Caesar').format(decoded, str(testkey))
        checkMatch(decoded, testkey, 'Caesar')

Now, you've got some duplicated logic you could try to remove. You could use a new variable like :

def testCaesar(ciphertext):
    for testkey in range(0,25):
        decoded = ''   # <--- uglyness
        for c in ciphertext.upper():
            if not c.isalpha():
                decoded_c = c
            else:
                c = ord(c) # this is here to improve readability of line below. Is there a better way?
                decoded_c = chr(c + testkey if c + testkey < 90 else c - testkey)
            decoded += decoded_c
        print('{0} : Rot{1} Caesar').format(decoded, str(testkey))
        checkMatch(decoded, testkey, 'Caesar')

Or you could define a function for this :

def decode_char(c, key):
    if not c.isalpha():
        return c
    else:
        c = ord(c) # this is here to improve readability of line below. Is there a better way?
        return chr(c + testkey if c + testkey < 90 else c - testkey)

def testCaesar(ciphertext):
    for testkey in range(0,25):
        decoded = ''   # <--- uglyness
        for c in ciphertext.upper():
            decoded += decode_char(c, testkey)
        print('{0} : Rot{1} Caesar').format(decoded, str(testkey))
        checkMatch(decoded, testkey, 'Caesar')

Now, Python has a style guide called PEP8 and it says :

  Do not rely on CPython's efficient implementation of in-place string
  concatenation for statements in the form a += b or a = a + b . This
  optimization is fragile even in CPython (it only works for some types)
  and isn't present at all in implementations that don't use
  refcounting. In performance sensitive parts of the library, the
  ''.join() form should be used instead. This will ensure that
  concatenation occurs in linear time across various implementations.

This can be easily done now :

decoded = ''.join(decode_char(c, testkey) for c in ciphertext.upper())

Also, a tiny optimisation could be to call upper() once and only once.


Sorting an array of strings using pointers (followup) This is a follow-up to this post.

Things I've changed:

The number of strings is checked.
I'm using pointers now.

I'll use fgets in more mission critical situations. For a lab assignment, the scanf approach will suffice I hope.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#define LEN 100
#define WID 80

void sort(char *s[], int n);

void display(char *s[], int n);

int main()
{
    int n, i;
    char *s[LEN];
    printf("Enter number of strings : ");
    scanf("%d", &n);
    printf("Enter strings one by one :\n");
    if (n > LEN) {
        printf("Sorry, maximum strings allowed is %d. Defaulting.", LEN);
        n = LEN;
    }
    for (i = 0; i < n; i++) {
        printf("%d : ", i + 1);
        s[i] = (char *) malloc(WID * sizeof(char));
        scanf(" %s", s[i]);
    }

    printf("The initial elements are:\n");
    display(s, n);
    sort(s, n);
    printf("The elements after sorting are:\n");
    display(s, n);

    return 0;
}

void sort(char *s[], int n)
{
    char *temp;
    int item, i;

    for (item = 1; item < n; item++) {
        temp = s[item];
        for (i = item; i > 0 && strcmpi(s[i - 1], temp) > 0; i--);
        memcpy(&s[i + 1], &s[i], (item - i) * sizeof(char *));
        s[i] = temp;
    }
}

void display(char *s[], int n)
{
    int i;
    printf("\n\n");
    for (i = 0; i < n; i++) {
        printf("%s ", s[i]);
    }
    printf("\n\n");
}

Any alternative or more elegant approach is welcome.

Advice 1

I suggest that you add the { and } to loop/conditional block even in the case it's a one-liner. The reason is if someone maintains your code and need to add a statement or a couple, they do not need to type those.

Advice 2

The display may print a more friendly text:

void display(char *s[], int n)
{
    int i;
    printf("\n\n[");
    char* separator = "";
    for (i = 0; i < n; i++) {
        printf("%s%s", separator, s[i]);
        separator = ", ";
    }
    printf("]\n\n");
}

For example: [abc, bb, def].

Advice 3

If you need to sort large array of strings, there are more efficient algorithms especially for sorting strings. See this. Also, I believe radix sort would perform good as well.

Hope that helps.


Automatically generate a thunderbird importable contacts file for all employees This is implemented in Ms Access 2003. The Table "People" is a ODBC linked database table on a MSSQL 2008 server. The file generated can be imported into Mozilla Thunderbird contacts to have a contact list of everyone in the company ready.

The entire thing is written under option explicit. This is mandated.

Private Sub GenerateContacts_Click()
'Automated Generation of an Importable .csv file for Mozilla Thunderbird contacts

'Variable Definitions
Dim fso As Object                   'File System Object
Dim oFile As Object                 'File Object
Dim path As String                  'User selected Path (Uneditable Textbox with "set path" button
Dim currentLine As String           'current Line to be written to file
Dim rs As Recordset                 'Recordset containing the data
Dim bigDelim As String              ' ",,,,,,,,,,,,,,,,,,,,,,,,,," Thunderbird CSV Format delimiter
Dim db As Database                  'Database object for query execution
Dim SQL As String                   'SQL Query to select the Users
Dim tel As String                   'Landline phone of the employee
Dim telMobi As String               'Mobile phone of the employee
Dim Email As String                 'Email adress of the employee
Dim Mng As Integer                  'Number of records to be processed
Dim pathDate As String              'Datediff to current date to append to filename to make files unique
Dim Name() As String               'Name of the employee

'Initialize the database
Set db = DBEngine(0)(0)

'Confirmation dialog
If Not MsgBox("Do you want to export all available contacts to thunderbird?", vbYesNo) = vbYes Then
  Exit Sub
End If
If Not Me![Outputpfad] <> "" Then
  MsgBox ("[Error] No output path selected.")
  Exit Sub
End If

SQL = "SELECT People.Name, People.Email, People.Telefon_int, People.Telefon_mobint FROM People WHERE People.Email <> '' AND People.Name NOT LIKE 'zz_%' ORDER BY People.Eintrag DESC"
Set rs = db.OpenRecordset(SQL, dbOpenDynaset, dbSeeChanges)
bigDelim = ",,,,,,,,,,,,,,,,,,,,,,,,,," 'Necessary for thunderbird .csv import format
pathDate = CStr(DateDiff("s", #1/1/1970#, Now()))
path = Me![Outputpfad] & "\"
path = path & pathDate & "-Company-Email-Contacts-Thunderbird.csv"

Set fso = CreateObject("Scripting.FileSystemObject")
Set oFile = fso.CreateTextFile(path)
oFile.WriteLine "Vorname,Nachname,Anzeigename,Spitzname,Primäre E-Mail-Adresse,Sekundäre E-Mail-Adresse,Messenger-Name,Tel. dienstlich,Tel. privat,Fax-Nummer,Pager-Nummer,Mobil-Tel.-Nr.,Privat: Adresse,Privat: Adresse 2,Privat: Ort,Privat: Bundesland,Privat: PLZ,Privat: Land,Dienstlich: Adresse,Dienstlich: Adresse 2,Dienstlich: Ort,Dienstlich: Bundesland,Dienstlich: PLZ,Dienstlich: Land,Arbeitstitel,Abteilung,Organisation,Webseite 1,Webseite 2,Geburtsjahr,Geburtsmonat,Geburtstag,Benutzerdef. 1,Benutzerdef. 2,Benutzerdef. 3,Benutzerdef. 4,Notizen,"

rs.MoveFirst
Do While Not rs.EOF
    Email = rs![Email]
    Name = Split(rs![Name], ",")
    If rs![Telefon_int] <> "" Then
        tel = rs![Telefon_int]
    End If
    If rs![Telefon_mobint] <> "" Then
        telMobi = rs![Telefon_mobint]
    End If
    currentLine = Name(1) & "," & Name(0) & "," & Name(1) & " " & Name(0) & ",," & Email & ",,," & tel & ",,,," & telMobi & bigDelim
    oFile.WriteLine (currentLine)
    tel = ""
    telMobi = ""
    rs.MoveNext
Loop
oFile.Close
rs.Close
Set fso = Nothing
Set oFile = Nothing
MsgBox ("File successfully generated at path:  " & path)
End Sub

As far as I can tell, it works, with any subset or superset of the database records. I tested it against a employee table with 4000 entries for performance, and didn't notice a slowdown. Our actual production table is about 400 entries big. People whose names are starting with "zz_" have left the company (I didn't make that design choice and it's not changeable by me).

Variables

I like that you're using Option Explicit. It helps prevents "easy" mistakes from typos. I don't like that you declare all your variables in a block and I somewhat dislike the naming scheme you use:

I also used to declare all variables in a block at the start of a procedure. It's significantly easier to declare them as close as possible to their usage, though. It especially allows you to limit their scope (preventing name-conflicts and inappropriate reuse), and it lowers the "cognitive load" because you don't need to keep in mind which variables exist and which type they have.

Another thing I noticed is that you're (possibly unintentionally) mix German into your variable names: tel and telMobi would probably be declared as phone and mobilePhone by a native speaker.

Additionally the casing is inconsistent. SQL could be sqlQuery, Email should just be email and Name is an array, which should accordingly be names.

Furthermore you use some hungarian notation (oFile), but don't do that consistently (as for oFso, oRs, oDb). Since you specifically mark objects (that need to be assigned to with Set), you should mark all objects the same way. Alternatively you can grab the Rubberduck VBE Addin (which I am involved in maintaining), that has an inspection warning you when you assign to an Object-typed variable without the Set keyword making the hungarian notation useless.

Last but not least as far as I can tell bigDelim will never change. I suggest you move it from a Sub-scoped variable into a module-scoped constant.

Responsibilities

Currently this sub does a few things:

Prompt the user for confirmation
Validate the provided input (namely the output path)
Run a SQL-Query against a database
Write the Query results into a file
(output a success message)

Each of these steps could probably be extracted into it's own method. This allows us to break down the procedure into "manageable" steps, reducing cognitive load and simplifying maintenance.

Semantics:

Some minor semantic issues I think are worth fixing:

You're late-binding the Scripting.FileSystemObject. I think that is a bad decision, because it means you can only declare fso as Object and accordingly lose all the help that the VBE can provide you with (autocomplete). That type should be available on all systems that this code will ever run on. No need to late-bind this.
the Name() you're using is a little bit messy. I'd prefer to have a firstName and a lastName to clarify usage. It should be possible to assign them as follows:

Dim split() As String
firstName = split(1)
lastName = split(2)

Setting objects to Nothing after usage is the same as setting Objects in java to null after usage. In my experience it's more of a hassle than the benefits you gain from it. When the object references go out of scope they're collected away anyways. I'd leave that part out :)


Is it ok to add a static method to the IEqualityComparer implementation? Linq methods like Distinct need IEqualityComparer object. But sometimes I just compares two objects and to create Comparer instance for that is seems redundant. So it would be nice to use static Equals method. But I'm not sure does not it break some guidelines.

public class ByteArrayComparer : EqualityComparer<byte[]>
{
    [DllImport("msvcrt.dll", CallingConvention = CallingConvention.Cdecl)]
    private static extern int memcmp(byte[] b1, byte[] b2, long count);

    //http://stackoverflow.com/a/1445405/2336304
    public override bool Equals(byte[] b1, byte[] b2)
    {
        return AreEqual(b1, b2);
    }

    // http://stackoverflow.com/questions/263400/what-is-the-best-algorithm-for-an-overridden-system-object-gethashcode/263416#263416
    // http://stackoverflow.com/a/7244729/2336304
    public override int GetHashCode(byte[] array)
    {
        unchecked
        {
            return array == null ? 0
                  : array.Aggregate(17, (current, element) => current * 31 + element.GetHashCode());
        }
    }

    public static bool AreEqual(byte[] b1, byte[] b2)
    {
        // Validate buffers are the same length.
        // This also ensures that the count does not exceed the length of either buffer.
        return b1 != null && b2!= null && b1.Length == b2.Length && memcmp(b1, b2, b1.Length) == 0;
    }
}

The AreEqual() does not do what one would expect.

As you have implemented the method, you say null != null.
GetHashCode()
You should consider replacing the tenary with a simple if statement. IMHO you would gain in readability.

public override int GetHashCode(byte[] array)
{
    unchecked
    {
        if (array == null) { return 0; }

        return array.Aggregate(17, (current, element) => current * 31 + element.GetHashCode());
    }
}

using a static method here, won't do any harm IMHO.


Extract specific positions from a char array I have two arrays:

char input[] = "ughIuytLikeretC";

and

bool mask[] = {
    false, false, false, true, false,
    false, false, true, true, true,
    true, false, false, false, true,
};

My function takes these two arrays and returns the characters in input whose positions are true in mask such that in this example, the result being ILikeC.

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

char *filtArray(char input[], bool mask[], char *filtered) {
    int j = 0;
    int i;
    for (i = 0; input[i]; i++) {
        filtered[j] = input[i];
        j += mask[i];
        }

    filtered[j] = 0;
    return filtered;
}

filtArray will run on billions of "input" strings of constant length and "mask" will be the same for all "input"s.

I see some things that may help you improve your code.

Provide complete code to reviewers

This is not so much a change to the code as a change in how you present it to other people.  Without the full context of the code and an example of how to use it, it takes more effort for other people to understand your code.  This affects not only code reviews, but also maintenance of the code in the future, by you or by others.  One good way to address that is by the use of comments.  Another good technique is to include test code showing how your code is intended to be used.  Here's the test main I used for your code:

int main() {
    const char *input[2] = {
        "ughIuytLikeretC",
        "xxxExxxdwarxxxd",
    };
    const bool mask[] = {
        false, false, false, true, false,
        false, false, true, true, true,
        true, false, false, false, true,
    };
    char filt[100];
    char maskstr[100];
    // create the mask string
    pmask(mask, maskstr);

    printf("Orig: %s\nMask: %s\nFilt: %s\n", input[0], maskstr, filtArray(input[0], mask, filt));
    printf("Orig: %s\nMask: %s\nFilt: %s\n", input[1], maskstr, filtArray(input[1], mask, filt));
    for (int i = 0; i < 10000000; ++i) {
        int n = rand() > RAND_MAX/2 ? 1 : 0;
        printf("Orig: %s\nMask: %s\nFilt: %s\n", input[n], maskstr, filtArray(input[n], mask, filt));
    }
}

After it applies the function to two strings, it then iterates 10 million times, choosing one or the other test inputs randomly.  This is for testing timing.

Use const where practical

The filtArray function does not (and should not) alter either the passed input or mask arrays and so both of those should be declared const.

char *filtArray(const char input[], const bool mask[], char *filtered) {

Consider bounds checking

If the input strings have already been validated for length, the function you have is OK, but in general, it's good to make sure there is enough room to copy the masked characters.  If there isn't enough room, that's the recipe for a buffer overflow vulnerability and must be eliminated, either by the calling routine or by this one.

Consider a custom copy

If the same mask is used for billions of strings, it would probably make sense to do things differently.  For example, one alternative might look like this:

#include <string.h>

char *filtArray(const char input[], char *filtered) {
    memcpy(&filtered[1], &input[7], 4);
    filtered[0] = input[3];
    filtered[5] = input[14];
    filtered[6] = '\0';
    return filtered;
}

Note that the mask is no longer used in this version, because the code has implemented it implicitly.  This is less flexible but offers better performance.  For 10 million strings on my machine, your original version takes about 1.3 seconds, while the version shown here takes around 1.0 seconds (redirecting the output to /dev/null on a Linux machine).

Use pointers rather than indexing for speed

Pointers are generally a faster way to access elements than using index variables.  For example, your filtArray routine could be written like this:

char *filtArray(const char *input, const bool *mask, char *filtered) {
    char *beginning = filtered;
    for ( ; *input; ++input, ++mask) {
        if (*mask) {
            *filtered++ = *input;
        }
    }
    *filtered  = '\0';
    return beginning;
}

Because you're just beginning, this may seem strange to you, but this kind of use of pointers is a very common idiom in C.

Compilers are good, but not quite that good yet

Because there's a tendency to assume the compiler will take care of it, here's compiler output comparison of the two approaches using gcc for ARM using the on-line compiler explorer:  https://godbolt.org/z/Y0TeVX

As can be seen in this case, the generated assembly code for the pointer version is much shorter.  Shorter code is usually faster (and it is in this case according to my testing) but not always.  For those who are expert in compiler design: The typical improvement is as likely to be the elimination of extra live variables as for the use of pointers per se, but the effect is nonetheless real.

Measured timings

For each of the three variations, original, pointer, and memcpy, here are the measured times for 10 million iterations and the variances of each set of samples and the relative speed measured as the average speed compared with the average speed of the original expressed as a percentage.  With no optimization:

\$\begin{array}{l|c|c|c}
{\bf name}&{\bf avg (s)}&{\bf var (s)}&{\bf relative}\\
\hline
\text{original}&1.344&0.01853&100.00\% \\
\text{pointer}&1.244&0.01193&92.56\% \\
\text{memcpy}&0.998&0.01177&74.26\%
\end{array}\$

With -O2 optimization:

\$\begin{array}{l|c|c|c}
{\bf name}&{\bf avg (s)}&{\bf var (s)}&{\bf relative}\\
\hline
\text{original}&1.038&0.01462&100.00\% \\
\text{pointer}&1.000&0.00135&96.34\% \\
\text{memcpy}&0.948&0.00692&91.33\%
\end{array}\$

These results were on a 64-bit Linux machine using gcc version 8.2.1.  I look forward to seeing other measured timing results.  Time is user time as measured by time -f %U (See https://linux.die.net/man/1/time for man page).


Responsive Foundation Orbit Image Gallery I decided to refactor the code for a responsive image gallery, that I setup not too long ago. Following the suggestions @joseph-the-dreamer made, and some other best practices, I cut down on the number of if statements.

Is there anything else I could do to make the activateSlider function more concise?

Is there another method one could use (in general) to avoid having to keep calling the same function on resize?

/* jQuery throttle / debounce - v1.1 - 3/7/2010 */
function debounce(a,b,c){var d;return function(){var e=this,f=arguments;clearTimeout(d),d=setTimeout(function(){d=null,c||a.apply(e,f)},b),c&&!d&&a.apply(e,f)}}

function activateSlider() {

   var slider = $('#slider'),
    sliderContainer = $('.orbit-container'),
    sliderContainerLinks = $(sliderContainer).children('a'),
    sliderKids = $('#slider li'),
    sliderContent = $('.slider-content'),
    sliderKidsContent = $(sliderKids).children(sliderContent),
    inactiveKidsContent = $('#slider li:not(.active) .slider-content'),
    accordionContainer = $('.accordion-container'),
    sliderBulletsContainer = $('.orbit-bullets-container');

  function transformSlider() {
    $(slider).foundation({
       orbit: {
         slide_number: false,
         timer: true,
         timer_speed: 8000,
         next_on_click: false
       }
   });

  if(window.matchMedia("(max-width: 767px)").matches) {
       /* Disable orbit gallery styles */
        $(sliderContainer).after($('.orbit-timer'));
        $(slider, sliderKids).attr('style', '');
        $(slider).removeClass('orbit-slides-container').removeAttr('data-orbit').addClass('accordion-container');
        $(sliderKids).removeAttr('data-orbit-slide').removeClass('active');
        $(sliderContainerLinks, sliderKidsContent).hide();
        $(inactiveKidsContent).css('display', 'none');

      /* Init accordion click functions */
        $(sliderKids).unbind().bind('click', function(){
            $(this).toggleClass('active').siblings().removeAttr('class');
            $(this).siblings().find(sliderContent).slideUp();
          $(this).find(sliderContent).slideToggle();
        });
  } else {
    /* If accordion styles are present, remove them */
    var OrbitStyles = ($(accordionContainer).length === 0);
    if (!OrbitStyles) {
        $(sliderContainerLinks).show();
        $(slider).removeClass('accordion-container').addClass('orbit-slides-container').attr('data-orbit', '');
        $(sliderBulletsContainer).before($('.orbit-timer'));
        $('.orbit-timer').removeClass('paused');
    }
      $(sliderContent).show();
      $(sliderKids).first().addClass('active').siblings().removeAttr('class');
  }
}
 transformSlider();
}

$(function(){
  if (window.Foundation) {
    activateSlider();
  }
});
/* Call the script on resize (must support ie8) */
if (window.addEventListener) {
   window.addEventListener('resize', debounce(function () {
    activateSlider();
  }, 250));
} else {
   window.attachEvent('resize', debounce(function () {
     activateSlider();
   }, 250));
 }

EDIT: It looks a little crude (stylewise) but the working version is here: http://jsbin.com/gicin/14/

First, I think you'd be better off if you had a representation of the gallery
as data (following the Rule of Representation).
It would go something like this:

A Simpler Method

Get your objects into memory. These can come from a REST API, or you could drop
this into the dom when you're rendering server side. However you like.

var gallery = [
  {
    title: "An image",
    src: "http://placehold.it/1400x348",
    width: 1400,
    height: 348,
    body: 'More hence euphemistic...',
    active: true,
    index: 0
    // any property you'll need to render this item goes here
  },
  // etc...
]

You can have the rendering code that consumes this list. The example shown uses
hyperscript.

var h = require('hyperscript')

function renderOne (slide) {
  h('li', {index: slide.index},
    h('img', {src: slide.src, width: slide.width}),
    h('section', slide.body)
    // and so on...
  )
}
function render (gallery) {
  h('ul#slider', gallery.map(renderOne))
}
// use it like this:
$('body').html(render(gallery))

Then you can listen for events on the containing element and modify the data
accordingly.

$('body').on('click', 'li', function (e) {
  var index = parseInt($(e.currentTarget).attr('index'), 10)
  gallery.forEach(function (slide){
    slide.active = false
  })
  gallery[index].active = true
})

With every property necessary for rendering held in the data representation,
your rendering logic can be very simple. Simple logic is easy to reason about
and breeds fewer bugs. To get an accurate dom after any change, all you need to
do is rerun:

$('body').html(render(gallery))

What about performance? There are excellent tools that abstract the dom into a
virtual dom that determines the minimal change necessary to get the current dom
to match the dom you want. Use those tools (like mercury
  or react).

If you're looking for another example, I did a similar post here.

The Original Method

That would be a significant change in method, however. Here's some ways to make
your current method easier to reason about for a newcomer to your codebase
(See it in action on jsbin here). Note: it was a style choice to omit semicolons and rely on automatic semicolon insertion.

Foundation now has a debounce function in its utils. May as well use that.

var debounce = window.Foundation.utils.debounce
var $copy, isSmall

You might also listen to requestAnimationFrame to get resize events. I left this
as resize.

// Call the script on resize (must support ie8)
var listen = window.addEventListener ? window.addEventListener : window.attachEvent
listen('resize', debounce(checkGallery), 250)

The logic is easy to follow if methods are mostly in the order they're called.

$(function(){
  // when swapping widgets, clone original state
  $copy = $('#slider').clone()
  checkGallery()
})

Descriptive names for complex boolean questions tend to increase clarity.

function checkGallery () {
  var wasSmall = isSmall
  isSmall = window.matchMedia('(max-width: 767px)').matches

  // only swap widgets if necessary
  if (wasSmall !== isSmall) initGallery(isSmall)
}

function initGallery (small) {
  // reset to inital state
  $('#hero').html(clone())

  if (small) {
    initAccordion()
  } else {
    initOrbit()
  }
}

This retains the selected slide when swapping widgets. I believe this is
not how the original worked, but I believe this was a desired behavior. I'd
point out that this logic is hard to follow for a newcomer to your codebase.
This function is a good example of why simple rendering logic + more complex
data structure is the way to go.

function clone () {
  $html = $copy.clone()

  // retain active item
  index = $('#slider > li').index($('#slider > li.active'))
  console.log('index', index)
  if (index > -1) $html.find('> li:eq('+index+')').addClass('active')

  return $html
}

This way, we don't bring foundation JavaScript into the mix unless we're using
it.

function initAccordion () {
  $('#slider').on('click', 'li', function (e) {
    $(e.currentTarget).addClass('active').siblings().removeClass('active')
  })
}

function initOrbit () {
  $('#slider').attr('data-orbit', '').foundation({
    orbit: {
      slide_number: false,
      timer: true,
      timer_speed: 8000,
      next_on_click: false
    }
  })
}

The only change necessary in CSS is this:

@media screen and (max-width: 767px) {
  #slider > li .slider-content {
    display: none;
  }
  #slider > li.active .slider-content {
    display: block;
  }
}


PHP Class for sending a response to Flash I've created this code to send a response to Flash which can be handled with AS2/AS3:

<?php
    /**
     * Basic server-side API controls for Flash-PHP communication
     * @author Marty Wallace
     * @version 1.0.2
     */
    class API
    {
        // MySQL
        private $con;
        private $sql_hostname = '';
        private $sql_username = '';
        private $sql_password = '';
        private $sql_database = '';

        // Properties
        public $response;

        /**
         * Constructor
         */
        public function __construct()
        {
            // Attempt connection
            $this->con = mysql_connect(
                $this->sql_hostname,
                $this->sql_username,
                $this->sql_password
            );

            // Connection could not be established
            if(!$this->con)
            {
                $this->response = array(
                    "status" => "no_connection",
                    "message" => "Could not connect to MySQL, try again later."
                );

                $this->respond();
            }

            // Select database
            mysql_select_db($this->sql_database);
        }

        /**
         * Send response back to Flash
         */
        public function respond()
        {
            $ar = array();
            foreach($this->response as $key => $value)
                array_push($ar, $key . "=" . $value);

            die(implode("&", $ar));
        }
    }
?>

I'm not that great with PHP and the respond() function seems like it could be written a little better.. Any suggestions?

I have used PHP for a few years now, and here are my suggestions:

array_push() can be used to add multiple values into an array at once, but in your case, your pushing only one value in at a time which means you can use:

$ar[] = $key . "=" . $value

This should also speed up your code a little( What's better to use in PHP $array[] = $value or array_push($array, $value)?).

die() is normally used when there is an error. It spits out a message to the user. If you are using this code to just connect to a MySQL database then change die(implode("&", $ar)); to  echo(implode("&", $ar));

But honestly, you should be able to take out the respond() function if you are only using it to send an array to flash.

        // Connection could not be established
        if(!$this->con)
        {
            $this->response = array(
                "status" => "no_connection",
                "message" => "Could not connect to MySQL, try again later."
            );

             echo(implode("&", $this->response));
        }


Refactoring different type of param to reduce duplicate code So say I have a method that does a bunch of stuff, and I want to refactor this method to take a different type of parameter:

public object A(MyObject a)
{
    // does a bunch of crap with a

    // then calls another method
    this.B(a);
}

Now even method B() does a lot of crap with that same Object a. What if I want an endpoint that takes a SecondObject b. What I just did was basically duplicate all the methods and change the parameter type, but now I'm sitting with basically a bunch of duplicate code here. What's the best way to refactor this so that all my methods can take a different set (not just a different type) of parameters and reduce duplicity?

Thanks.

Edit:

Example,

private Chatham.Panda.WCF.DataContracts.IInstrument FillHistoricInstrumentRates(
            Chatham.Panda.WCF.DataContracts.IInstrument instrument,
            Chatham.Enumerations.BidMidAsk valuationPrice,
            DateTime rateCutoffDate,
            bool loadRates,
            FillInstrumentRates recursiveDelegate)
        {
            if (instrument == null)
            {
                throw new ArgumentNullException(
                    "instrument",
                    "RateAcqusitionService.SetInstrumentHistRates's 'instrument' argument is null");
            }

            // Compound Instruments
            Chatham.Panda.WCF.DataContracts.CompoundInterestRateInstrument compoundInstrument =
                instrument as Chatham.Panda.WCF.DataContracts.CompoundInterestRateInstrument;

            if (compoundInstrument != null)
            {
                foreach (Chatham.Panda.WCF.DataContracts.IInstrument i in compoundInstrument.ChildInstruments)
                {
                    recursiveDelegate(i);

                    return compoundInstrument;
                }
            }

            // Simple Instruments
            Chatham.Panda.WCF.DataContracts.SimpleInterestRateInstrument simpleInstrument =
                instrument as Chatham.Panda.WCF.DataContracts.SimpleInterestRateInstrument;

            if (simpleInstrument != null)
            {
                // Set Rates for each Component
                foreach (Chatham.Panda.WCF.DataContracts.Component component in simpleInstrument.Components)
                {
                    this.FillHistoricComponentRates(
                        component,
                        valuationPrice,
                        rateCutoffDate,
                        loadRates
                        );
                }

                return instrument;
            }

            throw
                new NotSupportedException(
                    string.Format(
                        "Instrument with type '{0}' currently not supported",
                        instrument.GetType().FullName)
                    );
        }

Now I want to change this method so that it takes a List<ScheduleRow> rows, as well as some other random properties including within the Chatham.Panda.WCF.DataContracts.IInstrument instrument, instead of passing the whole instrument across the wire, but if I just create another method with all this code in it and different parameters, it won't look TOO much different, and have plenty of duplicate code.

Then I'd also be doing things like calling:

this.FillHistoricComponentRates(
                            component,
                            valuationPrice,
                            rateCutoffDate,
                            loadRates
                            );

With a list of rows instead of the entire component, which leads to THAT method being changed in the same exact fashion, so on and so on...

If I understand your question correctly I would refactor your method by implementing the template pattern in an OO or functional way whatever suits you best. Look at this excellent article for more information on the two options http://www.codeproject.com/KB/architecture/TMDPvsLambda.aspx.

This you'll be able to avoid duplicating your code while still being able to have different behaviors.


Implementation of ApplySort method from IBindingList I am currently developing a family of keyed collections, and I wanted to have one which would implement the IBindingList interface.

My BindingKeyedCollection is derived from a KeyedCollection which owns a look-up dictionary, used to make the researches by key.

However, I have suspicions about my implementation of the ApplySort method (a method used to sort the collection relatively to one of the items property). It seems to be working as it is now, but I am not sure that all items are synchronized properly.

protected virtual void ApplySortCore(PropertyDescriptor property, ListSortDirection direction)
    {
        this.SortProperty = property;
        this.SortDirection = direction;
        if (direction == ListSortDirection.Ascending)
        {
            ResetItems(this.OrderBy(item => property.GetValue(item)).ToList());
            IsSorted = true;
            OnListChanged(new ListChangedEventArgs(ListChangedType.Reset, property));
        }
        else if (direction == ListSortDirection.Descending)
        {
            ResetItems(this.OrderByDescending(item => property.GetValue(item)).ToList());
            IsSorted = true;
            OnListChanged(new ListChangedEventArgs(ListChangedType.Reset, property));
        }
    }

Basically I am using LINQ to order another collection and change its Items list with the main one.

private void ResetItems(List<TItem> items)
    {
        base.ClearItems();

        foreach (var item in items)
        {
            base.InsertItem(IndexOf(item), item);
        }
    }

I would like to have an external opinion on this way of sorting my collection, if they are any flaws that I would have missed, and if so, is there a way to avoid them?

The ApplySortCore() method could be made more dry

protected virtual void ApplySortCore(PropertyDescriptor property, ListSortDirection direction)
{
    this.SortProperty = property;
    this.SortDirection = direction;

    IEnumerable<T> items = null;
    if (direction == ListSortDirection.Ascending)
    {
        items = this.OrderBy(item => property.GetValue(item));
    }
    else
    {
        items = this.OrderByDescending(item => property.GetValue(item));
    }

    ResetItems(items.ToList ());

    IsSorted = true;
    OnListChanged(new ListChangedEventArgs(ListChangedType.Reset, property));
}

You should always try to code against interfaces instead of a concrete implementation.

In the ResetItems() method you should either use an IList<TItem> or better an IEnumerable<TItem> for the input parameter.

The call to this IndexOf() method seems a little bit strange. You already have the items sorted why don't you just add the items one by one ?

Because the methods are inherited from Collection<T> we can use the Add() method which simplifies the ResetItems() method to

private void ResetItems(IEnumerbale<TItem> items)
{
    base.ClearItems();

    foreach (var item in items)
    {
        base.Add(item);
    }
}


Rotate Matrix using Python This is my solution to Exercise 1.7 from Cracking the Coding Interview.  I would appreciate any feedback on coding style and algorithm efficiency.

I do know that there is an existing function in numpy for rotating a matrix, however I am trying to implement this as an exercise.

The problem statement follows:

  Given an image represented by an NxN matrix where each pixel in the image is 4 bytes, write a method to rotate the image by 90 degrees.  Can you do this in place?

import unittest

def rotate_square_matrix_right_90(matrix: list) -> list:
    """Rotate an NxN matrix 90 degrees clockwise."""
    n = len(matrix)
    for layer in range((n + 1) // 2):
        for index in range(layer, n - 1 - layer, 1):
            matrix[layer][index], matrix[n - 1 - index][layer], \
                matrix[index][n - 1 - layer], matrix[n - 1 - layer][n - 1 - index] = \
                matrix[n - 1 - index][layer], matrix[n - 1 - layer][n - 1 - index], \
                matrix[layer][index], matrix[index][n - 1 - layer]
    return matrix

class MyTest(unittest.TestCase):
    def test_rotate_matrix_right_90(self):
        input_matrix = [[0, 2, 4, 6], [-1, 1, 3, 5], [-2, 0, 2, 4], [-3, -1, 1, 3]]
        correct_rotated_matrix = [[-3, -2, -1, 0], [-1, 0, 1, 2], [1, 2, 3, 4], [3, 4, 5, 6]]
        self.assertSequenceEqual(rotate_square_matrix_right_90(input_matrix), correct_rotated_matrix)

if __name__ == "__main__":
    unittest.main()

You can use much simpler algorithm in python:
Transpose matrix:

zip(*matrix)

Inverse rows in transposed matrix (equals rotation right):

list(list(x)[::-1] for x in zip(*matrix))

However, if you want to rotate left, you need first inverse rows and then transpose, which is slightly more code.


Korean word segmentation using frequency heuristic This a continuation of a previous question. I want to thank Joe Wallis for his help with increasing the readability of my code. Although the changes made by Joe Wallis did increase the speed of the code, the speed improvements aren't enough for my purposes.

I'll reiterate the problem, but please feel free to look at the previous question. The algorithm uses a corpus to analyze a list of phrases, such that each phrase is split into constituent words in a way that maximizes its frequency score.

The corpus is represented as a list of Korean words and their frequencies (pretend that each letter represents a Korean character):

A 56
AB    7342
ABC   3
BC    116
C 5
CD    10
BCD   502
ABCD  23
D 132
DD    6

The list of phrases, or "wordlist", looks like this (ignore the numbers):

AAB       1123
DCDD  83

The output of the script would be:

Original    Pois        Makeup                    Freq_Max_Delta
AAB         A AB        [AB, 7342][A, 56]         7398
DCDD        D C DD      [D, 132][DD, 6][C, 5]     143

In the previous question, there are some sample inputs which I am using. The biggest problem is the size of the data sets. The corpus and wordlist have 1M+ entries in each file. It's currently taking on average 1-2 seconds to process each word in the wordlist, which in total will take 250 hours+ to process.

#!/usr/bin/env python
# -*- coding: utf-8 -*-
import sys, codecs, collections, operator, itertools
from argparse import ArgumentParser

sys.stdout = codecs.getwriter("utf8")(sys.stdout)
sys.stderr = codecs.getwriter("utf8")(sys.stderr)

def read_corpa(file_name):
    print 'Reading Corpa....'
    with codecs.open(file_name, 'r', 'UTF-8') as f:
        return {l[0]: int(l[-1]) for l in (line.rstrip().split('\t') for line in f)}

def read_words(file_name):
    with codecs.open(file_name, 'r', 'UTF-8') as f:
        for word in f:
            yield word.split('\t')[0]

def contains(small, big):
    small_ = len(small)
    for i in xrange(len(big) - small_ + 1):
        if big[i:i + small_] == small:
            return (i, i + small_)
    return None

def find_best(word, corpas):
    combos = {}
    for corpa, frequency in corpas.items():
        c = contains(corpa, word)

        if c:
            combos[word[c[0]:c[1]]] = frequency

    return combos

def create_combination(combos, word):
    if not combos:
        return None

    combo_keys = combos.keys()
    word = sorted(word)
    combinations_ = [
        j
        for i in range(len(combo_keys) + 1)
        for j in itertools.combinations(combo_keys, i)
        if sorted(''.join(j)) == word
    ]

    if not combinations_:
        return None

    result = None
    for combination in combinations_:
        sub = [(v, combos[v]) for v in combination]
        total = sum(map(operator.itemgetter(1), sub))
        if not result or total > result[2]:
            result = [combination, sub, total]

    return result

def display_combination(combination, word):
    if combination is None:
        print '\t\t'.join([word, 'Nothing Found'])
        return None

    part_final = ''.join(
        '[' + v[0] + ', ' +  str(v[1]) + ']'
        for v in combination[1]
    )

    print '\t\t'.join([word,' '.join(combination[0]), part_final, str(combination[2])])

def main():
    parser = ArgumentParser(description=__doc__)
    parser.add_argument("-w", "--wordlist", help="", required=True)
    parser.add_argument("-c", "--corpa", help="", required=True)
    args = parser.parse_args()
    corpas = read_corpa(args.corpa)
    for word in read_words(args.wordlist):
        combos = find_best(word, corpas)
        results = create_combination(combos, word)
        display_combination(results, word)

if __name__ == '__main__':
   main()

It will be much faster to use Python's in operator for your substring search rather than using your custom-built "contains" function.

In [15]: %timeit 'brown' in  'blah'*1000 + 'the quick brown fox'
100000 loops, best of 3: 2.79 µs per loop

In [16]: %timeit contains('brown','blah'*1000 + 'the quick brown fox')
1000 loops, best of 3: 870 µs per loop

Also I wonder if you could rewrite some of your custom functions as dictionary comprehensions, something like this:

for word in read_words(args.wordlist):
    combos = {k:v for k,v in corpus if k in word}
    results = {'Original': word,
               'Pois': list(combos.keys())
               'Makeup': combos.items()
               'Freq_Max_Delta': sum(combox.values())}

    print(results)


Dictionary of dictionary - Get 2nd keys I have a dictionary of dictionary such as d below, and I would like to get the list of all keys but not on the first level (i.e. ABC, DEF) but the one at the second level (i.e 0 and 1 for ABC ; 1 and 2 for DEF...)

I got this small code that does the job by reading all the dictionaries and by saving the keys (of second level):

d = {
    'ABC' : {'0' : [(1,0.5), (2, 0.25)], '1' : [(1,0.4), (3,0.35)]},
    'DEF' : {'1' : [(1,0.5), (4, 0.15)], '2' : [(2,0.3), (3,0.25)]},
    'GHI' : {'0' : [(3,0.2), (4, 0.05)], '2' : [(2,0.2), (2,0.25)]},
}

keys2 = []
for small_d in d.itervalues():
    keys2.extend(small_d.keys())

print set(keys2)
# set(['1', '0', '2'])

But I'm sure there is a more pythonic way to do it (and more efficient), any ideas?

Yes, there's a shorter (pythonic) way to do this using set comprehensions:

d = {
    'ABC' : {'0' : [(1,0.5), (2, 0.25)], '1' : [(1,0.4), (3,0.35)]},
    'DEF' : {'1' : [(1,0.5), (4, 0.15)], '2' : [(2,0.3), (3,0.25)]},
    'GHI' : {'0' : [(3,0.2), (4, 0.05)], '2' : [(2,0.2), (2,0.25)]},
}
print {j for i in d.itervalues() for j in i}
# set(['1', '0', '2'])

Then if you just want a list object you can use list() to convert the set to list.

More, in your example you could just make keys2 a set to begin with and call keys2.add instead of extend.

And some benchmarks from your solution, mine and other answers:

import timeit
from itertools import chain

def m1(d):
    return {j for i in d.itervalues() for j in i}

def m2(d):
    keys2 = []
    for small_d in d.itervalues():
        keys2.extend(small_d.keys())

    return set(keys2)

def m3(d):
    return set(chain.from_iterable(d.itervalues()))

def m4(d):
    keys2 = set([])
    for small_d in d.itervalues():
        keys2.update(small_d.keys())
    return keys2

d = {
    'ABC': {'0': [(1, 0.5), (2, 0.25)], '1': [(1, 0.4), (3, 0.35)]},
    'DEF': {'1': [(1, 0.5), (4, 0.15)], '2': [(2, 0.3), (3, 0.25)]},
    'GHI': {'0': [(3, 0.2), (4, 0.05)], '2': [(2, 0.2), (2, 0.25)]},
}

print(timeit.timeit(setup='from __main__ import m1, d', number=100000000))
print(timeit.timeit(setup='from __main__ import m2, d', number=100000000))
print(timeit.timeit(setup='from __main__ import m3, d', number=100000000))
print(timeit.timeit(setup='from __main__ import m4, d', number=100000000))

0.00136395591689003
0.0012866411345431158
0.0011429587956683193
0.0011374851827588035


Extracting Multiple Associative arrays in PHP public static function find_user_tracks($id)
{
    global $mySQL;
    $sql = "SELECT * FROM `tracks` WHERE account_id = {$id}";
    $result_set =  $mySQL->query($sql);
    $ret = array();
    while($row = $result_set->fetch_assoc())
    {
        $ret[] = $row;
    }
    return $ret;
}

The above code contains the result of a database query. The database looks like this:

So, basically, it will return a set of fields with values and then those values will be used for a certain page:

$row = Track::find_user_tracks($id);
if(!empty($row))
{
    $track_paths = array();
    $track_names = array();
    $track_desc = array();
    $track_ids = array();
    $track_artist = array();
    for($i = 0 ;$i<count($row);$i++)
    {
        $track_paths[] = $row[$i]['track_path'];
        $track_ids[] = $row[$i]['track_id'];
        $track_names[] = $row[$i]['track_name'];
        $track_desc[] = $row[$i]['track_desc'];
        $track_artist[] = $row[$i]['artist'];
    }
    echo "<h1> {$screename}'s Beats</h1>";
}

The $row variable contains the result from the previous queries. I loop over $row to extract every single bit of data like track_id, track_name, etc.

Is there any way to make this cleaner or improve it?

I think this will work. You'll have to have PHP version 5.3 though, otherwise you'll have to remove the lambda functions and define actual functions in their place. Disclaimer, not to say that this is easier to read or easier to understand, but I think it fits the "cleaner" request.

$track_paths = array_map( function ( $array ) { return $array[ 'track_path' ]; }, $row );
$track_ids = array_map( function ( $array ) { return $array[ 'track_id' ]; }, $row );
$track_names = array_map( function ( $array ) { return $array[ 'track_name' ]; }, $row );
$track_desc = array_map( function ( $array ) { return $array[ 'track_desc' ]; }, $row );
$track_artist = array_map( function ( $array ) { return $array[ 'track_artist' ]; }, $row );


Pointer version of strcat() Seems to work fine, I guess it won't work if the destination array's size is not defined by a macro, but a variable or something else, I thought about adding a third argument which is the size and let the user enter it, but that might be worse.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#define MAX_SIZE 100

char *ptr_str_cat(char *dest, const char* src){
    char *original;
    original = dest;
    if((strlen(dest) + strlen(src)) > MAX_SIZE){
        puts("The source string is too big for the destination");
        exit(EXIT_FAILURE);
    }
    dest = strchr(dest, '\0');
    *dest = '0';
    while((*dest++ = *src++) != '\0'){
        ; /*do nothing*/
    }
    return original;
}

int main(void){
    char dest [MAX_SIZE];
    char source [MAX_SIZE];
    puts("Enter the destination string");
    fgets(dest, sizeof(dest), stdin);
    dest[strlen(dest) -1 ] = '\0'; /*Remove trailing newline*/
    puts("Enter the source string");
    fgets(source, sizeof(source), stdin);

    printf("%s", ptr_str_cat(dest, source));

    return 0;
}

ptr_str_cat function:

The ISO C90 strcat requires the string dest to have sufficient space to hold the result of the concatenation. On the other hand strncat takes another argument size_t n and won't copy more then n characters to dest (+ the null terminator). You should definitely consider to implement one of these approaches, since the macro seems rather randomly chosen (you don't know whether dest is big enough to hold src).

Edit: See @JS1's answer to take a look at the approach of strncat. He also has a good catch with the Off-by-one-error.
If you want to completely leave out library function calls, you could even replace the strchr call with a simple loop.

while(*dest != '\0')
  dest++;

There is no need to explicitly overwrite the null terminator in dest. It will get overwritten by the contents of src and in the case of an empty src the null terminator is actually wanted.
The while-loop doesn't require extra curly braces. The NULL statement is sufficient.

while((*dest++ = *src++) != '\0')
    ; /* Intentionally left blank */

This is what you would come up with:

char *ptr_str_cat(char *dest, const char* src){
  char *original = dest

  while(*dest != '\0')
    dest++;

  while((*dest++ = *src++) != '\0')
    ;  /* Intentionally left blank */

  return original;
}

You don't even need to include <stdlib.h> using this implementation (since you don't use exit anymore).

main function:

Minor: dest[strlen(dest) - 1] = '\0'; will overwrite valid input (not the '\n'), when the first input line is longer than MAX_SIZE.

EDIT:

Good question regarding how to handle large input. I think this somehow depends on the target. I'd say our goal here is to create a piece of software, which is able to read in two strings (of any unknown size, separated by '\n') and later on have a pointer to the concatenation of these two strings.

One approach would be to allocate MAX_SIZE bytes of memory using malloc. Using fgets we fill the allocated memory and each time the string does not end on '\n' we reallocate MAX_SIZE more bytes and read in more characters. This will work until you run out of memory.


UVA 10474 - “Where is the marble?” I have been solving the problem from UVA 10474. In this problem, there will be marbles with numbers written on them. For example suppose five marbles with numbers 50 , 43, 43, 43, 2, 90, 44. There will be several query with number and position has to be told in a sorted sequence. For example after sorting these looks like : 2,43,43,43,44,50,90. Now, query for 43 will give output : 43 fond at 2 and query for -3 will output "-3 not found".

Can you please review it and provide me with feedback?

#include <iostream>
#include<stdio.h>
#include<vector>
#include<algorithm>

void printMarbelPositionIfFound()
{
    int numMarbles, numQueries, aMarble, case1;
    std::vector<int>marbles;

    case1 = 0;

    while(true)
    {
        scanf("%d %d", &numMarbles, &numQueries);
        if(0 == numMarbles && 0 == numQueries)return;

        case1++;

        for(int i=0; i<numMarbles; i++)
        {
            scanf("%d", &aMarble);
            marbles.push_back(aMarble);
        }

        std::sort(marbles.begin(), marbles.end());
        std::cout<<"CASE#"<<" "<<case1<<":"<<"\n";
        for(int i=0; i<numQueries; i++)
        {

            scanf("%d", &aMarble);

            std::vector<int>::const_iterator itr = std::lower_bound(marbles.begin(), marbles.end(),aMarble);
            if(*itr == aMarble)
            {
                std::cout<<aMarble<<" found at "<<itr - marbles.begin() + 1<<'\n';
            }
            else
            {
                std::cout<<aMarble<<" not found"<<'\n';
            }
        }

        marbles.clear();
    }
}

int main()
{
    printMarbelPositionIfFound();
    return 0;
}

You are mixing C with C++. I know these contests like speed, but it is usually unnecessary.
1. Prefer streams over scanf.
This means you can change this code:
    scanf("%d %d", &numMarbles, &numQueries);

Into this:
    std::cin >> numMarbles >> numQueries ;

2. Use the C++11 auto keyword when possible.
You have a C++11 tag, so I assume you have C++11 available.
You can change this:
std::vector<int>::const_iterator itr = std::lower_bound(marbles.begin(), marbles.end(),aMarble);

To this:
auto itr = std::lower_bound(marbles.begin(), marbles.end(),aMarble);

3. Make sure the iterator does not point past the end when you dereference it.
In this line: if(*itr == aMarble), you dereference the iterator. If aMarble is not in marbles, then std::lower_bound() will return marbles.end(). It is undefined behavior to dereference marbles.end(). Always check for this condition.
So change this line:
if(*itr == aMarble)

To this:
if (iter != marbles.end () && *itr == aMarble)

4. Prefer pre-increment over post-increment.
You have:
case1++;

In the case an int, it doesn't really matter. Your compiler will optimize this correctly. But if you had a complicated object, then you would be creating an unnecessary copy here. So:
++case1;

Would be a better habit to get into.
5. Encapsulate input and output with an Object.
You could create an object that represents a single unit of input. Something like this:
struct Input
{
    int nMarbles ;
    int nQueries ;
    static int caseNumber ;

    std::vector <int> marbles ;
    std::vector <int> queries ;

    Input () ;
    void Clear () ;

    friend auto operator>> (std::istream &is, Input &input) -> std::istream& ;
    friend auto operator<< (std::ostream &os, const Input &input) -> std::ostream& ;
};

Then you could implement the member and friend functions like this:
int Input::caseNumber = 0 ;

Input::Input () : nMarbles (), nQueries ()
{
}

void Input::Clear ()
{
    nMarbles = 0 ;
    nQueries = 0 ;
    marbles.clear () ;
    queries.clear () ;
}

auto operator>> (std::istream& is, Input &input) -> std::istream&
{
    ++Input::caseNumber ;

    is >> input.nMarbles >> input.nQueries ;

    if (input.nMarbles == 0 && input.nQueries == 0) {
        is.setstate (std::ios::failbit) ;
        return is ;
    }

    auto begin = std::istream_iterator <int> (is) ;
    std::copy_n (begin, input.nMarbles, std::back_inserter (input.marbles)) ;

    begin = std::istream_iterator <int> (is) ;
    std::copy_n (begin, input.nQueries, std::back_inserter (input.queries)) ;

    return is ;
}

auto operator<< (std::ostream &os, Input &input) -> std::ostream&
{
    os << "CASE# " << Input::caseNumber << ":" "\n" ;

    auto &marbles = input.marbles ;

    std::sort (std::begin (marbles), std::end (marbles)) ;

    for (int query : input.queries) {
        auto iter = std::lower_bound (std::begin (marbles), std::end (marbles), query) ;

        if (iter != std::end (marbles) && *iter == query) {
            os << query << " found at " << (std::distance (std::begin (marbles), iter) + 1) << "\n" ;
        }

        else {
            os << query << " not found" "\n" ;
        }
    }

    return os ;
}

Then your driver would look like this:
int main ()
{
    Input input ;

    while (std::cin >> input) {
        std::cout << input ;
        input.Clear () ;
    }

    return 0 ;
}

With the above code, I got an accepted answer with a runtime of 0.212 units on your website. So streams are more than fast enough for this particular challenge.


I think I should use JOIN but I did this instead Let me start by saying that I am brand new to PHP and MySQL.

I have two tables. One table is called domains and it contains the "rep" column which is an integer. It is related to the column "id" in the reps table. The reps table also houses the First and Last names of the rep assigned to a particular domain and when I query the database to show all of the domains, I need to show rep.first_name instead of domain.rep.

The first way I thought to do it was to make a loop that will create variables with the rep names and then inside of my loop to list the domains, I'll show them. It looks like this:

//Make Query
$rep_query = "SELECT * ";
$rep_query .= "FROM reps";
$rep_query_result = mysqli_query($connection, $rep_query);
//Test for Query Error
confirm_query($rep_query_result);
$rep_i = 1;
$rep_array = array("");
while($rep = mysqli_fetch_assoc($rep_query_result)) {
    ${'rep_' . $rep_i} = $rep['first_name'];
    array_push($rep_array, ${'rep_' . $rep_i});
    $rep_i++;
}

and inside of the loop where I'm looping through the domains, I can use the variable

$rep_array[$domains['rep']]

I then did some research on JOINs and realized this is the more common way to handle relationships and things of this sort, but my way seems okay.

What would you do differently?

Here is the domain query:

    //Make Query
    $domain_query = "SELECT * "; //IGNORE THIS FOR NOW <_<
    $domain_query .= "FROM domains ";
    $domain_query .= "WHERE status = 'Available' ";
    $domain_query .= "ORDER BY ". $order;
    $domain_query_result = mysqli_query($connection, $domain_query);
    //Test for Query Error
    confirm_query($domain_query_result);

and this is the join I came up with:

    $rep_query = "SELECT ";
    $rep_query .= "r.first_name, d.rep ";
    $rep_query .= "FROM `domains` AS d ";
    $rep_query .= "INNER JOIN `reps` AS r ";
    $rep_query .= "ON r.id = d.rep ";

How is it that I use the JOIN inside of the other query / loop to automatically set the IDs to the first_name? This seems to be the part that doesn't get explained in many of the tutorials and such that I've found.

First, about your current code:

You're doing a SELECT *. This is generally poor practice; instead, explicitly identify the columns you need: SELECT id, first_name.
Your $rep_array is a lookup table, so name it as such. Maybe $rep_id_to_name or something?
Your way of adding to the array is bad; it won't work if the rep ids are not contiguous. Instead, just directly set the id:

$rep_array = array();
while ($rep = mysqli_fetch_assoc($rep_query_result)) {
    $rep_array[$rep['id']] = $rep['first_name'];
}

Now, about using a JOIN...

I don't know the query for domains, but I'll assume it's something like this:

SELECT d.rep, ...
FROM `domains` d
WHERE ...

Then in order to use a join, you could do (note the absence of d.rep in the SELECT list:

SELECT r.first_name AS rep, ...
FROM `domains` d
JOIN `reps` r ON d.rep = r.id
WHERE ...

This will get the behaviour you want, but it wouldn't hurt to give a more meaningful name to rep. Keyboard strokes are (almost) free! Consider representative_name or something.


Functional / Declarative FizzBuzz I spent some time today trying to write FizzBuzz in a functional/declarative style. I thought it would be a good chance to get some feedback on it since I've been doing functional programming for about a year now.

(I know there are some similar questions asked here before, but their approach to the code has been different to mine and it would be good to get feedback more specific to my code)

The main things I want to know are:

How consistent is this code with the best practices in functional and declarative programming?
Are there any bad practices in it that I should be aware of?

const isFizz = number => number % 5 === 0
const isBuzz = number => number % 3 === 0

const newArrayInRange = (min, max) => [...Array(max + 1).keys()].slice(min)

const fizzBuzz = (min, max) => newArrayInRange(min, max)
  .map(number => {
    if (isFizz(number) && isBuzz(number)) {
      return 'fizzbuzz'
    }
    if (isFizz(number)) {
      return 'fizz'
    }
    if (isBuzz(number)) {
      return 'buzz'
    }
    return number
  })
  .join('\n')

console.log(fizzBuzz(1, 100))

Maybe some changes

Removing the arrays needed to hold the counter, its a huge overhead just for a counter.
Avoiding excessive logic statements using (num % val) to return strings and using the empty string to add a number (see code marked /*A*/)
Putting everything into an array to join when done as you have done. It is the quickest way to build a long string in JavaScript.

Thus we end up with the following...

const fizz = num => num % 5 ? "" : "fizz";
const buzz = num => num % 3 ? "" : "buzz";
const fizzBuzz = num => fizz(num) + buzz(num);
const fizzBuzzer = (min, max) => {
    const res = [];
    do {
        const fb =  fizzBuzz(min);
        res.push(fb ? fb : min);       /*A*/
    } while (min++ < max);

    return res.join("\n");
}

Personally the function fizz, buzz, fizzBuzz are just adding code without good reason and would need to be closed over to avoid polluting what ever scope it is in.

Thus the 3 function become the expression right of const fb =

Also I am not a fan of arrow functions in an open scope so using a function declaration to ensure accessibility.

function fizzBuzzer(min, max) {
    const res = [];
    do {
        const fb =  (min % 5 ? "" : "fizz") + (min % 3 ? "" : "buzz");
        res.push(fb ? fb : min);
    } while (min++ < max);

    return res.join("\n");
}

You ask

  How consistent is this code with the best practices in functional and declarative programming?

Best practice, well that is subjective, contextual and would only have comparative meaning when compared to "bad" code?

Your code is not bad, it works , it would have been worse 3 years back due to the way JS engines handled arrays, but now most optimizers recognize the pattern and make it fly.

There is a zillion ways to write any bit of code. JS programmers can still not agree on the use of semicolons and whether or not automatic semicolon insertion was a good idea or not, so how is any bit of code ever going to be best practice.

  Are there any bad practices in it that I should be aware of?

OMDG Yes there is.... Semicolons, where are they?

// Example of bad
const fizzBuzzard = (min, max) => {
    const next = n => n <= max ? order[n % 15] : () => "";
    const txt = w => n => (w ? w : n + "\n") + next(++n)(n);
    const [n, b, f, fb] = [txt(), txt("buzzard\n"), txt("fizz\n"), txt("fizzBuzzard\n")];
    const order = [fb, n, n, b, n, f, b, n, n, b, f, n, b, n, n];
    return next(min)(min);
}
console.log(fizzBuzzard(1, 100));


K&R C book, Exercise 1-21: Replace tabs with spaces I'm new to C, just started reading K&R C book, and am working through exercises. This is my solution to 1-21, and as far as I tested it works.
Anything that I'm doing wrong, or that it's not idiomatic C? What can be improved ?

  Exercise 1-21. Write a program entab that replaces strings of blanks
  by the minimum number of tabs and blanks to achieve the same spacing.
  Use the same tab stops as for detab. When either a tab or a single
  blank would suffice to reach a tab stop, which should be given
  preference?

#include <stdio.h>

#define TAB_SIZE 4

int main_entab() {
    int c;
    int character_count = 0;
    int whitespace_count = 0;
    while ((c = getchar()) != EOF) {

        if (c == ' ') {

            // A _ B _ | _ C _ _ |
            int numberOfCharsToReachTabStop = (TAB_SIZE - ((character_count + whitespace_count) % TAB_SIZE));

            whitespace_count++;

            if (numberOfCharsToReachTabStop == 1 && whitespace_count > 0) {
                putchar('\t');
                //advance count by how many whitespaces we are replacing, so next tab stop calculation is correct
                character_count = character_count + whitespace_count;
                whitespace_count = 0;
            }

        } else {
            //we encountered a non-whitespace char. Print all 'saved' whitespaces.
            while (whitespace_count > 0) {
                putchar(' ');
                whitespace_count--;
                character_count++;
            }

            //print the non-whitespace char
            character_count++;
            putchar(c);
        }
    }
}

Constants

#define TAB_SIZE 4

This is the old way of doing things.  Modernly C uses the same syntax as C++:

const int TAB_SIZE = 4;

While both will still function, this has the added benefit of offering type safety.  The #define is just a search and replace used by the preprocessor.  I.e. it replaces TAB_SIZE with the character 4 in an intermediate copy of the file.  The const variable will be managed by the compiler.

Update assignments

                character_count = character_count + whitespace_count;

This is more idiomatically written

                character_count += whitespace_count;

That form is exactly the same as the longer form.  It evaluates the left-hand side of the assignment, adds it to the right-hand side, and stores the result in the left-hand side.  But it is shorter and more easily recognizable.

Simplifying logic

However, I wouldn't do either of these.  Instead, on each iteration of the loop, just increment character_count.  Then you never have to add character_count and whitespace_count.  This will work in your program because you never use character_count separately from whitespace_count.  So make this change and replace character_count + whitespace_count with just character_count.

Yoda conditions

        if (c == ' ') {

This is often written as

        if (' ' == c) {

The reason is that if you accidentally leave off an equals sign ' ' = c will generate a compiler error.  Meanwhile, c = ' ' will silently do the wrong thing.  It wouldn't be so bad here, as it will be immediately obvious in the display.  But in a different kind of program, assigning to c might make a much more subtle bug.

Correct naming

            //we encountered a non-whitespace char. Print all 'saved' whitespaces.

This is incorrect.  A whitespace is any character that can produce indent.  It specifically includes tabs (and new lines).  But what you check is non-space characters.  This is easy enough to fix, just remove "white" both times here and rename whitespace_count to space_count.

Consistent naming style

You have the camelCased numberOfCharsToReachTabStop and snake_cased character_count and whitespace_count.  Please pick one kind of casing per identifier type and stick to it.  If local variables are snake_cased, then number_of_chars_to_reach_tab_stop.  Or change them all to camelCase.  I personally prefer snake_case, which is easier for non-native English speakers, as it doesn't rely on an ability to differentiate between capital and lowercase letters.  But the most important thing is to be consistent so that people can see snake_case and realize that it is a local variable or one of the other things that use snake_case.

TAB_SIZE is a constant, so it is appropriate to use a different casing style (e.g. ALL_CAPS) for it.

Bugs

As already noted, you don't print spaces at the end of a line.  But there is a more serious bug.  You treat tabs as single characters in the character count.  You should not.  Instead, tabs are TAB_SIZE characters.  Don't forget to adjust for the tab stop.  I.e. if you are currently one character past the tab stop, you'd only add three characters for the tab.  So if you adopt my previous suggestion of incrementing character_count on every iteration, you need something like

if ('\t' == c) {
    /* update character_count appropriately */
} else {
    character_count++;
}

I'll leave the actual update to you, as that's the exercise that you're exploring.


Optimizing jQuery/JS Switch Statement I am trying to optimize the following switch statement, but I'm having a brain fart about how to do it. Here's the code:

$('.newsroom-tabs div.pane-content ul li').click(function(){
// get the class name of the clicked link
var className = $(this).attr('class');
// variable so that jQuery isn't traversing the DOM each time
var hideFeaturedArticle = $('.featured-article').hide();
// based on the category selected, the featured article will
// show for that section
switch(className) {
    case 'air-quality-news-tab':
        hideFeaturedArticle;
        $('.featured-article.aqn').fadeIn(300);
        break;
    case 'awards-and-reviews-tab':
        hideFeaturedArticle;
        $('.featured-article.aar').fadeIn(300);
        break;
    case 'in-the-community-tab':
        hideFeaturedArticle;
        $('.featured-article.itc').fadeIn(300);
        break;
    case 'press-releases-tab':
        hideFeaturedArticle;
        $('.featured-article.pr').fadeIn(300);
        break;
    default:
        return className;

}
});

I feel like hiding the tabs each time is inefficient, but I'm not sure how to do this otherwise. Is there anything else I could do better here?

I notice that the elements you show have classes namesd after the elements you click.

For example:

The element with the class air-quality-news-tab
Will show the elements matching selector ".featured-article .aqn"

If this will always be the case, you could just reduce the clicked class down to the initials and use that to select the appropriate element like this:

(note that someone better at regex than me could probably make this even shorter)

$('.newsroom-tabs div.pane-content ul li').click(function() {
  var className = $.map($(this).attr('class').split('-'), function(val) {
    return val.match(/^[a-z]/i);
  }).join('').replace(/t$/i, '');
  $('.featured-article').hide();
  $('.featured-article.' + className).fadeIn(300);
});
.featured-article {
  display: none;
  width: 300px;
  height: 200px;
  background-color: #cccccc;
}

air-quality-news-tab

awards-and-reviews-tab

in-the-community-tab

press-releases-tab

aqn

aar

aqn

pr


Creating a cache manager I am purely new to C++ memory management.  Am I on the right path, or should I employ a different design strategy or a different memory manager policy (such as weak_ptr)?

#include <mutex>
#include <assert.h>
#include <iostream>
#include <unordered_map>
#include <memory>
#include <string>
#include <stdio.h>

//
// Requirements:
//  1: the bitmap could be used by multiple thread safely.(std::shared_ptr could?)
//    2: cache the bitmap and do not always increase memeory
//@NotThreadSfe
struct Bitmap {
    public:
        Bitmap(const std::string& filePath) {
            filePath_ = filePath;
            printf("foo %x ctor %s\n", this, filePath_.c_str());
        }
        ~Bitmap() {
            printf("foo %x dtor %s\n", this, filePath_.c_str());
        }
        std::string filePath_;
};

//@ThreadSafe
struct BitmapCache {
    public:
        static std::shared_ptr<Bitmap> loadBitmap(const std::string& filePath) {
            mutex_.lock();

            //whether in the cache
            auto iter = cache_.find(filePath);
            if (iter != cache_.end()) {
                if ((*iter).second) {
                    return (*iter).second;
                } else {
                    std::shared_ptr<Bitmap> newPtr(new Bitmap(filePath));
                    (*iter).second = newPtr;
                    return newPtr;
                }
            }

            //try remove unused elements if possible
            if (cache_.size() >= kSlotThreshold) {
                std::unordered_map<std::string,std::shared_ptr<Bitmap>>::iterator delIter = cache_.end();
                for (auto iter = cache_.begin(); iter != cache_.end(); ++iter) {
                    auto& item = *iter;
                    if (item.second && item.second.use_count() == 1) {
                        delIter = iter;
                        break;
                    }
                }
                if (cache_.end() != delIter) {
                    (*delIter).second.reset();
                    cache_.erase(delIter);
                }
            }

            //create new and insert to the cache
            std::shared_ptr<Bitmap> newPtr(new Bitmap(filePath));
            cache_.insert({filePath, newPtr});
            mutex_.unlock();
            return newPtr;
        }
    private:
        static const int kSlotThreshold = 20;
        static std::mutex mutex_;
        static std::unordered_map<std::string,std::shared_ptr<Bitmap>> cache_;
};

/* static */
std::unordered_map<std::string,std::shared_ptr<Bitmap>> BitmapCache::cache_;

/* static */
std::mutex BitmapCache::mutex_;

int main()
{
    //test for remove useless element
    char buff[200] = {0};
    std::vector<std::shared_ptr<Bitmap>> bmpVec(20);
    for (int i = 0; i < 20; ++i) {
        sprintf_s(buff, 200, "c:\\haha%d.bmp", i);
        bmpVec[i] = BitmapCache::loadBitmap(buff);
    }
    bmpVec[3].reset();
    std::shared_ptr<Bitmap> newBmp = BitmapCache::loadBitmap("c:\\new.bmp");

    //test for multiple threading...(to be implemenetd)
    return 0;
}

The major question you should be asking is "what am I going to do with these bitmaps"? Threading is an issue when you have multiple threads writing the same data - multiple threads reading the same data (and only reading it) don't cause such problems. Whether you are going about this the correct way or not depends heavily on the answer to that question.

However, some of this code can be cleaned up and improved somewhat, regardless of that answer.

Calling mutex_.lock() and then explicitly calling mutex_.unlock() is dangerous. If anything in between those calls throws an exception, then your mutex will never be unlocked, and you'll have a deadlock. Solving this is easy, and just requires using what is known as RAII (Resource Acquisition is Initialization). In this case, this is done by using a std::lock_guard:

  static std::shared_ptr<Bitmap> loadBitmap(const std::string& filePath) {
      std::lock_guard<std::mutex> guard(mutex_);
      ...
  } // mutex_ is unlocked as soon as it goes out of scope

This ensures that mutex_ will always be unlocked, no matter how the function exists (whether normally or through an exception).

You use auto in a few places, so you might as well simplify:

std::unordered_map<std::string,std::shared_ptr<Bitmap>>::iterator delIter = cache_.end();

to:

auto delIter = cache_.end();

The loadBitmap function itself has dubious design. It isn't just loading a bitmap, but is also trying to clean out the cache. Performing this second step should absolutely be broken out into a separate function:

void remove_from_cache()
{
    if (cache_.size() >= kSlotThreshold) {
    auto delIter = cache_.end();
    for (auto iter = cache_.begin(); iter != cache_.end(); ++iter) {
        auto& item = *iter;
        if (item.second && item.second.use_count() == 1) {
            delIter = iter;
            break;
        }
    }
    if (cache_.end() != delIter) {
        (*delIter).second.reset();
        cache_.erase(delIter);
    }
}

The current cache size is very small (20 elements). If you expect this to remain small (< 1000 elements), you may want to consider using a std::vector instead. At this size, constants involved in the O(1) lookup for a hashmap dominate, and std::vector will (potentially) be quicker. If you expect many more lookups than insertions, a sorted vector could also work.

The reason I suggest is that this would make clearing out the cache much easier: using a std::vector (which would look something like std::vector<std::pair<std::string, std::shared_ptr<Bitmap>>> (which could really do with a using statement or 3)), then erasing values is simply:

using pair_t = std::pair<std::string, std::shared_ptr<Bitmap>>;
using container_t = std::vector<pair_t>;
container_t cache;

void remove_from_cache()
{
    if(cache.size() > kSlotThreshold) {
        cache.erase(std::remove_if(cache.begin(), cache.end(),
             [](const pair_t& p) { return p.second.use_count() == 1; }),
             cache.end());
    }
}

Whether this removal algorithm is any good or not is up for some debate, you might want to look at something like an LRU (least recently used) cache instead - currently, there is no guarantee that your algorithm will remove things from the cache. Also, this would require a (very minor) change to your lookup code:

 auto iter = cache_.find(filePath);

would become:

 auto iter = std::find_if(cache.begin(), cache.end(),
     [&](const pair_t& p) { return p.first == filePath; });

A few final small points:

Prefer to use std::make_shared instead of constructing the shared pointer directly, for example:

std::shared_ptr<Bitmap> newPtr(new Bitmap(filePath));

should be:

auto newPtr = std::make_shared<Bitmap>(filePath);

If you're defining a struct just to give things a scope to live in (and hence using static for everything), use a namespace instead.


Validator for fractional numbers I used some web resources to find how to make a web page with javascript validator for fractional numbers. What do you think about it? How it can be improved and still look like a web page? Your help will be highly apreciated. Thank you very much!!

<title>
JavaScript fractional numbers validator
</title>

<form name="form1">

   Enter Decimal Number: <input id="Text1" type="text" />

<input id="Button1" type="button" value="Check" onclick="checkDecimal();" />

</form>

It will be great if the code is shorter and eventually possible to have maximum symbols length for the whole part that is different (let's say double) from the fraction part.

How about this one? Representing numbers as object to be able to use the logic for both whole and fraction operations. Also using regexp to check input symbols and length:

function checkNumber() {
  var stringsFromInput = document.getElementById("inputNumber").value.split(/[,\n.]/),
      numbers = [
        {
          name: "whole",
          regexp: /^[0-9]{1,10}$/,
          errorMsg: " number expected to be 10 symbols maximum"
        }, {
          name: "fraction",
          regexp: /^[0-9]{0,5}$/,
          errorMsg: " number expected to be 5 symbols maximum"
        }
      ];
  for (var n in numbers) {
    var name = numbers[n].name,
        value = stringsFromInput[n],
        regexp = numbers[n].regexp,
        msg = numbers[n].errorMsg,
        div = document.getElementById(name + "Div");
        regexp.test(value) ? div.innerHTML = name + " number is OK" : div.innerHTML = name + msg;
  }
}
<form>

    Enter a number
    <input type="string" class="form-control" id="inputNumber" placeholder="123456789.54321">
    <button type="button" onclick="checkNumber()">Check</button>

Status will

appear here

</form>


github page/ DHH Score Challenge in ruby I wrote the following code which when executed prints the score of https://github.com/dhh.

require 'typhoeus'
require 'json'

class DHHScoreChallenge
    def getData
        req = Typhoeus.get("https://api.github.com/users/dhh/events/public")
        body =  req.body
        json_body = JSON.parse(body)
   end

   def calculateScore json_body
       type_and_score = {
          :IssuesEvent => 7,
          :IssueCommentEvent =>  6,
          :PushEvent => 5,
          :PullRequestReviewCommentEvent => 4,
          :WatchEvent => 3,
          :CreateEvent => 2
       }
       score = 0
       json_body.each do |commit|
          commit_type =  commit['type']
          if type_and_score.has_key? commit_type.to_sym
             score = score + type_and_score[commit_type.to_sym]
          else
             score = score + 1
         end
       end
       score
   end

   def showScore score
      puts "DHH's github score is #{score}"
   end
end

begin
    d = DHHScoreChallenge.new
    json_body = d.getData
    score = d.calculateScore(json_body)
    d.showScore(score)
end

I am a beginner in ruby programming and would like to know the best practices of Ruby. I am trying to include portability, encapsulation, reusability in my code. Please let me know if they have been achieved and if there are any improvements that I can make.
Thanks in advance!

The major thing I see is, the code is imperative. But things will be lot more easier to read and follow, if you practise declarative way of writing code.

I would not name the class as DHHScoreChallenge. It should be ScoreChallenge, so that it can be used for any user that we pass in :)

I would make the method getData private, in order to externalise the service layer dependency.

Also I would move the type_and_score into a class level map

This logic is too complex to read

if type_and_score.has_key? commit_type.to_sym
  score = score + type_and_score[commit_type.to_sym]
else
  score = score + 1
end

This could have eliminated by using a combination of dig and fetch. Dig will return nil if you cannot find the key in hash and fetch on a hash can return a value if the value of the key returns nil

Also I see that you are converting the event key to symbol and trying to find the symbol in hash. Instead you might as well have created a hash with strings as symbols, it won't be much of a over head if you freeze the hash.

Something like this

  EVENTS_SCORE_MAP = {
    'IssuesEvent' => 7,
    'IssueCommentEvent' => 6,
    'PushEvent' => 5,
    'PullRequestReviewCommentEvent' => 4,
    'WatchEvent' => 3,
    'CreateEvent' => 2
  }.freeze

 EVENTS_SCORE_MAP.fetch(event.dig('type'), 1)

I rewrote the whole thing in a manner which I would in my production system

require 'typhoeus'
require 'json'

# Github service dealing with all the requests to github
class Github
  def events(user_name)
    url = "https://api.github.com/users/#{user_name}/events/public"
    send_request(
      lambda {
        Typhoeus.get(url)
      }, url
    )
  end

  private

  def send_request(request, path)
    request.call
  rescue Timeout::Error
    raise GatewayTimeoutError.new(gateway: 'github api timed out', path: path)
  end
end

# This class deals calculating the score
class ScoreChallenge
  EVENTS_SCORE_MAP = {
    'IssuesEvent' => 7,
    'IssueCommentEvent' => 6,
    'PushEvent' => 5,
    'PullRequestReviewCommentEvent' => 4,
    'WatchEvent' => 3,
    'CreateEvent' => 2
  }.freeze

  def initialize(user_name, score = 0)
    @user_name = user_name
    @score = score
  end

  def pretty_score
    calculate
    puts "#{@user_name} github score is #{@score}"
  end

  private

  def calculate
    events_of_user = JSON.parse(github_service.events(@user_name).body)
    events_of_user.each do |event|
      @score += EVENTS_SCORE_MAP.fetch(event.dig('type'), 1)
    end
  end

  def github_service
    @github_service = Github.new
  end
end

score_challenge = ScoreChallenge.new('DHH')
score_challenge.pretty_score

As you can see, I have exposed only one method called pretty_score. Which does everything that we need. This is the declarative style of programming I was talking about. I am letting my code figure out what to do, rather than me telling it do something step by step

Also Github is a service class I have created. It should ideally sit under a services folder. I have not implemented GatewayTimeoutError here, but for all practical purposes we should assume that these third party services can timeout/error out we should be able to handle them. You can also look into passing blocks, and custom errors as part of the above code.


Saving data in local storage I have written a small library for saving data on local storage. I am still a learner and I am not sure whether my code is OK for a production level application.

function AppStorage (aName) {
    "use strict";

    var prefix  = aName;

    var saveItem = function (key, value) {

        if (!key || !value) {

            console.error("Missing parameters \"key\" and \"value\"");

            return false;
        }

        if (window.localStorage && window['localStorage']) {

            try {

                localStorage.setItem(prefix + '_' + key, JSON.stringify(value));

            } catch (e) {

                return false;

            }

        } else  {

            return false;

        }

    }

    var getItem = function (key) {

        if (!key) {

            console.error("Missing parameter \"key\"");

            return false;
        }

        if (window.localStorage && window['localStorage']) {
            try {

                localStorage.getItem(prefix + '_' + key);

            } catch (e) {

                return false;

            }

        } else  {

            return false;

        }

    }

    var removeItem = function (key) {

        if (!key) {

            console.error("Missing parameter \"key\"");

            return false;
        }

        if (window.localStorage && window['localStorage']) {

            try {
                return localStorage.removeItem(prefix + '_' + key)
            } catch (e) {
                return false;
            }

        } else  {

            console.log("Browser doen not support HTML5 Web Storage");

        }

    }

    return {
        set: function (key, value) {
            return saveItem(key, value);
        },
        get: function () {
            return getItem(key, item);
        },
        remove: function () {

        }
    }

}

var as = new AppStorage ('MyApp');

Structure:

Instead of having a function encompass everything, consider using a prototype constructor, or if you can use ECMAScript 2015, use the class structure.

By using the prototype structure your code would look something like the following:

var AppStorage = function(name){
    this.prefix = name;
}
AppStorage.prototype.saveItem = function(key, value){

}
AppStorage.prototype.getItem = function(key){

}
AppStorage.prototype.removeItem = function(key){

}
var AS = new AppStorage('myName');
AS.getItem('thing');

Which removes the need for blocks like the following:

return {
    set: function (key, value) {
        return saveItem(key, value);
    },
    get: function () {
        return getItem(key, item);
    },
    remove: function () {

    }

Things to improve upon:

console.error: instead of that, use throw new Error() instead.
There's a consistent use of extraneous empty lines, remove them. They're pointless and waste space.
When you return the aliases for the functions, the remove function is left empty, if that is the desired case, then it should simply be removed, otherwise, you forgot to link the remove function.
window.localStorage && window['localStorage']: uh, that's identical; you can access object properties via the . method, or [''] method.
prefix + '_' + key: it would be easier/better to just append the underscore when you define the prefix initially.
removeItem is the only function that doesn't just return false if localStorage does not exist. Consider incorporating the error message in every function, or just check for localStorage on initialisation.

With those changes in mind...

... here's your updated code:

var AppStorage = function(name) {
    if (!(this instanceof AppStorage)) {
        throw new Error("AppStorage must be invoked with new!");
    }
    if (!window.localStorage){
        throw new Error("Browser does not support HTML5 Web Storage");
    }
    this.prefix = name + '_';
}
AppStorage.prototype.save = function(key, value) {
    if (!key || !value) {
        throw new Error("Missing parameters \"key\" and \"value\"");
    }
    try {
        localStorage.setItem(this.prefix + key, JSON.stringify(value));
    } catch (e) {
        return false;
    }
};
AppStorage.prototype.get = function(key) {
    if (!key) {
        throw new Error("Missing parameter \"key\"");
    }
    try {
        localStorage.getItem(this.prefix + key);
    } catch (e) {
        return false;
    }
};

AppStorage.prototype.remove = function(key) {
    if (!key) {
        throw new Error("Missing parameter \"key\"");
    }
    try {
        return localStorage.removeItem(this.prefix + key);
    } catch (e) {
        return false;
    }
};
var as = new AppStorage('MyApp');