Brute force algorithm for printing all possible alphanumeric combinations I'm still very much a beginner, so I don't really know much about best practices or the speed of any particular C functions.

You'll notice that I included a "cs50" library. This is from a MOOC I'm taking, all it does in this program is allow me to use their "bool" data type. I realize I don't need to delegate all of this code to so many functions, but this is part of a bigger problem set from said MOOC, where I am to crack a password, so I thought to make the functions now rather than later.

The program loops through and prints all possible iterations of alphanumeric combinations (no uppercase) from length 1-8. The algorithm in its current state is very slow. I've also never run it to the very end:

#include <cs50.h>       //GetString, booleans, and string datatype
#include <stdio.h>      //printf
#include <string.h>     //strlen

#define LOOP_MAX 8

bool BruteForceIntializer();
int looper (int amt, int count);

char engWord[9];
char alphaNum[36] = {"0123456789abcdefghijklmnopqrstuvwxyz"};
int maxSize = sizeof(alphaNum);

int main(void)
{
    if (BruteForceIntializer() == true)
        printf("Success.\n");
    else
        printf("Failure.\n");
}

bool BruteForceIntializer()
{
    int count;
    for (int amt = 0; amt < LOOP_MAX; amt++)
    {
        memset(engWord, '\0', 9);
        count = 0;
        if (looper(amt, count) == 0)
        {
            continue;
        }
    }
    return true; //only exits loop if all chars are looped successfully
}

int looper(int amt, int count)
{
    for(int i = 0; i < maxSize; i++)
    {
        engWord[count] = alphaNum[i];
        if (count == amt) //check if we're at the last character
        {
            printf("%s\n", engWord);
            if (i == (maxSize-1))
                return 0;
        }
        if (count != amt)
        {
            if (looper(amt, count+1) == 0)
                continue;
        }
    }
    return 1;
}

I must say that this looks pretty clean for a beginner.  There are no obvious problems with indentation and whitespace (common with beginners), which is a great start.  This especially makes it easier for others to read your code and for reviewers to focus on the more important aspects.

In most cases, don't use global variables:

char engWord[9];
char alphaNum[36] = {"0123456789abcdefghijklmnopqrstuvwxyz"};
int maxSize = sizeof(alphaNum);

This is highly discouraged because they're exposed to all the code and can be modified from anywhere, making maintenance and debugging much more difficult.  Instead, you can just put them in looper() since only that function uses them.  That way, if they're modified by accident, you'll know where to look.

In addition, alphaNum and maxSize should be const since they're not meant to be modified:

const char alphaNum[36] = {"0123456789abcdefghijklmnopqrstuvwxyz"};
const int maxSize = sizeof(alphaNum);

Some other issues seem to be with the naming:

There is one inconsistency: the naming convention of both non-main functions.  It's common for functions in C to start with a lowercase letter (either in camelCase or snake_case), so BruteForceInitializer() should be renamed as such.
It doesn't seem to make sense for BruteForceInitializer() to return bool if it's actually initializing something.  It looks like this function is doing most of the work anyway.  You could consider renaming this function in the verb form, which is common with functions (since they're performing a task).  It'll also help the check in main() to make more sense.
The name looper() doesn't quite seem clear, especially with the values it returns.  Based on the one comment in there, perhaps this function should also return bool.


Python pandas: Take column of counts and create DataFrame with a row per count I have a DataFrame with values in columns a and b and a third column with the count of that row. I would like to convert this into a DataFrame (either new or remake the old one) with columns a and b repeated the number of times as is in the count column. It's probably more clear with an example. I have this DataFrame:

import pandas as pd
df = pd.DataFrame({'a' : [1,2,3], 'b' : [0,0,1], 'count' : [3,1,4]})

I am converting it like this:

new_df = pd.DataFrame(columns=df.columns[:-1])
for _, row in df.iterrows():
    num = row['count']
    for i in range(num):
        pd.concat([new_df, row])
        new_df = new_df.append(row[:-1])

This does exactly what I want but seems inelegant to me because of the for-loop inside iterrows. Is there a better or more pythonic way to do this?

You are correct in thinking that iterrows is a very bad sign for Pandas code. Even worse is building up a DataFrame one row at a time like this with pd.concat - the performance implications are dreadful.

Instead of reaching for loops, your first step should be to check if there is a vectorized DataFrame method you could use. In this case.. perhaps there isn't.

Next step is dropping into NumPy. And lo and behold, there is numpy.repeat, which allows you to repeat an array along an axis with another array of counts. We can wrap that up in a function.

def repeat_frame(df, counts):
    rep_array = np.repeat(df.values, counts, axis=0)
    return pd.DataFrame(rep_array, columns=df.columns)

# would be called with
repeat_frame(df[['a', 'b']], df['count'])

This runs in 450 µs on my machine compared to your current solution in 20.2 ms for your sample data. On a larger sample size with 1000 rows, it runs in 511 µs compared to 6.86 s for your current solution, roughly 13000 times faster.

If you find yourself wanting to use pd.concat or similar in any situation like this to build up a DataFrame row-by-row, stop! There is going to be a faster way.

Check for vectorized Pandas functions that can help you out
Ditto for NumPy and SciPy after that

And if you are certain you have some wild iteration logic that cannot be vectorized (which is unlikely), create an empty array of the necessary shape and consider using Numba or Cython to speed up filling it up with a loop.


Mongoose schema to store items from Wynncraft's API I am making a Schema to store items from Wynncraft's API.

See an example assortment of items here:

https://api.wynncraft.com/public_api.php?action=itemDB&search=a

I have not finished making the Schema because I want to know if there's a better way I should approach it:

const mongoose = require('mongoose');
const uniqueValidator = require('mongoose-unique-validator');

const ItemSchema = new mongoose.Schema({
  name: {
    type: String,
    required: true,
    unique: true
  },
  tier: {
    type: String,
    enum: ['Unique', 'Rare', 'Legendary', 'Mythic', 'Set', 'Normal'],
    required: true
  },
  identified: {
    type: Boolean,
    required: true
  },
  set: {
    type: String,
    required: true
  },
  sockets: {
    type: Number,
    required: true
  },
  accessoryType: {
    type: String,
    required: () => this.category == 'accessory'
  },
  material: {
    type: String
  },
  restrictions: {
    type: String
  },
  damage: {
    type: String
  },
  fireDamage: {
    type: String
  },
  waterDamage: {
    type: String
  },
  airDamage: {
    type: String
  },
  thunderDamage: {
    type: String
  },
  earthDamage: {
    type: String
  },
  attackSpeed: {
    type: String
  },
  type: {
    type: String
  },
  armorType: {
    type: String
  },
  armorColor: {
    type: String
  },
  addedLore: {
    type: String
  },
  dropType: {
    type: String
  },
  health: {
    type: Number
  },
  fireDefense: {
    type: Number
  },
  waterDefense: {
    type: Number
  },
  airDefense: {
    type: Number
  },
  thunderDefense: {
    type: Number
  },
  earthDefense: {
    type: Number
  },
  level: {
    type: Number
  },
  quest: {
    type: String
  },
  classRequirement: {
    type: String
  },
  strength: {
    type: Number
  },
  dexterity: {
    type: Number
  },
  intelligence: {
    type: Number
  },
  agility: {
    type: Number
  },
  defense: {
    type: Number
  },
  healthRegen: {
    type: Number
  },
  manaRegen: {
    type: Number
  },
  spellDamage: {
    type: Number
  },
  damageBonus: {
    type: Number
  },
  lifeSteal: {
    type: Number
  },
  manaSteal: {
    type: Number
  },
  xpBonus: {
    type: Number
  },
  lootBonus: {
    type: Number
  },
  reflection: {
    type: Number
  },
  strengthPoints: {
    type: Number
  },
  dexterityPoints: {
    type: Number
  },
  intelligencePoints: {
    type: Number
  },
  agilityPoints: {
    type: Number
  },
  defensePoints: {
    type: Number
  },
  thorns: {
    type: Number
  },
  exploding: {
    type: Number
  },
  speed: {
    type: Number
  },
  attackSpeedBonus: {
    type: Number
  },
  poison: {
    type: Number
  },
  healthBonus: {
    type: Number
  },
  soulPoints: {
    type: Number
  },
  emeraldStealing: {
    type: Number
  },
  healthRegenRaw: {
    type: Number
  },
  spellDamageRaw: {
    type: Number
  },
  damageBonusRaw: {
    type: Number
  },
  bonusFireDamage: {
    type: Number
  },
  bonusWaterDamage: {
    type: Number
  },
  bonusAirDamage: {
    type: Number
  },
  bonusThunderDamage: {
    type: Number
  },
  bonusEarthDamage: {
    type: Number
  },
  bonusFireDefense: {
    type: Number
  },
  bonusWaterDefense: {
    type: Number
  },
  bonusAirDefense: {
    type: Number
  },
  bonusEarthDefense: {
    type: Number
  },
  category: {
    type: String,
    enum: ['accessory', 'armor', 'weapon'],
    required: true
  },
  // flags will be added as items are moved from Wynncraft's official API
  // into my database. The official API is inconsistent, I will put problems here,
  // e.g. "item.sockets undefined, setting to 0" and things like this.
  flags: {
    type: [String],
    required: true
  }
});

ItemSchema.plugin(uniqueValidator);

module.exports = mongoose.model('Item', ItemSchema);

There's so much repetition and it's just so long, I wonder if this schema can be created in a less verbose way.

As I see there is about 60 properties for each item in Wynncraft DB. I see two ways to improve your design:

Distribute all properties across multiple schemas: basic item information (name, category, etc), defense information, bonus damage information and other. In this case those schemas will be related to each other. Maybe you don't even need to store all item's data (depends on goal of your application).
If you want to identically map properties of item to your storage, then I guess you can get list of all properties as JSON (by using Wynncraft's public API) and convert it to schema definition. JSON with list of properties could be cached for better performance.


Count non-transparent pixels in a canvas I need to count non-transparent pixels in a canvas. I am pretty sure there is a nice and elegant syntax for doing this. Note that the data returned by getImageData is not a regular array and cannot be sliced, for example, or it does not have a reduce method.

data = context.getImageData(0, 0, canvas.width, canvas.height).data
count = 0
i = 0
for x in data
  if (i + 1) % 4 == 0 && x > 0
    count++
  i++

What about this?!

data = context.getImageData(0, 0, canvas.width, canvas.height).data
count = 0
count++ for x, i in data when (i+1) % 4 is 0 and x > 0


Bash grep taking 3 days to finish. Any suggestions to enhance it? I have a script like this that I would like to seek some suggestions on enhancing it.

cd /home/output/
cat R*op.txt > R.total.op.txt
awk '{if( (length($8)>9) || ($8 ~ /^AAA/) ) {print $0}}' R.total.op.txt > temp && mv temp R.total.op.txt

cat S*op.txt > S.total.op.txt
awk '{if( (length($8)>9) || ($8 ~ /^AAA/) ) {print $0}}' S.total.op.txt > temp && mv temp S.total.op.txt

cat R.total.op.txt S.total.op.txt | awk '{print $4}' | sort -k1,1 | awk '!x[$1]++' > genes.txt
rm *total.op.txt
head genes.txt

cd /home/output/
for j in R1_with-genename R2_with-genename S1_with-genename S2_with-genename
do
***** for i in `cat genes.txt`; do cat $j'.op.txt' | grep -w $i >> $j'_'$i'_gene.txt';done
done
ls -m1 *gene.txt | wc -l
find . -size 0 -delete
ls -m1 *gene.txt | wc -l
rm genes.txt

cd /home/output/
for i in `ls *gene.txt`
do
paste <(awk '{print $4"\t"$8"\t"$9"\t"$13}' $i | awk '!x[$1]++' | awk '{print $1}') <(awk '{print $4"\t"$8"\t"$9}' $i | awk '{if( (length($2)>9) || ($2 ~ /^AAA/) ) {print $0}}' | sort -k2,2 | awk '{ sum += $3 } END { if (NR > 0) print sum / NR }') <(awk '{print $4"\t"$8"\t"$9}' $i| awk '{if( (length($2)>9) || ($2 ~ /^AAA/) ) {print $0}}' | sort -k2,2 | wc -l) <(awk '{print $4"\t"$8"\t"$9"\t"$13}' $i | awk '{if( (length($2)>9) || ($2 ~ /^AAA/) ) {print $0}}' | sort -k2,2 | grep -v ":::" | wc -l) > $i'_stats.txt'
done
rm *gene.txt

cd /home/output/
for j in R1_with-genename R2_with-genename S1_with-genename S2_with-genename
do
cat $j*stats.txt > $j'.final.txt'
done
rm *stats.txt

cd /home/output/
for i in `ls *final.txt`
do
sed "1iGene_Name\tMean1\tCalculated\tbases" $i > temp && mv temp $i
done
head *final.txt

The second for loop (marked with asterisks) that has cat genes.txt is the grep loop that is taking 3 days to finish. Can someone please advice any enhancements to the command and if this entire script can be made into a single command? Thanks in advance. I posted in stackoverflow and was asked to send it here. :)

Without your data, it's difficult to test if the changes help at all. I can only assume that the constant reading and writing to files on disk is slowing down your program. If your data is simply big enough to require 3 days to run in a bash script, then you may have to switch to another programming language with more complex data structures that can do your processing faster.

The first thing I notice is that you have cat and temporary files everywhere. The programs you are piping cat output to can take file name arguments. Plus, writing to disk is one of the slowest things you can do to move data around. For example, all this

cat R*op.txt > R.total.op.txt
awk '{if( (length($8)>9) || ($8 ~ /^AAA/) ) {print $0}}' R.total.op.txt > temp && mv temp R.total.op.txt

cat S*op.txt > S.total.op.txt
awk '{if( (length($8)>9) || ($8 ~ /^AAA/) ) {print $0}}' S.total.op.txt > temp && mv temp S.total.op.txt

cat R.total.op.txt S.total.op.txt | awk '{print $4}' | sort -k1,1 | awk '!x[$1]++' > genes.txt
rm *total.op.txt

can be replaced with the following with only one file write

awk '{if( (length($8)>9) || ($8 ~ /^AAA/) ) {print $0}}' R*op.txt S*op.txt | awk '{print $4}' | sort -k1,1 | awk '!x[$1]++' > genes.txt

Now, for the loop you marked

for j in R1_with-genename R2_with-genename S1_with-genename S2_with-genename
do
    for i in `cat genes.txt`
    do
        cat $j'.op.txt' | grep -w $i >> $j'_'$i'_gene.txt'
    done
done

Try changing this to (with some better names)

for file in R1_with-genename R2_with-genename S1_with-genename S2_with-genename
do
    while read -r line
    do
         grep -w $line $file'.op.txt' >> $file'_'$line'_gene.txt'
    done < genes.txt
done

This will read the file line by line instead of all at once with cat genes.txt. An important note is that this will give different results than your script if there are multiple words per line in genes.txt. To read word-by-word, use

for file in R1_with-genename R2_with-genename S1_with-genename S2_with-genename
do
    while read -r line
    do
        for word in $line
        do
            grep -w $word $file'.op.txt' >> $file'_'$word'_gene.txt'
        done
    done < genes.txt
done

If genes.txt is much larger than the R1_with-genename, etc., files, then reversing the order of the loops could help by only reading the larger file once.

while read -r line
do
    for file in R1_with-genename R2_with-genename S1_with-genename S2_with-genename
    do
         grep -w $line $file'.op.txt' >> $file'_'$line'_gene.txt'
    done
done < genes.txt

Using >> appends to the end of the file if the file exists. Is this what you want? If running the script should overwrite previous results, use a single >.

Also, since you delete the genes.txt file right afterward, you don't even have to create it.

awk '{if( (length($8)>9) || ($8 ~ /^AAA/) ) {print $0}}' R*op.txt S*op.txt | awk '{print $4}' | sort -k1,1 | awk '!x[$1]++' | while read -r line
do
    for file in R1_with-genename R2_with-genename S1_with-genename S2_with-genename
    do
         grep -w $line $file'.op.txt' >> $file'_'$line'_gene.txt'
    done
done


Is this the best way to check sorting (in recursion) without loops? Please let me know if there is a shorter way to  do it again without any kind of loops. Recursion only.

static bool isSortedArray (int[] ar, int length)
{
    if (length == 0)
        return true;
    bool temp = isSortedArray(ar, length - 1);
    if (temp && ar[length - 1] <= ar[length])
        return true;
    else
        return false;
}

Based on the naming guidelines methods should be named using PascalCase casing.

If you evaluate two conditions connected by && you should always evaluate the faster one first. You can return the result of the condition directly.

This can be reached if you forget the temp variable and rearange the condition like

static bool isSortedArray (int[] ar, int length)
{
    if (length == 0)
    {
        return true;
    }

    return ar[length - 1] <= ar[length] && isSortedArray(ar, length - 1);
}

You should also take care of the posibility that ar will be null or that the number of elements is smaller than length.

Using braces {} for single if..else statements will make your code less error prone.

ar as a parametername is not well choosen. You shouldn't shorten parameter names.

isSortedArray should be better named IsArraySorted.

A construct like

if (condition)
{
    return true;
}
else
{
    // more code here
}

will always make the else redundant because if the condition is true it will never be reached.


Assign variables to save each directories from a folder I want to assign corresponding variables based on the name to save each directory from a folder, including its subfolders, so I can get access to the file in each folder easily. The code created by myself doesn't look concise. Any suggestions?

import os

modelFld = r'C:\model'

baseInputFld = [os.path.join(modelFld, x) for x in os.listdir(modelFld) if 'base' in x.lower()][0]

oilWellFld = [os.path.join(baseInputFld, x) for x in os.listdir(baseInputFld) if 'oilwell' in x.lower()][0]
diswaterWellFld = [os.path.join(baseInputFld, x) for x in os.listdir(baseInputFld) if 'waterwell' in x.lower()][0]
quarryFld = [os.path.join(baseInputFld, x) for x in os.listdir(baseInputFld) if 'quar' in x.lower()][0]
linearRefFld = [os.path.join(baseInputFld, x) for x in os.listdir(baseInputFld) if 'linear' in x.lower()][0]
txCountyFld = [os.path.join(baseInputFld, x) for x in os.listdir(baseInputFld) if 'county' in x.lower()][0]
tigerFld = [os.path.join(baseInputFld, x) for x in os.listdir(baseInputFld) if 'tiger' in x.lower()][0]

step1Fld = [os.path.join(modelFld, x) for x in os.listdir(modelFld) if 'step1' in x.lower()][0]
step2Fld = [os.path.join(modelFld, x) for x in os.listdir(modelFld) if 'step2' in x.lower()][0]
step3Fld = [os.path.join(modelFld, x) for x in os.listdir(modelFld) if 'step3' in x.lower()][0]
step4Fld = [os.path.join(modelFld, x) for x in os.listdir(modelFld) if 'step4' in x.lower()][0]
step5Fld = [os.path.join(modelFld, x) for x in os.listdir(modelFld) if 'step5' in x.lower()][0]
step6Fld = [os.path.join(modelFld, x) for x in os.listdir(modelFld) if 'step6' in x.lower()][0]

step1InputFld = [os.path.join(step1Fld, x) for x in os.listdir(step1Fld) if 'input' in x.lower()][0]
step2InputFld = [os.path.join(step2Fld, x) for x in os.listdir(step2Fld) if 'input' in x.lower()][0]
step3InputFld = [os.path.join(step3Fld, x) for x in os.listdir(step3Fld) if 'input' in x.lower()][0]
step4InputFld = [os.path.join(step4Fld, x) for x in os.listdir(step4Fld) if 'input' in x.lower()][0]
step5InputFld = [os.path.join(step5Fld, x) for x in os.listdir(step5Fld) if 'input' in x.lower()][0]
step6InputFld = [os.path.join(step6Fld, x) for x in os.listdir(step6Fld) if 'input' in x.lower()][0]

step1OutputFld = [os.path.join(step1Fld, x) for x in os.listdir(step1Fld) if 'output' in x.lower()][0]
step2OutputFld = [os.path.join(step2Fld, x) for x in os.listdir(step2Fld) if 'output' in x.lower()][0]
step3OutputFld = [os.path.join(step3Fld, x) for x in os.listdir(step3Fld) if 'output' in x.lower()][0]
step4OutputFld = [os.path.join(step4Fld, x) for x in os.listdir(step4Fld) if 'output' in x.lower()][0]
step5OutputFld = [os.path.join(step5Fld, x) for x in os.listdir(step5Fld) if 'output' in x.lower()][0]
step6OutputFld = [os.path.join(step6Fld, x) for x in os.listdir(step6Fld) if 'output' in x.lower()][0]

You should create functions to get rid of your repetitive logic. This is called DRY (Don't repeat yourself).

The first task you repeatedly accomplish is getting the first object in a base folder matching some string.

This can be accomplished using this function:

def find_in(root, string):
    return next(os.path.join(root, x) for x in os.listdir(root) if string in x.lower())

Instead of building the whole list and then taking only the first element, this is a generator and the first element of that generator is returned. This should save a bit of time and memory. To appreciate how much, try running these three commands and see how long they take:

>>> next(x**2 for x in xrange(100000000)
>>> next(x**2 for x in range(100000000)
>>> [x**2 for x in range(100000000)][0]

(The first one uses xrange, which is like range, but returns also an object like a generator, whereas the normal range returns the full list in Python 2.7.)

The second task you accomplish is the same thing as the first, but nested once. You want the first element in a stepX folder and then get the first object containinginput or output from that folder.

For this we can use the previous function and define a second function:

def find_nested(root, step, direction):
    return find_in(find_in(root, step), direction)

After this is done we can save the results in lists, instead of separate variables. Lists are for collections of similar objects.

In addition, there is an official Python style-guide, PEP8, which you should adhere to. It recommends using lower_case_with_underscores instead of camelCase. You should also avoid abbreviating variable names. Clear and more readable is better.

Final code:

import os

def find_in(root, string):
    """
    Returns the first object in the folder `root` which matches `string`.
    Case in-sensitive.
    """
    return next((os.path.join(root, x) for x in os.listdir(root) if string in x.lower()))

def find_nested(root, step, direction):
    """Returns the `direction` object from the `step` folder in `root`."""
    return find_in(find_in(root, step), direction)

if __name__ == "__main__":
    model_field = r'C:\model'
    root_folder = find_in(model_field, 'base')

    folders = ['oilwell', 'waterwell', 'quar', 'linear', 'county', 'tiger']
    fields = [find_in(root_folder, string) for string in folders]

    steps = ["step%i" % (i + 1) for i in range(6)]
    input_fields = [find_nested(root_folder, step, "input") for step in steps]
    output_fields = [find_nested(root_folder, step, "output") for step in steps]

In case not all stepX folders actually contain an input or output file/folder, this will raise a StopIteration. To avoid this, you can unwrap the list comprehension into an actual for loop and print the missing file:

input_fields = []
for step in steps:
    try:
        input_fields.append(find_nested(root_folder, step, "input"))
    except StopIteration:
        print "Folder", step, "has no `input` file."

And similar for the output_fields.


Improve my browser-detecting JavaScript Any suggestions how to make this code better? I'm novice in coding...))
In this code I'm want to show some hint depending of browser type on semi-transparent overlay. I'm detecting browser type (I'm need - Firefox, Chrome, IE 9 and higher), create all nessesary elements, append this elements to a document.body, and attach events.
To detect "document.ready" I'm use domready.js

// Perform browsers detection;
    var browser = {};
    (function() {
        browser.firefox = false;
        browser.chrome = false;
        browser.msie = false;

        var nAgt = navigator.userAgent;
        var verOffset;

        if ( (verOffset=nAgt.indexOf("MSIE"))!=-1 ) {
            // For IE we also need to check browser version, because hint will be shown only if version is 9 or higher;
            browser.version = parseInt( nAgt.substring(verOffset+5), 10 );
            if (browser.version >= 9 ) {
                browser.msie = true;
                browser.name = "msie";
                browser.hint = {
                    position: "fixed",
                    display: "none",
                    zIndex: "101",
                    bottom: "50px",
                    left: "35%"
                };
            }
        }
        else if ( (verOffset=nAgt.indexOf("Chrome"))!=-1 ) {
            browser.chrome = true;
            browser.name = "chrome";
            browser.hint = {
                position: "fixed",
                display: "none",
                zIndex: "101",
                bottom: "0",
                left: "0"
            };
        }
        else if ( (verOffset=nAgt.indexOf("Firefox"))!=-1 ) {
            browser.firefox = true;
            browser.name = "firefox";
            browser.hint = {
                position: "fixed",
                display: "none",
                zIndex: "101",
                top: "0",
                right: "10px"
            };
        }
    })();

    domready(function() {
        if (browser.firefox || browser.chrome || browser.msie) {
            // Create "overlay" element
            var overlay = document.createElement("div");
            overlay.setAttribute("id", "hintOverlay");
            var overlayStyles = {
              width: "100%",
              height: "100%",
              background: "gray",
              opacity: "0.75",
              zIndex: "100",
              position: "fixed",
              top: "0",
              left: "0",
              display: "none"
            };
            for(var s in overlayStyles){
                if (overlayStyles.hasOwnProperty(s)) {
                    overlay.style[s] = overlayStyles[s];
                }
            }
            overlay.addEventListener("click", function() {
                this.style.display = "none";
                document.getElementById("hintElem").style.display = "none";
            });
            document.body.appendChild(overlay);

            // Create "hint" element
            var hintElem = document.createElement("img");
            hintElem.setAttribute("src", "images/" + browser.name + ".png");
            hintElem.setAttribute("id", "hintElem");
            for(var s in browser.hint) {
                if (browser.hint.hasOwnProperty(s)) {
                    hintElem.style[s] = browser.hint[s];
                }
            }
            document.body.appendChild(hintElem);

            // Attach click event to all links that have "download" in name
            var links = document.getElementsByTagName("a");
            for(var i = 0; i < links.length; i++) {
                if(links[i].getAttribute("id").indexOf('download') == 0) {
                    links[i].addEventListener("click", function() {
                        document.getElementById("hintOverlay").style.display = "block";
                        document.getElementById("hintElem").style.display = "block";
                    });
                }
            }
        }
    });

"Browser-sniffing" is bad, usually because the methods used to detect them are usually "spoofable" and unreliable

User Agent string can be spoofed (changed) and can't be relied on. In fact, vendors change UAs to tell websites they are another browser, some for valid purposes. I think it was IE10 who added a "Mozilla" to their UA to tell websites to load non-IE versions of pages, with confidence that the newer IE will render "unhacked"/not modified for IE pages without breaking.

Even if you UA strings are fixed, you can't keep track of the 70+ (I think it was 70+, or something with a 7) different browsers in the wild today. Yes, there are browsers other than the major 5, like mobile browsers, command-line browsers, headless browsers. And don't forget to count the older versions, do still use them.

Recently, developers use feature testing instead, to check if certain features are available in the browser. Modernizr is an example of such frameworks.


First attempt at a pure JavaScript slider I'm working on this pure JS slider. It's quite a basic one, but it's what I need. I would like your feedback on it and any improvements/features that I'm missing.

HTML

Slide 1

Slide 2

Slide 3

Slide 4

Slide 5

    <button class="controls" id="previous">&lt;</button>
    <button class="controls" id="next">&gt;</button>

JS

var currentSlide = 0;
var slides = document.querySelectorAll("#slides .slide");
var controls = document.querySelectorAll(".controls");
var next = document.getElementById("next");
var previous = document.getElementById("previous");

for (var i = 0; i < controls.length; i++) {
    controls[i].style.display = "inline-block";
}

function goToSlide(n) {
    slides[currentSlide].className = "slide";
    currentSlide = (n + slides.length) % slides.length;
    slides[currentSlide].className = "slide showing";
}

function nextSlide() {
    goToSlide(currentSlide + 1);
}

function previousSlide() {
    goToSlide(currentSlide - 1);
}

next.onclick = function() {
    nextSlide();
};
previous.onclick = function() {
    previousSlide();
};

CSS

/*
Essential - Core
*/
#slides {
    position: relative;
    height: 300px;
    padding: 0px;
    margin: 0px;
    list-style-type: none;
}

.slide {
    position: absolute;
    left: 0px;
    top: 0px;
    width: 100%;
    height: 100%;
    opacity: 0;
    z-index: 1;
    -webkit-transition: opacity 1s;
    -moz-transition: opacity 1s;
    -o-transition: opacity 1s;
    transition: opacity 1s;
}

.showing {
    opacity: 1;
    z-index: 2;
}

/*
Non-essential - Styles
*/

.slide {
    font-size: 40px;
    padding: 40px;
    box-sizing: border-box;
    background: #333;
    color: #fff;
}

.slide:nth-of-type(1) {
    background: red;
}

.slide:nth-of-type(2) {
    background: orange;
}

.slide:nth-of-type(3) {
    background: green;
}

.slide:nth-of-type(4) {
    background: blue;
}

.slide:nth-of-type(5) {
    background: purple;
}

.controls {
    color: #fff;
    font-size: 40px;
    cursor: pointer;
}

.controls:hover {
    color: #333;
}

.container {
    position: relative;
}

.container button:nth-of-type(1) {
    position: absolute;
    left: 0;
    top: 50%;
    z-index: 10;
}

.container button:nth-of-type(2) {
    position: absolute;
    right: 0;
    top: 50%;
    z-index: 10;
}

The slider must be responsive. However, I didn't add any "responsiveness" code to it, because I believe that can be handled by CSS. Am I wrong?

Here's the JSFiddle

I would suggest wrapping this whole thing in an IIFE such that you can modularize all of your slide show functionality into its own scope.

That might look like this:

(function() {
    // your javascript code
}())

This would prevent you from defining a bunch of variables in global scope which could interact negatively with other javascript on a page where you want to insert the slide show.

I agree with other answer about removing styling for .controls into CSS.  You don't use this DOM collection anywhere else in your code, so why have it in JS at all?

You might consider separating the logic for hiding/showing slides into a separate method from goToSlide(). This allows goToSlide() to just focus on setting current slide index and calling appropriate hide/show methods.

I worry about writing to className as you could clobber any other classes that had been applied to a slide element. Since it seems like your code is really only worried about adding/removing showing, consider accessing classList on the element to add/remove this class only.

I would probably place the logic related to "wrapping" index values into the next/previous convenience function and let goToSlide() do one thing only - show the slide index as given.

Consider throwing an error for an illegal slide index passed to goToSlide(), since this function is exposed publicly.

If you truly want to make this code re-usable, you may want to consider making this functionality into a proper "class" and internalize all references to state and DOM bindings.  This would allow you to place multiple instances of these slide shows onto a page at once.

Putting it all together might yield something like:

// you could have this class defined in an externally included file
function Slider(slideSelector, nextId, prevId, toggleClassName) {
    this.currentIndex = 0;
    this.slides = querySelectorAll(slideSelector);
    this.next = getElementById('#' + nextId);
    this.prev = getElementById('#' + prevId);
    this.toggleClassName = toggleClassName;

    this.next.onclick = function() {
        this.nextSlide();
    }
    this.prev.onclick = function() {
        this.prevSlide();
    }
}

Slider.prototype.goToSlide = function(idx) {
    this.validateIndex(idx);
    this.currentIndex = idx;
    this.hideAllSlides();
    this.showSlide(idx);
}

Slider.prototype.nextSlide = function() {
    var idx = this.currentIndex + 1;
    if (idx === this.slides.length) {
        idx = 0;
    }
    this.goToSlide(idx);
}

Slider.prototype.prevSlide() = function() {
    var idx = this.currentIndex - 1;
    if (idx < 0) {
        idx = this.slides.length - 1;
    }
    this.goToSlide(idx);
}

Slider.prototype.hideAllSlides = function() {
    this.slides.forEach(function(slide) {
        slides.classList.remove(this.toggleClassName);
    });
}

Slider.prototype.showSlide = function(idx) {
    this.validateIndex(idx);
    this.slides.item(idx).classList.add(this.toggleClassName);
}

Slider.prototype.validateIndex = function(idx) {
    if(!Number.isInteger(idx)) {
        throw new TypeError('Non-integer value passed');
    }
    if ((idx > this.slides.length - 1) || idx < 0) {
        throw new RangeError('Out of range index value: ' + idx);
    }
}

// following code could then be in your document ready handler
// you could instantiate any number of sliders
// each which could operate independently of each other
var slider1 = new Slider('#slider1 .slides', 'next1', 'prev1', 'showing');
var slider2 = new Slider('#slider2 .slides', 'next2', 'prev2', 'showing');

Notes:

You could make ES6 class as well, but I did not show this, as I see nothing in your original code to make me think you are working in ES6.
Taking class approach should eliminate need for IIFE around slider code, in that you have already encapsulated your logic/state.
I have added index value validation, as given that the goToIndex() method could theoretically be called form anywhere in code that has access to the object in it's scope, it becomes more important to make sure object can not be put into a bad state.


Registration and Login System for Application I am developing a social platform and currently just completed the registration and login PHP pages. The application is an android application
that authenticates against a MySQL DB. I am seeking critiques on how "safe" and correct my PHP pages are that I am authenticating against, IE: Am I storing and authenticating passwords in a safe manner. This is my first try so I am up for changes.

Registration Page

<?php
define('HOST', 'localhost');
define('USER', 'admin');
define('PASS', 'password');
define('DB', 'database');
$con = mysqli_connect(HOST, USER, PASS, DB);

$username = $_POST["username"];
$password = $_POST["password"];
$email = $_POST["email"];
$age = $_POST["age"];
$id = $_POST["id"];
$hash = password_hash($password, PASSWORD_DEFAULT);

$sql = "INSERT INTO `table` (Username,Password,Email) values
       ('$username','$hash','$email')";

if (mysqli_query($con, $sql)) {
$response["success"] = 1;
$response["message"] = "Success";
die(json_encode($response));
} else {

$response["success"] = 0;
$response["message"] = "Invalid ";
die(json_encode($response));
}
mysql_close();
php
?>

Login Auth

<?php
define('HOST', 'localhost');
define('USER', 'admin');
define('PASS', 'password');
define('DB', 'database');
mysql_connect(HOST, USER, PASS);
mysql_select_db(DB)or die;
$username = $_POST["username"];
$password = $_POST["password"];

if (!empty($_POST)) {
if (empty($_POST['username']) || empty($_POST['password'])) {
    // Create some data that will be the JSON response
    $response["success"] = 0;
    $response["message"] = "One or both of the fields are empty .";
    die(json_encode($response));
}
$query = "SELECT * FROM `table` WHERE `Username` = '$username' LIMIT 1";
$result = mysql_query($query) or die(mysql_error());
$row = mysql_fetch_array($result);
$hash = $row["Password"];
if (password_verify($password, $hash)) {
    $response["success"] = 1;
    $response["message"] = "You have been sucessfully login";
    die(json_encode($response));
} else {

    $response["success"] = 0;
    $response["message"] = "invalid username or password ";
    die(json_encode($response));
}
} else {

$response["success"] = 0;
$response["message"] = " One or both of the fields are empty ";
die(json_encode($response));
}
mysql_close();

Any input or information to make this better/more functional is greatly appreciated.

Security

Yes, you are storing passwords safely. But your code is still incredibly insecure, as you are completely open to SQL injection.

You should use prepared statements, always, for all variable data.

[I also don't see any protection against CSRF; If you don't have it, you should add it, even for login forms; A user may for example force-login a victim into a fake account, and - if the victim does not pay attention - the attacker can thus see actions the victim takes on the site (which may potentially be interesting on a social platform and would definitely violate the privacy of your users)]

Misc

There are a lot of small mistakes and oddities in your code which makes it harder to read and maintain than necessary. Here's a small list, although I may have missed some.

Your indentation is off, making your code hard to read.
You mix mysql_ and mysqli_ functions, which will not work.
Do not use mysql_ functions; they are deprecated and will be removed in PHP7.
You have statements after you die (eg mysql_close();), which doesn't make sense.
Why die? There are few reasons to do it (one would be after a redirect), but in your cases it doesn't really make sense.
You have some duplication which you should remove (eg the One or both of the fields are empty error parts; You also have a lot more die(json_encode($response)); than you need).
I would use a lot more functions. It makes code more readable (because logical blocks now have a name) and more reusable and testable.
Why do you have POST values that you don't use (eg age, id)?
You define your db info more than once, which makes your code hard to maintain. It also makes hiding your db password harder.
You first access POST, and only afterwards ask if POST is empty or not (and you only actually check this in your login code, not your registration code).


Set an attribute to change an objects behavior, at specific times and for a specific period I have a program which I require to cease some of it's activities at specific times. This can change day-to-day, week-to-week.

The code below (working) is a test script. My program would check use the sleep_data() data set to check if the current date falls within any of the periods for a given day alongside the other updating of the program's state (every 2 sec). If so, it would call a method and the program sleeps a particular sub-set of it's activities.

There must be a better way.

 #!/usr/bin/env python3

import datetime
import logging
import time

from dateutil.parser import parse

logging.basicConfig(
    level=logging.DEBUG,
    format="%(asctime)s - %(levelname)s - %(message)s")
#logging.disable(logging.CRITICAL)

def sleep_data():
    """Return a dict of lists containing tuples of the periods in which to stand
       down selected bot's activities
    """
    #
    # parse("00:00") + oneday_delta = end of current day
    #
    oneday_delta = datetime.timedelta(hours=24)
    #
    # 1 = Monday, 5 = Friday etc. parse returns datetime.datetime object.
    #
    times = {
        1 : [(parse("21:55"), parse("23:15"))],

        2 : [(parse("21:55"), parse("23:15"))],

        3 : [(parse("21:55"), parse("23:15"))],

        4 : [(parse("21:55"), parse("23:15")),
             (parse("15:45"), parse("15:45:10"))],  # test use

        5: [(parse("20:55"), parse("00:00") + oneday_delta)],

        6: [(parse("00:00"), parse("00:00") + oneday_delta)],  # sleep all Sat

        7: [(parse("00:00"), parse("23:15"))],
        }
    return times

class Test:
    def __init__(self, sleep_times: dict):
        self.sleep_times = sleep_times
        self.trading = True

    def check_times(self, day: int, date_time: object):
        """If time within an exclusion period, set trading attrib to False"""
        exclusion_periods = self.sleep_times.get(day)

        for period in exclusion_periods:
            _from, to = period
            if date_time > _from and date_time < to:
                self.trading = False
                return
            self.trading = True

    def run(self):
        while True:
            date_time = datetime.datetime.now()
            day = date_time.isoweekday()
            self.check_times(day, date_time)
            print(self.trading)
            time.sleep(1)

if __name__ == "__main__":
    t = Test(sleep_data()).run()

Rather than making your times a dictionary indexed by an int just make it a list of tuples (start, end) and iterate through the list of trading times.
Input the list in order or sort it before starting so you can be assured that time[n].start < time[n].end and time[n].end < time[n+1].start.

For something as simple as your example it is clear enough what the code is doing.  It doesn't scale as you add more complexity to the decision to trade or not.  Consider different exchanges, different products, order types, etc.  Your single check_times function is going to get very complex.

To prepare for such coming complexity I would have a Trader class for each combination of things that could influence trading time, each with its own check_times function.  A Controller class would be responsible for choosing the appropriate Trader to handle things based on the attributes at hand.  Currently the only attribute you are looking at is the current time and so there would only be two Traders, and ActiveTrader and a DeactivatedTrader.

Again, depending upon the complexity of what is being implemented, these Trader's could be classes that descend from a common Trader class or could be duck-typed to have the same API.  Let Don't-Repeat-Yourself be your guide here.  If you are writing more Python code to set up the inheritance than you have saved by inheriting from a common ancestor you are doing it wrong.


Rails models for Users, Offers, Comments, Documents, and Reviews I want to simplify my Rails models, current looks like this:

class User < ActiveRecord::Base
  has_many :offers, dependent: :destroy
  has_many :reviews, dependent: :destroy
  has_many :comments, dependent: :destroy
end

class Offer < ActiveRecord::Base
  belongs_to :user
  has_many :documents, as: :documentable
  has_many :comments, dependent: :destroy
  has_many :reviews, dependent: :destroy
end

class Review < ActiveRecord::Base
  belongs_to :user
  belongs_to :offer
end

class Comment < ActiveRecord::Base
  belongs_to :user
  belongs_to :offer
end

class Document < ActiveRecord::Base
  belongs_to :documentable, polymorphic: true
end

As you can see, I have the same relations on Comment and Review models, but they have some difference in columns, what are the options do I have to minimize the amount of models?

First thing is if they are different treat them different. You can share code with help of interaction or service of some other way may be with active active_interaction. But if you really want to have less table then you may define one single model say Content and this will have a column content_type: integer with default value as 0 and all your other columns then convert the content_type to enum something like below

class Content < ApplicationRecord
  enum content_type: { comment: 0, rating: 1 }
end

this will help you to differentiate between comment and rating. With proper indexing it will make no issue.


Display a Text() with a foreground color based on a condition I decided to write a simple tab bar for macOS using Swift.
tabs.swift
import SwiftUI
import Foundation
import Combine

class ViewRouter: ObservableObject {
  @Published var currentView = "home"
}

struct Tabs: View {

  @ObservedObject var viewRouter = ViewRouter()

  var body: some View {
    GeometryReader { geometry in
      VStack {
        if self.viewRouter.currentView == "home" {
          Text("Home")
        } else if self.viewRouter.currentView == "settings" {
          Text("Settings")
        }
        Spacer()

        HStack {
          { () -> Text in
            if self.viewRouter.currentView == "home" {
              return Text("Home").foregroundColor(Color.blue)
            } else {
              return Text("Home")
            }
          }()
          .onTapGesture {
            self.viewRouter.currentView = "home"
          }

          { () -> Text in
            if self.viewRouter.currentView == "settings" {
              return Text("Settings").foregroundColor(Color.blue)
            } else {
              return Text("Settings")
            }
          }()
          .onTapGesture {
            self.viewRouter.currentView = "settings"
          }
        }
      }.edgesIgnoringSafeArea(.bottom).padding(20)
    }
  }
}

In ContentView.swift, I just display this using Tabs() in the ContentView struct.
In the HStack, I use two anonymous functions to give the text in the tab bar a blue color when that item is selected:
{ () -> Text in
  if self.viewRouter.currentView == "home" {
    return Text("Home").foregroundColor(Color.blue)
  } else {
    return Text("Home")
  }
}()
  .onTapGesture {
    self.viewRouter.currentView = "home"
  }

I also do this for the second item in the tab bar.
Is this the best (fastest and most concise) way to do this, or is there a better way?

There is a shorter and faster way, using ternary operators.
You could use
Text("Home").foregroundColor(self.viewRouter.currentView == "home" ? Color.blue: Color.black)

instead  of
{ () -> Text in
  if self.viewRouter.currentView == "home" {
    return Text("Home").foregroundColor(Color.blue)
  } else {
    return Text("Home")
  }
}()

However, with the advent of macOS Mojave 10.14 two years ago, users had an option to enable dark mode. So, unless you've explicitly disabled dark mode in your app, Color.black will blend in with the rest of your app bar. To prevent this, check for dark mode with UserDefaults.standard.object(forKey: "AppleInterfaceStyle"). If the user is using light mode, UserDefaults.standard.object(forKey: "AppleInterfaceStyle") will be nil, and if it's dark mode, it will be "Dark"
Text("Home").foregroundColor(self.viewRouter.currentView == "home" ? Color.blue: UserDefaults.standard.object(forKey: "AppleInterfaceStyle") == nil ? Color.black: Color.white)

As @BenLeggiero has suggested, this is a much shorter way to do the above:
Text("Home").foregroundColor(viewRouter.currentView == "home" ? .blue : .primary)


Stacks: Balanced Brackets Hacker Rank Here is the general goal of this exercise:

  For each string, print whether or not the string of brackets is balanced on a new line. If the brackets are balanced, print YES; otherwise, print NO.

Please comment as this was a job interview and I had 30 mins to give something working and the best performance wise.

using System;
using System.Collections.Generic;
using System.Linq;
using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace JobInterviewTests
{
    [TestClass]
    public class UnitTest1
    {
        [TestMethod]
        public void TestMethod1()
        {

            List<string> list = new List<string> {"({[]})", "][(]}})("};

            foreach (var barcketsList in list)

            {
                string expression = barcketsList;
                Stack<char> stack = new Stack<char>();
                char[] symbols = expression.ToCharArray();
                if (symbols.Length == 0)
                {
                    break;
                }
                bool flagPrint = false;
                for (int start = 0; start < symbols.Count(); start++)
                {
                    char temp = symbols[start];
                    if (temp == '[' || temp == '{' || temp == '(')
                    {
                        stack.Push(temp);
                    }
                    else
                    {
                        if (stack.Count == 0) //only closer no opening
                        {
                            Console.WriteLine("NO");
                            flagPrint = true;
                            break;

                        }
                        char top = stack.Peek();
                        if (temp == ']')
                        {
                            if (top != '[')
                            {
                                break;
                            }
                            stack.Pop();
                        }
                        else if (temp == '}')
                        {
                            if (top != '{')
                            {

                                break;
                            }
                            stack.Pop();
                        }
                        else // temp == ')'
                        {
                            if (top != '(')
                            {

                                break;
                            }
                            stack.Pop();
                        }
                    }
                }
                if (!stack.Any() && !flagPrint)
                {
                    Console.WriteLine("YES");
                }
                else if (!flagPrint)
                {
                    Console.WriteLine("NO");
                }
            }
        }
    }
}

Let's look at the code first

List<string> list = new List<string> { "({[]})", "][(]}})(" };

Poor choice of variable name. You picked expression just a little below, that is a good one. Why not use the plural form of it: expressions?

foreach (var barcketsList in list)

barcketsList contains a typo, not a big deal usually. However, you should pay some more attention for an interview.
bracket-s + List you really don't want hinting this is an array/collection/list twice.

foreach (var barcketsList in list)
// <- unnecessary white line
{

Don't leave white line just anywhere within a method in your code. They should be used to divide the logical blocks, eg: declaration/initialization vs logic vs return, logic#1 vs logic#2

string expression = barcketsList;
Stack<char> stack = new Stack<char>();
char[] symbols = expression.ToCharArray();

A temporary variable expression is not necessary here. In fact, both this and barcketsList is only used once. You can substitute one by the other.
Use var when you can, it makes the variable names aligned thus making scanning easier. The variable name itself or its right-hand side assignment should give enough clue usually, plus you can always mouse-over with intellisense.

for (int start = 0; start < symbols.Count(); start++)
{
    char temp = symbols[start];

Since you are just using the index (start) to get the char, you can use a foreach-loop here

if (stack.Count == 0) //only closer no opening
{
    Console.WriteLine("NO");
    flagPrint = true;
    break;

The use of flagPrint here to escape the default final output is a bit more of a hack. For instance, since you are using stack.Any() to check for balance, you can push the closing bracket and break out of the loop to achieve the same behavior. You can also take an another approach and extract a IsBalanced out of existing code and use return.

if (!stack.Any() && !flagPrint)
{
    Console.WriteLine("YES");
}
else if (!flagPrint)
{
    Console.WriteLine("NO");
}

If we were to keep the flag, this part can be reduced to:

if (!flagPrint)
{
    Console.WriteLine(!stack.Any() ? "YES" : "NO");
}

What is not in the code

IsBalanced is not in a separated method.
Lack of argument checking/error handling

Solution

public void CheckAllExpressionsAreBalanced()
{
    var expressions = new List<string> { "({[]})", "][(]}})(" };
    foreach (var expression in expressions)
    {
        //Console.WriteLine("Expression `{0}` {1} balanced.",
        //    expression,
        //    IsBalanced(expression) ? "is" : "is not");
        Console.WriteLine(IsBalanced(expression) ? "YES" : "NO");
    }
}

// Define other methods and classes here
public bool IsBalanced(string expression)
{
    // argument validation
    if (expression == null)
        throw new ArgumentNullException(nameof(expression));

    // definition
    var brackets = new[] { "()", "{}", "[]" };
    var pairedBrackets = brackets.ToDictionary(x => x[0], x => x[1]);

    // non-bracket characters doesn't affect the result,
    // so we can filter them out to make the flow simpler
    Func<char, bool> isBracket = c => pairedBrackets.ContainsKey(c) || pairedBrackets.ContainsValue(c);

    // implementation
    var stack = new Stack<char>();
    foreach (var c in expression.Where(isBracket))
    {
        var isOpening = pairedBrackets.ContainsKey(c);
        var pair = isOpening
            ? pairedBrackets[c]
            : pairedBrackets.First(x => x.Value == c).Key;

        if (isOpening)
        {
            stack.Push(c);
        }
        else
        {
            // closing bracket with empty stack
            if (!stack.Any())
                return false;

            // closing bracket doesnt match last opening bracket
            if (stack.Peek() != pair)
                return false;

            // matched
            stack.Pop();
        }
    }

    return !stack.Any();
}

EDIT: I skipped the requirements...
EDIT2: Updated IsBalanced to accept non-bracket characters input. As @HenrikHansen points out, it doesn't make sense to throw an exception.
EDIT3: Fixed issue with trailing brackets.


C - K&R getint() variation I'm currently learning C with "The C Programming Language" by K&R. At the moment I'm at the chapter about pointers (since I come from Java, they are new to me). In the book is an example code for a method getint(), which is supposed to read integers from input, but doesn't handle freestanding '+' and '-' correctly.

  As written, getint treats a + or - not followed by a digit as a valid representation of zero. Fix it to push such a character back on the input.

This is what I wrote:

int getint(int *pn)
{
   int c, sign;

   while(isspace(c = getch()))
      ;

   if(c == EOF)
      return EOF;

   if(!isdigit(c) && c != '+' && c != '-')
      return 0;

   sign = (c == '-') ? -1 : 1;

   if(c == '+' || c == '-')
      c = getch();

   if(!isdigit(c)) {
      ungetch(c);
      return 0;
   }

   *pn = 0;
   while(isdigit(c)) {
      *pn = *pn * 10 + c - '0';
      c = getch();
   }

   *pn *= sign;
   ungetch(c);

   return 1;
}

I modified the version inside K&R quite a bit and was wondering if I did a good job at doing so. My getint() is supposed to return EOF when end of file is reached, 0 if it failed to read the current character as an integer and 1 if it successfully wrote the int into *pn.

The K&R code begins with the follwing lines:

int c, sign;
while(isspace(c = getch()))
    ;

if(!isdigit(c) && c != EOF && c != '+' && c != '-') {
    ungetch(c);
    return 0;
}

And I think that the code will stuck if a non-digit, non-'+/-' and non EOF file character is found. Since it'll just unget this char and read it again in the next function call. Is this made on purpose? I left out the ungetch(), since I think that the function is supposed to just read the next char of input in the next function call...

OP's dropping of ungetc() is incorrect.  OP's thought of "it'll just unget this char and read it again in the next function call." assumes the next function call will be a repeated call to getint() - this is not warrented.  Since the calling code should inspect the return value and noting it is not 1, execute other code to consume the data in stdin.

if(!isdigit(c) && c != '+' && c != '-') {
  ungetch(c);  // not present in OP code, but present in K&R
  return 0;
}

The though being, if getint(int *pn) cannot read the int, leave stdin alone as much as practicable.  Other functionality could be to at least consume 1 character, but I find the first more in keeping with C I/O functions.

A quandary:

In other cases, OP's code below does "Fix it to push such a character back on the input" but IMO does not push back properly.  The sign character is not push-backed, only the digit.  Instead the sign is consumed by getint().

    c = getch()
...
sign = (c == '-') ? -1 : 1;
if(c == '+' || c == '-')
  c = getch();
if(!isdigit(c)) {
  ungetch(c);  // non-digit push-backed, but not sign.
  return 0;
}

But portably using ungetc() twice in a row is not guaranteed.

  One character of pushback is guaranteed. If the ungetc function is called too many times on the same stream without an intervening read or file positioning operation on that stream, the operation may fail.  C11dr §7.21.7.10 3

Since code really should "unget" 2 characters (+/- and the following non-digit), but it is not portable - what to do?  Since many systems do support pushing back more than 1 character with restrictions, consider attempting it.

Of course, all the consumed white-space characters are not push-backed either.  Yet pushing back the sign is prudent - many systems do this with scanf("%d", ...).

Checking the return value of ungetc() would discern success.  A slight variation follows:

int c;
int sign = EOF;

while(isspace(c = getch()))
  ;
if(c == EOF)
  return EOF;
sign = c;
if (sign == '+' || sign == '-')
  c = getch();

if (!isdigit(c)) {
  ungetch(c);  // Pedantic code would check this return value.
  if (sign == '+' || sign == '-') {
    // attempt to push-back sign
    if (ungetch(sign) == EOF) {
      ; // Operation failed
    }
  }
  return 0;
}

...

if (sign == '-') *pn = -(*pn);

Other code issues:

Code does not detect overflow.

Code unnecessarily overflows when scanning INT_MIN.  parse_int(char **s) offers a candidate solution.

Code does not differentiate between an EOF received from getch() due to end-of-file and input error.  Coding goal is "return EOF when end of file is reached".  OTOH, code has no specification concerning input error, so returning EOF for both conditions is reasonable.


FirstDuplicate Finder I read the solution to this question, which seems to be the same target. My solution is (I'm sure) not the quickest, but some test case is apparently taking longer than 4s. I did a test case with 10,000 random integers and it completed in just over .33 seconds, so I'm trying to find what would result in this code hanging.

Here are the requirements:

  Given an array a that contains only numbers in the range from 1 to
  a.length, find the first duplicate number for which the second
  occurrence has the minimal index. In other words, if there are more
  than 1 duplicated numbers, return the number for which the second
  occurrence has a smaller index than the second occurrence of the other
  number does. If there are no such elements, return -1.

  Example

  For a = [2, 1, 3, 5, 3, 2], the output should be firstDuplicate(a) =
  3.

  There are 2 duplicates: numbers 2 and 3. The second occurrence of 3
  has a smaller index than the second occurrence of 2 does, so the
  answer is 3.

  For a = [2, 4, 3, 5, 1], the output should be firstDuplicate(a) = -1.

My solution:

def firstDuplicate(a):
    currentdupe = -1
    dist = len(a)
    if len(a) == 0: return -1
    if len(a) == 1: return -1
    if len(a) == 2 and a[0] == a[1]: return a[0]
    else:
         #new list
         b = list(a)
         b.sort()

         #check each double in the sorted range
         for x in (range(len(b)-1)):
             if b[x] == b[x+1]:
                 #if distance is less than last found, use this one
                 if a[a.index(b[x])+1:].index(b[x]) < dist:
                     dist = a[a.index(b[x])+1:].index(b[x])
                     currentdupe = b[x]
         return currentdupe

Even in a worst case of 9,998 values of '1' and 2 values of '2', it still ran in just over .5s. When it runs a test case I get "Program exceeded the 4s time limit." How can this be?

Review

Adhere PEP8

functions and variable should be snake_case
Avoid multiple statements on one line

if len(a) == 0: return -1

Indentation matters, and an indentation of 4 spaces is the standard

You don't need all these guard clauses

if len(a) == 0: return -1
if len(a) == 1: return -1
if len(a) == 2 and a[0] == a[1]: return a[0]

The else: does nothing here, only adds an extra indentation level
When you need both the index and the item it is better to enumerate!

for x in (range(len(b)-1)):

For timing, see the Time complexity for python

You use some expensive operations while this can be done is a single loop

.sort() O(n log n)
.index() is O(n)

Note: Look for example at vnp's solution


Using Threading.Event to signal main thread I was reading on this page about using a thread, catching an exception, and notifying the calling thread.

The answer suggested Queue, but in the comments it states:

  Queue is not the best vehicle to communicate an error back, unless you
  want to have a full queue of them. A much better construct is
  threading.Event()

From the Python documentation:

  This is one of the simplest mechanisms for communication between
  threads: one thread signals an event and other threads wait for it.

I'm new to multi threading and I wanted to give it a try, so I came up with this trivial example. I'm unsure if I'm on the right track.

import threading

def dividebyzero(event,first,second):
    try:
        result = first / second
    except ZeroDivisionError:
        event.set()
        print("Can't divide by 0")
    else:
        print(result)

e = threading.Event()
t1 = threading.Thread(name='DivideByZero',target=dividebyzero, args=(e, 10, 0),)

t1.start()
t1.join()

if e.is_set():
    print("Error occurred in division thread")

Question:

Using threading.Event as shown in my example, is it the correct way to signal the main thread an error as occurred?
I'm not using event.wait() because it seems unnecessary, I call join() on the t1 thread, so when I call is_set() hopefully I'll get an accurate result. Is it mandatory to use event.wait() when using threading.event()
I understand that if I needed to raise more exceptions using an event probably isn't ideal, how would you know which exception was raise? In that situation would a Queue be better?

If I were to use event.wait() there is a chance the main thread could hang forever, because if the exception doesn't occur the event is never set.

I don't have an error or problems with the code, it works, I just want it reviewed to make sure I used event correctly.

Yes and no. Your approach works, and it is easy to understand. If, however, you want to keep track of what type of exception was raised, it could be improved. What if you have some super complex function that can raise 10 different exceptions, and you need access to the exception traceback if something goes wrong? You'd need 10 threading.Event instances. In that case, you should use a queue.Queue instead, which is thread-safe (untested snippet):

import queue
import threading

def dividebyzero(first, second, exc_queue):
    try:
        result = first / second
    except ZeroDivisionError as exc:
        exc_queue.put(exc)
    else:
        print(result)

excq = queue.Queue()
t = threading.Thread(target=dividebyzero, args=(10, 0, excq))
t.start()
t.join()
if excq.qsize() != 0:
    print(excq.get())

No, it is not mandatory to use threading.Event.wait(). Your observation is right: calling event.wait() might make the thread hang indefinitely.
goto 1;


Bundling CSS to speed up page loads My goal is to make web page loading faster by bundling all the CSS files together. The web browser will then only have to load only CSS files. This will only run when the application is starting.

void function setupApplication() {
  application.initialized           = now();

...

var renderCSSFile = application.GWASSETS & "combo.css";
if (FileExists(renderCSSFile)) FileDelete(renderCSSFile);

var renderCSS = "";
for (var myCSS in ListToArray("
    bootstrap, bootstrap-theme, bootstrap-jasny, font-awesome.min,
    datepicker,daterangepicker-bs3,
    datatables.bootstrap, datatables.colvis, datatables.fixedcolumns,
    select2, select2-bootstrap,
    main, scorecard"))  {

    if(FileExists(application.GWASSETS & trim(myCSS) & ".css")) {
        renderCSS &= FileRead(application.GWASSETS & trim(myCSS) & ".css");
        }
    }
FileWrite(renderCSSFile, renderCSS);

...

}

Is this the cleanest way to do this?

As suggested by Toni, you should use Gulp (or another task runner) to optimize the code shipped to the server and streamline your work. Among other things, task runners can concatenate files, minify/uglify code, transpile ES6 into ES5, serve your app locally, etc.

I strongly recommend Gulp (built on Node.js) because it allows you to create different tasks (such as the ones mentioned above) and stream them via a single pipeline, thus processing code fast and efficiently.

The main Gulp methods are task() (the task you want to create), src() (the source file(s)), pipe() (pipes the plugins that transform your code: pipe(plugin())) and dest()(where Gulp will send the processed file(s)). While Gulp provides a frame to create the "tasks pipeline", you need to use plugins to process/transform your code. Plugins are installed separately from Gulp as they are not part of the Gulp module itself (this means you need to install Gulp and all other plugins you want to use to create tasks).

I created a little tutorial below that will teach you the basics of Gulp. If you follow these steps you will create a simple sass task and have an idea what Gulp can do for you. Note: sass is a ruby gem. Make sure you have sass installed on your computer otherwise this will not work. You also need to have node.js and npm installed (https://nodejs.org/en/):

// First you need to install your node modules on the terminal:

$ npm install --global gulp-cli // gives you access to gulp on the command line

$ mkdir gulp-sass-demo && cd gulp-sass-demo // or any dir name you prefer. Make sure you don't name it "gulp-sass" though, because this is the name of a plugin we are going to use and npm will not allow you to install the plugin in a folder with the same name.

$ npm init // initiates npm on this directory--just hit enter for every question--at the end of the process you should have a "package.json" file, which holds important information about your node project

$ npm install --save-dev gulp gulp-sass // saves these two node modules as dev dependencies, meaning dependencies you need to develop the app but not to run it.

Note about how to install packages: If your project used "jQuery", for instance, you would need to install it as a dependency (not dev dependency) because your app would need jQuery's functionalities to work. In that case you would run this command $npm install --save jquery(this lists the module as a dependency on "package.json") or simply $npm install jquery (this doesn't list the module in package.json but makes it available as well).

After installing gulp and gulp-sass your package.json should look something like this:

{
  "name": "gulp-sass-demo",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "author": "",
  "license": "MIT",
  "devDependencies": {
    "gulp": "^3.9.1",
    "gulp-sass": "^3.1.0"
  }
}

That is it for the plugins installation. Now you should create a sass directory and one sass file that we will process via gulp-sass. So:

/gulp-sass-demo
  /sass
    styles.scss

In styles.scss, paste the following sass:

$font-stack: Helvetica, sans-serif;
$primary-color: #fff;

body {
  font: $font-stack;
  color: $primary-color;
}

Now you need to create a gulpfile.js, where you write your tasks:

// on the terminal make sure you are at the root of your project
$ touch gulpfile.js // creates the gulpfile--you can use vim, nano, or any other editor if you want to create and edit the file. i.e vim gulpfile.js

Open gulpfile.js in your IDE (you can do this on command line editors as well if you prefer), copy and paste the code below:

var gulp = require('gulp');
var sass = require('gulp-sass');

gulp.task('sass', function() {
  gulp.src('sass/*.scss')
  .pipe(sass())
  .pipe(gulp.dest('css'))
});

gulp.task('default', ['sass']);

This is what the code above does:

1) uses the node.js module loader system to require the "gulp" npm module and the "gulp-sass" npm module and assigns these modules to variables

2) uses these variables to create a "sass" task

3) gets all ".scss" files from the 'sass' directory

4) processes the files using the sass plugin

5) sends the resulting css from the processed .scss files to a css directory--if you don't have that directory Gulp creates it for you

6) registers the task 'sass' to run after the task 'default'. To run a gulp task you need to use the gulp command + the task name on the terminal. In this case you would run $ gulp sass since the name of your task is "sass". When you register a "default" task, you can simply run gulp without specifying any task name and gulp will look for tasks registered in the second argument (in this case ['sass'] only) and run that task for you. You can register other tasks in the array when you have them and have something like this: gulp.task('default', ['sass', 'js', 'html']);

So if you go to your terminal window and run $ gulp (the default task) it will then run the 'sass' task automatically. You can still run $ gulp sass if you want to run the task directly (watch Gulp messages as it runs the task).

Once the task is finished, go back to your IDE. Your directory should look like this now:

/gulp-sass-demo
  /css
    styles.css
  /node-modules // the modules you installed via npm
  /sass
    styles.scss

Gulp just created a css file that you can link on your HTML.
Your styles.css should look like this:

body {font: Helvetica, sans-serif;
      color: #fff; }

There is much more to Gulp but I hope this provides a good introduction to it.


Creating and populating a generic list I would like to know if this is being done correctly.

using System;
using System.Collections;
using System.Collections.Generic;
using System.Data;
using System.Diagnostics;
using System.Linq;
using System.Text;

public class InnerJoinTables
{

    public List<InnerJoinTable> Items = new List<InnerJoinTable>();

}

public class InnerJoinTable
{
    public int ID { get; set; }
    public string RightTable { get; set; }
    public string LeftTable { get; set; }

    public InnerJoinTable(int ID, string RightTable, string LeftTable)
    {
        this.ID = ID;
        this.RightTable = RightTable;
        this.LeftTable = LeftTable;
    }
}

Calling code:

InnerJoinTables m = new InnerJoinTables();

m.Items.Add(new InnerJoinTable(1, "A", "B"));

The code will work, so from that point of view, it is correct.

But there are also some issues with it:

I don't see any reason for the InnerJoinTables class to exist. You should just use List<InnerJoinTable> directly in the calling code. That's unless there's something else going on that you're not showing.
Public fields should not be used. Use a property instead.


WP/PHP getting API data and using hook Below are the two things I'm trying to solve, logically. This is just a proof of concept; and I'm not implementing this on real data nor via full plugin at this time. Just looking at logic and syntax solutions.

Below are the (2) requirements.

1.) Given an API route for retrieving a specific record, do the following:

Get the id param
Return a 404 if the record isn't found
Return a response with the record if it is found

Route format:

/publishers/(?P<id>\d+)

/publishers/397

2.) Add a WordPress hook to determine the value of the visible property on some item. Then use that hook elsewhere to change the value to false;

Below is what I have so far. Which appears correct; but just would like to see if there is a more optimal way, and/or this is still best approach. Just some feedback on my code solution; via the above two points.

My Code solutions:

<?php
function get_publisher( \WP_REST_Request $request ) {
    $request = json_decode( $request );
    $publisher_id = $request[ 'ID' ];
    if ( empty( $publisher ) ) {

        $response = [
            'status'    => 404,
            'data'      => [
                'errors' => [
                        'general' => 'Sorry, no record was found.'
                ],
            ],
        ];
        return json_encode( $response );
    }
    $response = [
        'status'    => 200,
        'data'      => [
            'publisher' => $publisher,
        ]
    ];
    return json_encode( $response );
}
function modify_item( $item ) {
    $item->visible = apply_filters( 'item_visibility', true );

    return $item;
}
function hide_item() {
    return false;
}
add_filter( 'item_visibility', 'hide_item' );

You will need to settle on the variable name containing $request[ 'ID' ];. At the moment, you are using $publisher_id and $publisher ...which of course is a script fouling typo. I'd probably avoid declaring the variable, because I don't find a declarative coding style to be very beneficial in this case.
You could use a different syntax to declare your array to be less verbose. For example:

 $response['status'] = 404;
 $response['data']['errors']['general'] = 'Sorry, no record was found.';

By writing an if-else, you can avoid rewriting json_encode().

..I don't WP so I cannot say if there any native techniques to leverage.

The first function could look like this:

function get_publisher(\WP_REST_Request $request) {
    $request = json_decode($request);
    if (empty($request['ID'])) {
        $response['status'] = 404;
        $response['data']['errors']['general'] = 'Sorry, no record was found.';
    } else {
        $response['status'] = 200;
        $response['data']['publisher'] = $request['ID'];
    }
    return json_encode($response);
}


Container that holds objects of various types, and can be searched for an object convertible to a given type I built a container that can hold objects of various types, and look up an object convertible to a given type.  I wrote something about my intended use-case here:

https://stackoverflow.com/questions/58562173/typesafe-way-to-provide-enriched-dependencies-to-derived-classes-in-c

Basically I want to be able to throw a bunch of dependencies onto a pile and then grab the exact version I want from calling code -- for instance, if I just need an IWidget, I can do

IWidget * widget = dependencies.get<IWidget *>();

but if the underlying widget is actually a FancyGreenWidget then I can do

FancyGreenWidget * widget = dependencies.get<FancyGreenWidget *>();

and if it's not fancy and green then I'll get a helpful runtime error during program initialization rather than a surprise segfault sometime while the program is running.

I'm not really familiar with C++17, but it seemed like I needed a bunch of C++17 features to implement my use-case.  I feel like someone who writes modern C++ on a daily basis can probably glance at this and tell me several things I'm doing wrong.  In particular I suspect there's a much cleaner way to write the visitor in the get<T>() method.

template<class... Types>
  class VariantContainer
  {
    private:

      std::vector<std::variant<Types...>> m_values;

    public:

      template<class T>
      void add(const T &t)
      {
        static_assert(std::disjunction_v<std::is_same<T, Types>...>);

        m_values.push_back(t);
      }

      template<class T>
      T get()
      {
        for(int i = 0; i < m_values.size(); i++)
        {
          std::optional<T> result = std::visit(
            [](auto&& arg) {
              using Targ = std::decay_t<decltype(arg)>;

              if constexpr(std::is_convertible_v<Targ, T>)
              {
                return std::optional<T>(static_cast<T>(arg));
              }
              else
              {
                return std::optional<T>();
              }
          },
          m_values[i]);

          if(result.has_value()) { return result.value(); }
        }

        std::string message = "Could not find member of type ";
        message += abi::__cxa_demangle(typeid(T).name(), 0, 0, nullptr);

        throw std::runtime_error(message);
      }
  };

Sample usage:

    VariantContainer<Base *, Derived1 *, Derived2 *> v;

    Derived2 d;

    v.add(&d);

    Base * b = v.get<Base *>(); // returns a pointer to d

    Derived1 * d1 = v.get<Derived1 *>(); // ERROR

    Derived2 * d2 = v.get<Derived2 *>(); // returns a pointer to d

I'm also in the market for a more descriptive name for this class if anyone has any ideas.

You said you implemented

  a container that can hold objects of various types, and look
  up an object convertible to a given type

This idea can be simplified and generalized: we want to

  look up an object convertible to a given a type in a sequence of variants

We can focus on the get function and strip everything else.  The container does not have to be a std::vector<std::variant> — we can use iterators:

template <typename T, typename ForIt>
T get_convertible(ForIt first, ForIt last);

In the code, you want to find an element that is convertible to T, and throw an exception if such an element is not found.  You used std::optional to conditionally return the visited element.  This overhead can be eliminated:

template <typename T, typename ForIt>
T get_convertible(ForIt first, ForIt last)
{
    auto it = std::find_if(first, last, [](const auto& variant) {
        return std::visit([](const auto& value) {
            using U = std::decay_t<decltype(value)>;
            return std::is_convertible_v<U, T>;
        }, variant);
    });

    if (it == last) {
        throw std::runtime_error{"..."};
    } else {
        return std::visit([](const auto& value) -> T {
            using U = std::decay_t<decltype(value)>;
            if constexpr (std::is_convertible_v<U, T>)
                return value;
        }, *it);
    }
}

(live demo, includes code to disable return warning)


Another way to find the nearest neighbor points in a 2D plane I was inspired by another question to post another method of finding the two points in a plane that are closest to each other1.

This one is a line-sweep algorithm. It works roughly like this:

Sort the points based on their X coordinates.
Take two left-most points. They give us our first guess at the shortest distance (call it D).
Insert those two points into a set that's sorted based on Y coordinates. This collection forms a vertical "band" of points whose X coordinates are within D units of the X coordinate of the current point.
Consider the next point to the right of those currently in the "band" as the current point.
Trim the band to remove points more than D units away in the X dimension from the current point.
Find the points in the band that are vertically within D units of the Y coordinate of the current point.
Look through the points in that rectangle (maximum of 6) to see if any is closer than D units from the current point.
If so, record the points and distance.
Repeat from step 4 for remaining points.

Here's the code:

#include <iostream>
#include <cmath>
#include <algorithm>
#include <vector>
#include <set>
#include <cassert>

struct point {
    double x, y;

    // Used by the `set<point>` to keep the points in the band
    // sorted by Y coordinates.
    bool operator<(point const &other) const {
        return y < other.y;
    }

    friend std::ostream &operator<<(std::ostream &os, point const &p) {
        return os << "(" << p.x << ", " << p.y << ")";
    }
};

double dist(point const &a, point const &b) {
    return std::hypot(a.x - b.x, a.y - b.y);
}

// We're going to modify the input we receive, so we receive it by value.
// If we knew that the source was going to be modifiable, we could receive
// by non-const reference instead;
std::pair<point, point> min_dist(std::vector<point> points) {

    std::sort(points.begin(), points.end(),
        [](point const &a, point const &b) {
        return a.x < b.x;
    }
    );

    // First and last points from `point` that are currently in the "band".
    auto first = points.cbegin();
    auto last = first + 1;

    // The two closest points we've found so far:
    auto first_point = *first;
    auto second_point = *last;

    std::set<point> band{ *first, *last };

    double d = dist(*first, *last);

    while (++last != points.end()) {
        while (last->x - first->x > d) {
            band.erase(*first);
            ++first;
        }

        auto begin = band.lower_bound({ 0, last->y - d });
        auto end = band.upper_bound({ 0, last->y + d });

        assert(std::distance(begin, end) <= 6);

        for (auto p = begin; p != end; ++p) {
            d = std::min(d, dist(*p, *last));
            first_point = *p;
            second_point = *last;
        }

        band.insert(*last);
    }
    return std::make_pair(first_point, second_point);
}

int main() {
    std::vector<point> points{
        {1, 1},
        {17, 9},
        {23, 23},
        {3, 3},
        {100, 100},
        {200, 200},
        {24, 24},
        {300, 300}
    };

    auto r = min_dist(points);

    std::cout << "Closest points: " << r.first << ", " << r.second
            << ". Distance = "<< dist(r.first, r.second);
}

1. In this case, I've used Euclidian distance, but another metric such as Manhattan distance could be used as well.

Your algorithm looks (and works) great, but I found two problems with part of code responsible for finding closest pair within band.

Wrong pair of points is returned as result

The closest distance in d variable is correctly updated, but first_point and second_points are updated even if d < dist(*p, *last), which leads to returning wrong pair of points. You can reproduce the problem on following set of points: {-1, 10}, {0, 10}, {0, 9}, {0, 8}, {0, 7}, {0, 6}, {0, 5}, {0, 4}, {0, 3}, {0, 2}, {0, 1}, {0, 0}, {0.99, 0.99}

You can fix that by replacing your for loop with the following one:

for (auto p = begin; p != end; ++p) {
    if (d > dist(*p, *last)) {
        first_point = *p;
        second_point = *last;
        d = dist(first_point, second_point);
    }
}

for loop is not executed when there is only one point in band

If there is only one element in band, then both lower_bound and upper_bound return iterator pointing to the same element in band, which means that for loop will not be executed, because the condition p != end is not met. You can reproduce the problem on following set of points: {-1, 0}, {0 ,0}, {0.1, 0.1}


Extracting single files from a tarball (spreadsheets) I've created a program that will pull spreadsheets out of a large tarball.

First it specifies the file name through the "folio" number which is given as an argument when the program is called. From there it runs a bash command that will list a text file containing the information needed to find the tarball, the information needed is a date. It then collects the information finds the tarball, runs another bash command to unarchive the spreadsheet from the tarball, and then moves the spreadsheets.

I would like some critique on my work. Are there better ways to extract from a tar? Would this be better as a module then a class?

I know it doesn't delete the directory that is created from extracting the spreadsheets, mostly because I don't trust myself enough to remove a directory called home.

#!/usr/local/bin/ruby

require 'rubygems'
require 'colored'
require 'fileutils'

class ArchiveManager

  attr_accessor :folio_num

  def initialize(folio_num)
    @folio_num = folio_num
    @src_dir = "/home/<dir>/<user>/<dir>/home/common/4000_UW_spreadsheets/#{folio_num}.ods"
    @dst_dir= "/home/<dir>/<user>/<dir>"
  end

  def num_check
    if folio_num =~ /^\d{4,7}$/
      search_for_spreadsheet
    else
      puts
      puts "                  Invalid Folio length".red.bold,
       "        USAGE: unarchive_ss <folio number 4-7 digits>".red.bold
      puts
    end
  end

  def search_for_spreadsheet
    location = FileUtils.chdir("/home/<dir>/<user>/<dir>")
    digits = `grep -lr #{@folio_num}.ods *.txt`
    file_name = digits.match(/\d+/).to_s
    puts "Spreadsheet found in UW_archive_#{file_name}.tgz".green.bold
    extract(file_name, location)
  end

  def extract(file_name, location)
    if File.exist?("#{@dst_dir}/#{@folio_num}.ods")
      found_error = raise SecurityError.new("File: #{@folio_num} exists within directory: #{@dst_dir}; will not continue..")
      abort("File exists in directory, either use file as parachute, or get rid of it..".red.bold)
    else
      tar_location = "home/common/4000_UW_spreadsheets"
      `tar -x -f UW_archive_#{file_name}.tgz #{tar_location}/#{@folio_num}.ods`
      move(tar_location, location)
    end
  end

  def move(tar_location, location)
    if File.exist?("#{@src_dir}")
      puts "File extracted, and being moved.".green.bold
      sleep(1)
      FileUtils.mv("#{@src_dir}", "#{@dst_dir}")
      puts "File: #{@folio_num}.ods, is now located in directory path: #{@dst_dir}".blue.bold
      puts "Remove created directories manually".red.bold
    else
      puts "Unable to locate file: #{@folio_num}.ods; source not found.".red.bold
      src_error = raise LoadError.new("Unable to find file: #{@folio_num}.ods; in directory #{@src_dir}")
    end
  end
end

ss = ArchiveManager.new(ARGV[0])
ss.num_check

Example of usage: File 2715015

Your main questions

  Are there better ways to extract from a tar?

Using shell commands, no. Well, with any modern version of tar, instead of tar -x -f archive file you could write simpler as tar xf archive file.

I don't know Ruby, but probably there exist libraries to extract files from archives. It would be better to use that.

The current script is a mixture of Ruby and Bash commands.
This is a fragile setup:

It requires two technologies to run
It requires understanding two technologies

The solution would be more robust if it was written either completely in Ruby or in Bash.

  I know it doesn't delete the directory that is created from extracting the spreadsheets, mostly because I don't trust myself enough to remove a directory called home.

You could recursively remove the empty parent directories. That should be safe.

Naming

Looking at this usage:

ss = ArchiveManager.new(ARGV[0])
ss.num_check

This doesn't really tell what's going to happen.
The program extracts a file from an archive.
This would look like a more natural usage:

archive_manager = ArchiveManager.new()
archive_manager.extract_file(ARGV[0])

Design

The naming issue brought to the fore some design issues.
The naming change I suggested earlier for more natural usage,
also forces further changes that will lead to better design:

Currently the ArchiveManager class is one-time use to extract a single file. That's not a natural way to use classes. Making the ArchiveManager reusable by adding a extract_file method with a file parameter is more natural.
Currently many methods of ArchiveManager depend on the folio_num stored in the class. Methods are easier to understand when their dependencies are more directly visible, for example as input parameters. It also makes them easier to test.


INI configuration handler Just to quickly explain what this is and what it does: it's a config handler that loads a file that has configuration data in INI format, key=value that sort of format. I then just store it all inside a double string dictionary and take items from the dictionary when I need them.

Questions:

Is my dispose pattern okay? Do I even need to dispose in the first place? I'm still learning about disposing and memory management.
Should I be locking on the dictionary?
Am I okay to use LINQ? I recently got told that LINQ slows down your code. Is there any better way to do this? Is the speed much different?

namespace Pitaya.Root.Core.Config
{
    internal class ConfigHandler : IDisposable
    {
        private static readonly ILogger Logger = LogManager.GetCurrentClassLogger();

        private Dictionary<string, string> _configItems;

        internal ConfigHandler()
        {
            _configItems = new Dictionary<string, string>();
            LoadConfig("/resources/config/config.ini");
        }

        internal void LoadConfig(string configFile)
        {
            try
            {
                if (!File.Exists(configFile) || FileUtilities.IsFileLocked(new FileInfo(configFile)))
                {
                    Logger.Error("Either your config file couldn't be found, or its open somewhere else.");

                    Console.ReadKey(true);
                    Environment.Exit(0);
                }

                _configItems = File.ReadLines(configFile).Where(IsConfigurationLine).Select(line => line.Split('=')).ToDictionary(line => line[0], line => line[1]);
            }
            catch (Exception exception)
            {
                Logger.Fatal("Error while loading config: " + exception.Message);
                Logger.Fatal(exception.StackTrace);

                Console.ReadKey(true);
                Environment.Exit(0);
            }
        }

        private static bool IsConfigurationLine(string line)
        {
            return !line.StartsWith("#") && line.Contains("=");
        }

        internal string GetConfigValueByKey(string key)
        {
            if (!_configItems.TryGetValue(key, out var value))
            {
                Logger.Warn("Missing configuration key `" + key + "`");
            }

            return value;
        }

        public void Dispose()
        {
            Dispose(true);
        }

        private void Dispose(bool disposing)
        {
            if (!disposing)
            {
                return;
            }

            _configItems.Clear();
            _configItems = null;
        }
    }
}

I recently got told that LINQ slows down your code.

Tell them they are wrong.

  Is there any better way to do this?

No. You first code for convenience and pretty code.

  Is the speed much different?

It doesn't matter unless you need to optimize it for speed but you do this only for parts that you have and can measure. You don't optimize code that you believe might by slow because someone heard from someone who heard from someone else that something might be slow.

Your ConfigHandler doesn't have to be IDisposable. You are not using any resources and clearing a dictionary isn't doing anything useful.

if (!File.Exists(configFile) || FileUtilities.IsFileLocked(new FileInfo(configFile)))

You should check only one condition at a time and log a precise message.

  "Either your config file couldn't be found, or its open somewhere else."

You are not helping yourself with this message. Each time any of these occurs you'll need to check both cases instead of only one.