Bash script to encode images in base64 and generate a CSS file from them These are the first lines of code I've ever written. I've been interested in the idea of learning to program for quite a while, but never really pulled the trigger, and now I've been playing around with some HTML & CSS and thought this might be a good starter project. So after a couple of days of furious googling, I managed to cobble this script together.

What I'm mostly curious about is the overall logic and structure of the code. The whole script grew organically from me looking up the Bash constructs and figuring out how to combine them to do what I want, so I don't know how sensible it is from the programming standpoint.

The idea is to keep the script in my project's folder and use relative paths, so I can just run it when I need to regenerate the CSS.

#!/bin/bash

# CSS ruleset template:
#
# .________::before { background-image: url(data:________;base64,__________); }
#  filename                                      mimetype        base64 str
#
# CSS class names and other identifiers can contain the characters
# A-Z, a-z, 0-9, hyphen, underscore and Unicode characters U+00A0 and higher.
# They cannot start with a digit, two hyphens or a hyphen followed by a digit.
# (https://www.w3.org/TR/CSS2/syndata.html#characters)
#
# url(), unquoted, requires escaping the characters ()'" and whitespace.
# url('') & url("") require escaping newlines and the quote used for quoting.
# (https://www.w3.org/TR/CSS2/syndata.html#uri)
#
# Base64 encoding uses characters A-Z, a-z, 0-9 and any two of these: +-.,:/_~
# Therefore, there's never need to escape anything, so no quotes are necessary.

css_path="icons.css" # relative to the script

cd "${BASH_SOURCE%/*}"

: > "$css_path"

for file in icons/*; do
    echo Processing "$file"

    filename="$(name_ext="${file##*/}"; echo "${name_ext%.*}")"
    ext="${file##*.}"

    shopt -s nocasematch
    case "$ext" in
        avif      ) mime="image/avif"   ;;
        bmp       ) mime="image/bmp"    ;;
        gif       ) mime="image/gif"    ;;
        jpg | jpeg) mime="image/jpeg"   ;;
        png       ) mime="image/png"    ;;
        svg       ) mime="image/svg+xml";;
        *         ) mime="unsupported"  ;;
    esac

    if [[ "$mime" = "unsupported" ]]; then
        unsupported+=("$file")
    elif [[ ! "$filename" =~ ^-?[A-Za-z_][A-Za-z0-9_-]*$ ]]; then
        invalid_class_name+=("$file")
    else
        base64str="$(base64 --wrap=0 "$file")"
        printf ".%s::before { background-image: url(data:%s;base64,%s); }\n" \
        "$filename" "$mime" "$base64str" \
        >> "$css_path"
    fi
done

if [[ -z "$unsupported" && -z "$invalid_class_name" ]]; then
    echo "All done!"
else
    if [[ -n "$unsupported" ]]; then
        printf "\n\n%s\n\n" "UNSUPPORTED FILES (SKIPPED)"
        printf "  %s\n"       "${unsupported[@]}"
    fi
    if [[ -n "$invalid_class_name" ]]; then
        printf "\n\n%s\n" "FILENAMES INVALID AS CSS CLASS NAMES (SKIPPED)"
        printf "  %s\n"   "Allowed characters: A-Z, a-z, 0-9, '-', '_'"
        printf "  %s\n\n" "Can't begin with: 0-9, '--', '-0' through '-9'"
        printf "  %s\n"   "${invalid_class_name[@]}"
    fi
fi
```

cd "${BASH_SOURCE%/*}"

: > "$css_path"

What happens if  cd fails?  Nothing good.  Make a habit of always, always testing the result of cd (and synonyms like pushd) for failure.   Bash's -e switch will promote all error exits to fatal script errors; a less extreme approach is simply:

cd … || exit 1

if [[ -z "$unsupported" && -z "$invalid_class_name" ]]

This is testing the first elements of these two arrays for zero length.  You want to test the lengths of the arrays instead.

When testing for zero, the use of arithmetic (( … )) is a nice touch: it returns truth for non-zero and false for zero.

When a conditional includes an else block, avoid negating the condition.  In other words: prefer if x then yes else no  over if not x then no else yes.

 if (( ${#unsupported[@]} || ${#invalid_class_name[@]} ))

filename="$(name_ext="${file##*/}"; echo "${name_ext%.*}")"
ext="${file##*.}"

This is awkward.  Most of the fault rests with Bash itself.  A regular expression is a little cleaner:

[[ $file =~ ([^/]*)\.([^.]*)$ ]] && filename=${BASH_REMATCH[1]} ext=${BASH_REMATCH[2]}

if [[ "$mime" = "unsupported" ]]; then

Quotes aren't needed around variables (or literal strings that lack whitespace) inside [[ … ]].

  printf ".%s::before { background-image: url(data:%s;base64,%s); }\n" \
  "$filename" "$mime" "$base64str" \
  >> "$css_path"

It's good practice to indent continuations.  In the specific case of printf, it can be helpful to align variables with their format-string placeholders.

unsupported+=("$file")

invalid_class_name+=("$file")

If these exist in the shell that invokes your script, their initial values will persist and muck things up.  Make a habit of zeroing variables you intend to append to (or increment, etc.).  At the same time, improve readability by expressly declaring your arrays as such:

declare -a unsupported=() invalid_class_name=()

: > "$css_path"

This could be annoying if the script fails.  Consider taking a backup of the output file before zeroing it, and restoring the backup on error exit.


RAII pattern for downgradable ReadWriteLock The example for downgrading ReentrantReadWriteLock in the Java documentation seems really unsafe when handling exceptions. I tried to write a simple class to simplify it. Do you see any cases where it can fail?

I'm looking for a review from a concurrency point of view, such as if this code may fail if an exception is thrown inside a nested lock before the downgrade.

import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReadWriteLock;

public class RWLockWrapper implements AutoCloseable {
    private final ReadWriteLock rwl;
    private Lock current;

    //RAII
    public RWLockWrapper(ReadWriteLock rwl) {
        this.rwl = rwl;
        this.current = rwl.writeLock();
        this.current.lock();
    }

    public void downgrade() {
        final Lock read = rwl.readLock();
        read.lock();
        try {
            current.unlock();
        } finally {
            current = read;
        }
    }

    @Override
    public void close() {
        current.unlock();
    }
}

To be used like this:

try (RWLockWrapper lock = new RWLockWrapper(rwl)) {
    //write locked

    lock.downgrade();

    //read locked
}

You didn't give a terrible amount of code to be reviewed, and I am not really proficient in concurrency and the like but I still got a few points I want to make.

But enough of introduction let's jump into the code:

public class RWLockWrapper implements AutoCloseable {

Wonderful, concise and clear. RWLockWrapper is a beatiful name for that class. You could have spelled it out, but that is definitely not much improvement in clarity.

I also like the design choice of implementing AutoCloseable. This makes your wrapper easy and obvious to use.

One big thing that's missing here: Documentation! I strongly suggest you write a little JavaDoc for this. It doesn't even need to be much, it should just explain the class in short and inform the user about the caveats of using it.

private final ReadWriteLock rwl;
private Lock current;

oh sweet joy! You made your ReadWriteLock read-only. I like that design choice for two reasons: You make clear that the Wrapper is usable for only one and exactly one Lock at a time, and additionally you cleanly make it impossible to do any different.

The name could be better, but it's a short class, so it's definitely not very detrimental to have a name like rwl.

As an aside: current could probably use a better name, but for the love of {higher entity of choice} I can't find a better one...

//RAII
public RWLockWrapper(ReadWriteLock rwl) {
    this.rwl = rwl;
    this.current = rwl.writeLock();
    this.current.lock();
}

The first thing that I really find not acceptable in your code is that comment.

It's utterly useless, and for me (who doesn't use locks all that much) uninformative and confusing. Again I strongly recommend you document what you are doing here.

As a no-experience user, I'd have some expectations to your class, and that you write-lock things, is definitely not the first thing I'd think :(

JavaDoc can really help a lot.

Additional minor nitpicks: I personally put a space between the name and the opening parens, I feel I can read that better.

public RWLockWrapper (ReadWriteLock rwl) {

Furthermore you are overqualifying current with this., which I don't like. I personally prefer to not use this wherever possible, but you may decide to do differntly so.

public void downgrade() {
    final Lock read = rwl.readLock();
    read.lock();
    try {
       current.unlock();
    } finally {
       current = lock;
    }
}

not much to say here. Again I'd prefer a little more whitespace, again missing documentation. Also you may want to mention in possible documentation, that the state of the Lock will always be locked read after calling this method.

Moving swiftly on. Where's the upgrade ()????

Confusion ensues

What, there is no upgrade?? I think you get it.
When you expose some downgrade (), I expect to also be able to upgrade ().

I don't really understand why you don't expose that method. I suspect you have no real need for it. I feel that upgrade/downgrade is a pair just like add/remove, create/destroy and similar.

That aside, moving swiftly on.

@Override
public void close() {
    current.unlock();
}

Hmmm.... Okay for the sake of the argument we assume, that calling unlock() will not throw any exceptions...

No. There is always the possibility for an execption to be thrown, even more so, when you don't really secure yourself against it...

But that aside, hypothetical question: What would happen if someone did the following

    RWLockWrapper myCoolLock = new RWLockWrapper(lockForTotallyImportantAndCriticalThing);
    //... do some important stuff.
}

Oh crap. Oh crap... Everything broke!!

I think you see what I am getting at. Given the case someone uses your wrapper wrongly, things can go wrong easily.

I suggest you consider adding:

@Override
public void finalize () {
    current.unlock();
}

Why that? To make sure, that your Lock does not stay locked when you can never unlock it again, because you lost it in a tangled mess of bits and bytes.

This should provide additional security to the wrapper even against non-proficient, blue-eyed or just plain {insert favourite insult, doubting intelligence} programmers.


Getting all (forward) permutations of a list of strings I have the following concise-ish (and working) approach to getting all the forward permutations of a list of string. So with the list:

  w = ["Vehicula", "Sem", "Risus", "Tortor"]

the results should be:

  ['Vehicula', 'Vehicula Sem', 'Vehicula Sem Risus', 'Vehicula Sem Risus Tortor', 'Sem', 'Sem Risus', 'Sem Risus Tortor', 'Risus', 'Risus Tortor', 'Tortor']

To do this, we loop through each element, and perform an inner loop that looks ahead and saves slices of the remaining elements to a result array.

w = ["Vehicula", "Sem", "Risus", "Tortor"]
results = []

i = 0
l = len(w)
while i < l:
    j, k = i, i + 1
    while k <= l:
        results.append(" ".join(w[j:k]))
        k = k + 1
    i = i + 1
print results

With Python, I always feel like I'm missing a trick, so I'm curios to see if there are any native Python functions that will make this more efficient?

Edit

I tested all three with timeit and mine is definitely the slowest. I've got to get to grips with itertools - it is very powerful.

Verbose (Mine): 3.61756896973
Comprehensions: 3.02565908432
Itertools: 2.83112883568

Here's an alternative approach using itertools.combinations to generate the (start, end) pairs:

from itertools import combinations
w = "Vehicula Sem Risus Tortor".split()
results = [' '.join(w[i:j]) for i, j in combinations(range(len(w) + 1), 2)]


Simple calculation in Java I am kind of new to Java and I have tried making a simple calculator. This lets you do different calculations in a row until you type done then you get the answer. It works as I expected but I was just wondering if I could get some feedback on it and how I can improve on it or what better approaches I could have taken.

This is my main class:

public class Main {
static  float Result = 0.0f;
static String input, Iteration;
static float firstInput = 0.0f;
static float secondInput;
static Calculations calculations = new Calculations();

public static void main(String[] args) {
    getFirstNumber();
    tryCatchMethod();
    calculations.sortCalc();
    outPutResult();

}

public static void getFirstNumber(){
    input = getInput("Enter the first number to work with");

}

public static void getIteration(){
    Iteration = getInput("Please enter the number");
    try {
        secondInput = Float.parseFloat(Iteration);
    } catch (NumberFormatException e) {

        input = getInput("Please enter a valid number");
        tryCatchMethod();   ////recursion, this is for when a valid number is not entered. This will continue happening until a valid number is entered.
    }
}

private static void tryCatchMethod(){

    try {
        firstInput = Float.parseFloat(input);
        Result = firstInput;
    } catch (NumberFormatException e) {

        input = getInput("Please enter a valid number");
        tryCatchMethod();   ////recursion, this is for when a valid number is not entered. This will continue happening until a valid number is entered.
    }

}

public static void outPutResult(){

    System.out.println("the result is :" + Result);
}

static String getInput(String prompt) {
    BufferedReader stdin = new BufferedReader(new InputStreamReader(System.in));

    System.out.print(prompt);
    System.out.flush();

    try {
        return stdin.readLine();
    } catch (Exception e) {
        return "Error: " + e.getMessage();
    }
}

}

Then the class with my calculation methods:

  public class Multiplication extends Main  {

public void basicMult(){
    Result = Result * secondInput;

}

public void basicSubt(){
     Result = Result - secondInput;
}

public void  basicAdd(){
     Result = Result + secondInput;
}

public void basicDiv(){
if(secondInput == 0.0f){
    System.out.println("Cannot divide by 0");
}
else{
    Result = Result / secondInput;
}

}
}

Class to switch between each calculation:

 public class Calculations extends Main{

    Multiplication calculation = new Multiplication();
    String getCalc;

    public void sortCalc(){
         getCalc = getInput("'+' '-' '*' or '/' or 'Done'");
         calcSwitch();
    }

    private void calcSwitch(){
        switch(getCalc){
        case "+":
            getIteration();
            calculation.basicAdd();
            sortCalc();
            break;
        case "-":
            getIteration();
            calculation.basicSubt();
            sortCalc();
            break;
        case "*":
            getIteration();
            calculation.basicMult();
            sortCalc();
            break;
        case "/":
            getIteration();
            calculation.basicDiv();
            sortCalc();
        case "Done":

            break;
        case "default":
                System.out.println("That isn't a mathematical function");
                sortCalc();
            break;
        }

    }

}

First things first, you can compare your solution with some of the other calculator implementations in Java on this site. :)

Names in Java

The naming convention used in Java is camelCase for field names, and preferably the singular form for class names. The preference is such that you have instance-s of a Customer class, not Customers.

You have an inconsistent approach to your field names (input, Iteration), and since you are getting started in Java, I will suggest sticking to the naming convention so tha it's easier for you to read and understand other Java code using the same convention, in the long run.

Class inheritance

You may have misunderstood the usage of class inheritance given the extent of having two classes (Multiplication, Calculations) extending another (Main) when they don't share a common... state.

First, your Multiplication class name is ill-advised. When I first looked at it, I thought you were going to create sub-classes for all four arithmetic operators. It turns out that Multiplication implements them as four methods. Furthermore, the only relation between this class and the parent class Main is the sharing of a single static field Result (I'll get to this later).

The usual approach for this kind of functionality is to re-think how your arithmetic methods take in two inputs to give you an output. Setting aside the float-or-double question, you should consider having methods like this:

public static float plus(float a, float b) {
    return a + b;
}

Method visibility modifier is public as it is likely to be used outside the class, not because everything defaults to public.
static method as the calculation only depends on the two method arguments, and no other class state.
The use of the two method arguments makes it clear what the inputs are, rather than letting callers of the method guess how they need to pass it in.
Again, the use of the return type also makes it clear where the output is going - back to the caller of this method.

If you still prefer to have these four methods in a standalone class, give your Multiplication class a quick rename (CalculatorUtils?) and you can safely - and should - remove the extends Main declaration: it is simply not needed.

Second, your Calculations class looks... messy. It's messy in the sense that:

It's magically responsible for instantiating your Multiplication class,
It has poorly-named method names, and
It doesn't require anything from the Main class, other than the static methods.

So, once again, you will not need extends Main here. Furthermore, when you have the static methods suggested above from the now-renamed CalculatorUtils class, your switch statement should resemble something more like this:

public static float compute(String operator) {
    // assume that you have a method `getFloatInput()` that returns a float
    switch (operator) {
        case "+":
            return CalculatorUtils.plus(getFloatInput(), getFloatInput());
        case "-":
            return CalculatorUtils.minus(getFloatInput(), getFloatInput());
        case "*":
            return CalculatorUtils.multiply(getFloatInput(), getFloatInput());
        case "/":
            return CalculatorUtils.divide(getFloatInput(), getFloatInput());
        default:
            throw new IllegalArgumentException("Invalid operator.");
    }
}

In fact, you may also want to consider combining both classes together, and exposing the float inputs in the method signature too:

// final is used here to prevent sub-classing, since it's not really required
public final class CalculatorUtils {

    private CalculatorUtils() {
        // private constructor to prevent instantiation, this class
        // is meant to be used solely through its static methods
    }

    // now that we have the compute() method, perhaps we no longer
    // need these methods to be accessible outside of this class
    private static float plus(float a, float b) {
        return a + b;
    }

    // ...

    public static float compute(String operator, float a, float b) {
        switch (operator) {
            case "+":
                return plus(a, b);
            case "-":
                return minus(a, b);
            case "*":
                return multiply(a, b);
            case "/":
                return divide(a, b);
            default:
                throw new IllegalArgumentException("Invalid operator.");
        }
    }
}

Class state vs static fields

The one thing that strikes out as odd to me in your code is the heavy reliance on static fields and methods with void return types. This indicates that while you seem to think you need inheritance here (when you don't, see above section), you have forgotten about how to make effective use of class methods to pass information around explicitly, instead choosing to rely on a global state.

From a beginner's point-of-view, I think the biggest obstacle to using static fields (not methods) throughout is that it makes your own code harder to understand in the long run. The reader has to memorize where these fields appear, and how they are modified throughout the codebase. If I may draw from my earlier example again, compare your Multiplications method:

public void  basicAdd(){
    Result = Result + secondInput;
}

Where are Result and secondInput from, and their type?
How can the method caller retrieve the result of incrementing Result?

With my suggestion:

public static float plus(float a, float b) {
    return a + b;
}

This method takes in two float values.
It returns the result of adding the values together directly to the method caller.

Doing iterations

You have a weird way of iterating through methods. Instead of using loop structures, you have your methods repeatedly calling each other at the end as such (// Here... // And here.):

public class Calculations extends Main{
    public void sortCalc(){
         getCalc = getInput("'+' '-' '*' or '/' or 'Done'");
         calcSwitch(); // Here...
    }

    private void calcSwitch(){
        switch(getCalc){
        case "+":
            getIteration();
            calculation.basicAdd();
            sortCalc(); // And here.
            break;
        // ...
    }
}

The downside of methods calling each other is that it's inefficient since the JVM will be also have to be busy with increasing the call stack. For example, this is how your oddly-named tryCatchMethod() can be improved:

// instead of wrapping a BufferedReader/InputStreamReader over System.in,
// you can also consider something like Scanner scanner = new Scanner(System.in);
// and pass that *one* instance here
private static float getFloatInput(Scanner scanner) {
    while (scanner.hasNextLine()) {
        try {
            return Float.parseFloat(scanner.nextLine());
        } catch (NumberFormatException e) {
            System.err.println("Invalid float input.");
        }
    }
    throw new RuntimeException("No input encountered.");
}

Putting it altogether

Given the relatively simple approach we have here, you probably do not require any class state, since you can easily pass your numeric inputs around well-specified methods to get your results. For example, in your main() method, you only require something like the following (try-with-resources is recommended on the Scanner instance since you appear to be at least on Java 7):

public class Main {

    public static void main(String[] args) {
        try (Scanner scanner = new Scanner(System.in)) {
            float result = getFloatInput(scanner);
            String operator;
            while (!(operator = getOperatorInput(scanner)).equals("Done")) {
                result = CalculatorUtils.compute(operator, result, getFloatInput(scanner));
                // print intermediate result here?
            }
            System.out.println("The result is :" + result);
        }
    }
}


Insertion sort in JavaScript Here is a REPL of the code. The idea here is to learn how the basic insertion sort algorithm works rather than to use existing sort libraries.

How can it be clearer and more efficient?

function insertionSort(array)
{
    for(var i = 1; i < array.length; ++i)
    {
        var currentItem = array[i];

        for(var j = i - 1; j >= 0; --j)
        {
            if(array[j] <= currentItem) break;
            array[j + 1] = array[j];
        }

        array[j + 1] = currentItem;
    }

    return array;
}

//test
var initialArray = [ 2, 4, 6, 8, 0, 5, 2, 5, 7, 8, 4, 7, 9 ];
var solutionArray = [ 0, 2, 2, 4, 4, 5, 5, 6, 7, 7, 8, 8, 9 ];

var sortedArray = insertionSort(initialArray);

var passed = JSON.stringify(sortedArray)==JSON.stringify(solutionArray);
console.log('passed:' + passed);

Eliminating some unnecessary code

Some programmers find break statements to be problematic. If you'd like to know more, here is a programmers stack exchange question about breaks and continues.

You could easily eliminate your use of break by just changing the inner loop to the following:

for (var j = i - 1; j >= 0 && array[j] > currentItem; j--) {
  array[j + 1] = array[j];
}

This shortens your code a bit and condenses all the control flow logic of you loop into one spot.

You could shorten your code even further by eliminating the conditional j >= 0:

for (var j = i - 1; array[j] > currentItem; j--) {
  array[j + 1] = array[j];
}

However, this might obfuscate the code a bit.

Note: This works because array[-1] will return undefined and undefined > myVar will always evaluate to false - ECMAScript Standard


Favorite Game List utilizing vectors and iterators So in learning c++ I have to make a little program that makes a game list that can add games to the list, remove games, and display the list, and it has to use vectors and iterators, so I made the program and need feedback on how to improve it and make it cleaner/better

// Favourite Game list
// Program using vectors and iterators

#include<iostream>
#include<string>
#include<algorithm>
#include<vector>
#include<ctime>
#include<cctype>
#include<cstdlib>

using namespace std;

int main()
{
    bool quit = false;
    string game_name;
    vector<string> gameList;
    vector<string>::iterator myIterator;
    vector<string>::const_iterator iter;
    int choice;
    while (!quit)
    {
        cout << "1: Add a Game to List" << endl;
        cout << "2: Remove a Game from the List" << endl;
        cout << "3: List all the Games on the List" << endl;
        cout << "4: Exit the Program" << endl;
        cin >> choice;

        switch (choice)
        {
        case 1:
            cout << "Enter the Game Name to add" << endl;
            cin >> game_name;
            gameList.push_back(game_name);
            break;
        case 2:
            for (iter = gameList.begin(); iter != gameList.end(); ++iter)
                cout << *iter << endl;
            cout << "Here is the list of games enter the one to remove" << endl;
            cin >> game_name;
            iter = find(gameList.begin(), gameList.end(), game_name);
            cout << *iter << "This is the game we will remove" << endl;
            gameList.erase(iter);

            break;

        case 3:
            cout << "Your Games" << endl;
            for (iter = gameList.begin(); iter != gameList.end(); ++iter)
                cout << *iter << endl;
            break;

        case 4:
            quit = true;
            cout << "You chose quit! The Program Will Close Now" << endl;

        }

    }

    return 0;
}

Unused headers
As far as I can tell, nothing from the following headers is actively being used, so they shouldn't be included.

<ctime>
<cctype>
<cstdlib>

using namespace std
While it's probably not quite as bad using it inside a .cpp file instead of a header, it's generally seen as a code smell.
Variable usage
There are some variables that are declared at the begin of main, but only used further down (if at all, myIterator appears to not be used at all). This makes it harder to keep track of variables.
Magic numbers/literals
There are a lot of numbers/string literals, whose intention is not obvious in all cases. Examples include values 1 to 4 for input choices, or strings like "Your Games".
One way of dealing with these so called magic numbers is to put them into adequately named enums, constants or variables.
Container choice
There doesn't seem to be any dependency on actually keeping the order of the games intact. In addition, a small oversight (it is possible to add the same game twice) leads me to suggest that maybe a different container would be a better fit: std::unordered_set.
std::unordered_set allows us to insert and remove items with amortized \$\mathcal{O}(1)\$ runtime complexity, as well as to ignore duplicates.
Iterators or ranged for loops?
Generally, I prefer ranged for loops over "manual" iterator loops if the code isn't requiring some specific iterator behavior. Since this isn't the case here, I would suggest using ranged for loops for ease of reading.
Prefer smaller functions
Generally, it is easier to work with smaller functions: There is less mental overhead due to shorter code, fewer variables and narrower scope. Additionally, it allows for code reuse, which might reduce code duplication.
While the given code is small enough that these concerns might not matter right now, it will become a lot more obvious once additional logic like formatting, input handling or similar gets added.
Error checking
Neither user inputs nor results of calls to std::find are being checked for invalid values. This means I can easily cause errors inside the program (or crash it) by just entering weird stuff, or trying to remove non-existing entries.
Summary
Using all these suggestions, a cleaned up version could look like this:
#include <iostream>
#include <string>
#include <unordered_set>

enum class input_choices : int {
    add = 1,
    remove,
    list,
    exit
};

std::ostream& operator<<(std::ostream& output, input_choices choice) {
    output << static_cast<int>(choice);
    return output;
}

std::istream& operator>>(std::istream& input, input_choices& choice) {
    int temp = 0;
    input >> temp;
    choice = static_cast<input_choices>(temp);

    return input;
}

namespace messages {
    static const auto menu_header = "Menu";
    static const auto option_add = "Add a game to the list";
    static const auto option_remove = "Remove a game from the list";
    static const auto option_list = "Show all games on the list";
    static const auto option_exit = "Exit the program";
    static const auto enter_menu_choice = "Your choice: ";

    static const auto enter_game_to_add = "Enter the Game Name to add";
    static const auto enter_game_to_remove = "Here is the list of games enter the one to remove";
    static const auto list_header = "Your games";
    static const auto quitting = "Exiting program...";
    static const auto invalid_choice = "Unknown menu option";
}

class game_list_menu {
    std::unordered_set<std::string> games;

public:
    void add_game() {
        const auto game = enter_game(messages::enter_game_to_add);
        games.insert(game);
    }

    void remove_game() {
        print_games();

        const auto game = enter_game(messages::enter_game_to_remove);
        games.erase(game);
    }

    std::string enter_game(std::string_view prompt) const {
        std::cout << prompt << ": ";

        auto game = std::string{};
        std::cin >> game;

        return game;
    }

    void print_games() const {
        std::cout << "\n" << messages::list_header << ":\n\n";

        for(auto& game : games) {
            std::cout << "\t" << game << "\n";
        }
    }

    void print_menu() const {
        std::cout << "\n" << messages::menu_header << "\n\n";

        print_choice(input_choices::add, messages::option_add);
        print_choice(input_choices::remove, messages::option_remove);
        print_choice(input_choices::list, messages::option_list);
        print_choice(input_choices::exit, messages::option_exit);

        std::cout << "\n" << messages::enter_menu_choice;
    }

    void print_choice(input_choices choice, std::string_view description) const {
        std::cout << "\t" << choice << ": " << description << "\n";
    }

    void main_loop() {
        auto choice = input_choices::exit;

        do {
            print_menu();

            std::cin >> choice;

            switch(choice) {
            case input_choices::add:
                add_game();
                break;
            case input_choices::remove:
                remove_game();
                break;
            case input_choices::list:
                print_games();
                break;
            case input_choices::exit:
                std::cout << messages::quitting << "\n";
                break;
            default:
                std::cout << messages::invalid_choice << "\n";
                break;
            }
        } while(choice != input_choices::exit);
    }
};

int main() {
    auto menu = game_list_menu{};
    menu.main_loop();
}

Can you see how you can easily tell what each function does or what the meaning of each number/literal is?


Translating text using Google Translate mobile site I have this code to translate text using the Google Translate mobile site. Currently, text size is limited by the request method. Everything else seems to works just fine.

I am also about to post this on PyPi but I don't know how I should name the package, the file and the function.

Basically today you must do:

import translate.translate
translate.translate.translate("hello")

which seems really bad. How should I name this?

Could you suggest any improvements to this?

#!/usr/bin/env python
# encoding: utf-8

import six

if (six.PY2):
    import urllib2
    import re
    import urllib
else:
    import urllib.request
    import urllib.parse
    import re

agent = {'User-Agent' : "Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1; .NET CLR 1.1.4322; .NET CLR 2.0.50727; .NET CLR 3.0.04506.30)"}

def translate(to_translate, to_language="auto", language="auto"):
    """
    Returns the translation using google translate
    you must shortcut the language you define (French = fr, English = en, Spanish = es, etc...)
    if you don't define anything it will detect it or use english by default
    Example:
    print(translate("salut tu vas bien?", "en"))
    hello you alright?
    """
    base_link = "http://translate.google.com/m?hl=%s&sl=%s&q=%s"
    if (six.PY2):
        link = base_link % (to_language, language, urllib.pathname2url(to_translate))
        request = urllib2.Request(link, headers=agent)
        page = urllib2.urlopen(request).read()
    else:
        link = base_link % (to_language, language, urllib.parse.quote(to_translate))
        request = urllib.request.Request(link, headers=agent)
        page = urllib.request.urlopen(request).read().decode("utf-8")
    expr = r'class="t0">(.*?)<'
    result = re.findall(expr, page)
    if (len(result) == 0):
        return ("")
    return(result[0])

if __name__ == '__main__':
    to_translate = 'Bonjour comment allez vous?'
    print("%s >> %s" % (to_translate, translate(to_translate)))
    print("%s >> %s" % (to_translate, translate(to_translate, 'es')))
    print("%s >> %s" % (to_translate, translate(to_translate, 'ar')))

import translate.translate
translate.translate.translate("hello")

The verbosity is strong with this one. The clarity is not.

Why do you have a translate in a translate in a translate? With constructs like this, you can bet there are better names available.

Inside your main the same problem occurs:

to_translate = 'Bonjour comment allez vous?'
print("%s >> %s" % (to_translate, translate(to_translate)))
print("%s >> %s" % (to_translate, translate(to_translate, 'es')))
print("%s >> %s" % (to_translate, translate(to_translate, 'ar')))

That's 3 counts of translate per call. It's obvious you're translating something, but the rest is vague.

Afar from that, are you familiar with the Python Enhancement Proposals? Some of them include style suggestions, most notably the PEP 8. You might want to take at least a good look at it.

The most notable violation of your code against the PEP 8 is your lines are too long. Long lines are hard to read, hard to parse by humans. The readability of your code will improve if you split them up logically.

The ugly way of breaking-up lines is with line continuations:

agent = {'User-Agent' : "Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1; .NET CLR 1.1.4322; .NET CLR 2.0.50727; .NET CLR 3.0.04506.30)"}
agent2 = {'User-Agent' : "Mozilla/4.0 (\
compatible; MSIE 6.0; Windows NT 5.1; SV1; .NET CLR 1.1.4322; \
.NET CLR 2.0.50727; .NET CLR 3.0.04506.30)"}
print agent
print agent2

Those produce the same output.

The proper way would be turning agent into a proper data format and parsing it on request or reading it from somewhere else. You really shouldn't have hard-coded strings that long in your code anyway.


print array of previous smaller elements I am solving interview question from here.

  Problem : Given an array, find the nearest smaller element G[i] for every element A[i] in the array such that the element has an index smaller than i.

  More formally, G[i] for an element A[i] = an element A[j] such that
      j is maximum possible and j < i and A[j] < A[i].
  Elements for which no smaller element exist, consider next smaller element as -1.

  Example: Input : A : [4, 5, 2, 10, 8] Return : [-1, 4, -1, 2, 2]

How can I optimise my solution:

def prev_smaller(arr):
    new_stack = []
    status = False
    for i in range(len(arr)):
        for j in range(i,-1,-1):
            if arr[i]>arr[j]:
                status = True
                new_stack.append(arr[j])
                break
        if not status:
            new_stack.append(-1)
        status = False
    return new_stack

assert prev_smaller([34,35,27,42,5,28,39,20,28]) == [-1,34,-1,27,-1,5,28,5,20]
assert prev_smaller([1]) == [-1]
assert prev_smaller([1,4,5,6]) ==[-1,1,4,5]

This can be done in a single pass over input list with a stack to keep track of previous smaller elements. Note that the stack will have a sub-sequence that is sorted at any given point.

def FastSolve(v):
  res = []
  s = [-1]
  for x in v:
    while s[-1] >= x:
      s.pop()
    res.append(s[-1])
    s.append(x)
  return res

Note that this exact question has been asked before here and a very similar variation (next smaller instead of last) here - the latter question has an excellent detailed explanation of the O(n) solution and code examples.


Simulating gravitational attraction between bodies I'm currently working on a simple simulator in Unity3D that involves planets orbiting each other. I wrote the following C# script to do that. It uses the classic gravitational formula,

$$F=G\frac{m_1m_2}{r^2}$$

using UnityEngine;

/// <summary>
/// Class used for calculating
/// how an object orbits another
/// object.
/// </summary>
/// <param name="parentObject">The parent gameObject to orbit.</param>
/// <param name="objectMass">The mass of the object.</param>
/// <param name="gravityConstant">The gravitational constant.</param>
[RequireComponent(typeof(Rigidbody))]
public class OrbitObject : MonoBehaviour
{
    public OrbitObject parentObject;
    public float objectMass;
    public float gravityConstant;

    private Vector3 parentPosition;
    private float parentMass;

    /// <summary>
    /// Calculates the gravitational attraction.
    /// </summary>
    /// <returns>The force to apply</returns>
    Vector3 CalculateGravitationalForce()
    {
        this.parentPosition = this.parentObject.transform.position;
        Vector3 force = this.parentPosition - transform.position;
        float distance = force.magnitude;
        float strength = (this.gravityConstant * this.parentMass * this.objectMass) / (distance * distance);
        force.Normalize();
        force = force * strength;
        return force;
    }

    /// <summary>
    /// FixedUpdate is run once per frame.
    /// FixedUpdate is used so that calculations
    /// don't need to be multiplied by Time.deltaTime.
    /// </summary>
    void FixedUpdate()
    {
        rigidbody.AddForce(this.CalculateGravitationalForce());
    }

    /// <summary>
    /// Start this instance.
    /// </summary>
    void Start()
    {
        this.parentPosition = this.parentObject.gameObject.transform.position;
        this.parentMass = this.parentObject.objectMass;
    }
}

For those who are curious, a gif of the script in action can be found here.

Just a note on your commenting:

Should you be breaking XML comments up across lines like that?

/// <summary>
/// Class used for calculating
/// how an object orbits another
/// object.
/// </summary>

I feel that should be three lines:

/// <summary>
/// Class used for calculating how an object orbits another object.
/// </summary>

Likewise, I feel that the following comment:

/// <summary>
/// FixedUpdate is run once per frame.
/// FixedUpdate is used so that calculations
/// don't need to be multiplied by Time.deltaTime.
/// </summary>

Could be rewritten as:

/// <summary>
/// FixedUpdate is run once per frame.
/// </summary>
/// <remarks>
/// FixedUpdate is used so that calculations don't need to be multiplied by Time.deltaTime.
/// </remarks>

Personally, I think breaking comments along multiple lines like that makes it harder to read. You're eyes have to jump to unnatural places.

Likewise, the <remarks> bit is nice because those comments aren't shown in Intellisense, but they can still be placed in the generated XML documentation files.

The only other bit I would mention, is the use of this. You should either use it consistently, or try to limit it. Sometimes (especially in larger cases) it helps to clear things up. But if you use it for some of the properties and not others, it can have a negative impact.

I'm referring to the following:

Vector3 CalculateGravitationalForce()
{
    this.parentPosition = this.parentObject.transform.position;
    Vector3 force = this.parentPosition - transform.position;
    float distance = force.magnitude;
    float strength = (this.gravityConstant * this.parentMass * this.objectMass) / (distance * distance);
    force.Normalize();
    force = force * strength;
    return force;
}

Where does transform.position come from? The fact that you use this on all the other properties/fields of the class but not that one throws confusion into the mix. It's not a local variable, that much is true. But where exactly does it come from, is a little harder to notice right out of the box.

Again, these are all suggestions. Whether or not you like/use them is up to you, but this is how I would write it out.


Criticize my threaded image downloader I'm going to be working on a much larger version of this program and I just wanted to see if there was anything I should change in my style of coding before I made anything larger.

If it wasn't obvious, this code goes through each comic on A Softer World and downloads each comic.

Oh and with larger projects, should I try and fit my main() function into a class or is having a large main() function normal with programs?

from BeautifulSoup import BeautifulSoup as bs
from urllib import urlretrieve
from urllib2 import urlopen
from os import path, mkdir
from threading import Thread
from Queue import Queue

class Worker(Thread):

    def __init__(self, queue):
        self.queue = queue
        self.url = 'http://asofterworld.com/index.php?id='
        Thread.__init__(self)

    def run(self):
        while 'running':
            number = self.queue.get()
            soup = bs(urlopen(self.url + number))
            match = soup.find('p', {'id' : 'thecomic'}).find('img')

            filename = match['src'].split('/')[-1]
            urlretrieve(match['src'], 'Comic/' + filename)
            self.queue.task_done()

def main():

    queue = Queue()

    if not path.isdir('Comic'):
        mkdir('Comic')

    for number in xrange(1, 977):
        queue.put(str(number))

    for threads in xrange(20):
        thread = Worker(queue)
        thread.setDaemon(True)
        thread.start()

    queue.join()

main()

The names Worker and run are very general, while what they are doing is actually very specific (crawl a single web site for images matching a pattern). It would be better if the names reflected their content.
The magic numbers and strings in the code should either be parameters or constants. The thread count, for example, would be a good candidate for a parameter (since you might handle threading differently when importing Worker), while the different parts of the URL could be class constants.
Regarding main, it should IMO only contain that which is necessary for running from a shell (as opposed to importing the functionality in another Python program). That is, when importing everything except main there should be no missing pieces to get the same functionality as when running from a shell.


Reducing memory usage when comparing two iterables I'm changing code that writes data to a DB, so I have a dump (a text file) of an earlier run to compare against, to ensure that my changes don't screw things up. Here goes:

def dbcheck(cursor):
    dbresult = list()
    cursor.execute("SELECT COLUMN FROM TABLE")
    for item in cursor.fetchall():
        line = item[0] + "\n"
        dbresult.append(line)
    with open(dbdump) as f:
        for n, line in enumerate(f):
            if line != dbresult[n]:
                print("DB content does not match original data!")

This code runs fine, but I'm worried that dbresult can grow really large, so am looking for a less risky way of doing this. I'm also curious of what else can be improved.

[sidenote] I left out exception handling for the sake of simplicity/clarity.

This should do it:

def dbcheck(cursor):
    cursor.execute("SELECT COLUMN FROM TABLE")
    with open(dbdump) as f:
        for item in cursor:
            if f.readline() != item + '\n'
                print("DB content does not match original data!")

No need to read either the whole column nor the whole file before iterating.


Sync Server Time with User's Timezone I have a web application which is being used by users from different parts of the world. In order for users of the site, to be able to view the timestamps in their local time, I've a settings page set up which will allow the user to store their preferred timezone (Europe/Berlin, America/Chicago) etc. When a user is authenticated successfully, value fetches from the DB and stores it in a session.

From what I've read so far, the best practice seems to be to store the datetime value in UTC format when a new record is created in a database and while displaying it, convert it to the user's timezone. So to achieve the following, this is what I'm doing:

if($this->userIsAuthenticated())
{
   $this->session->timezone = "Europe/Berlin"; // this value is coming from db
}

// inserting records
date_default_timezone_set("UTC");
$data = array("created_time"=>date("Y-m-d H:i:s"));
$this->db->insert("my_table", $data);

// fetching records
$date_from_db = $row->created_time;
$display_date = new DateTime($date_from_db , new DateTimeZone('UTC'));
$display_date->setTimeZone(new DateTimeZone($this->session->timezone));
return $display_date->format("m/d/Y h:i a");

So the workflow is as follows:

Use date_default_timezone_set() for every insert query
Pull the value from the db and set it to UTC using DateTime class
Then set the timzeone to user's timezone

This solution works. But, is this best way of doing it? Or, are there any more modular ways of achieving the same result?

I personally would have saved it as a simple timestamp rather than a date, whether or not that's actually a better idea is debatable, but a timestamp requires half as much space as a datetime and is, imo, much easier to use.

On the client side you can get the user's UTC offset with a simple new Date().getTimezoneOffset() which will return the difference between the user's time and UTC in minutes (multiply by 60 to get seconds). Then you can use PHP's time() function to get the current timestamp and subtract the user's offset.

When you need to display it as a human readable date, PHP's date() can take a timestamp as a second parameter to display a specific time. Timestamps are just a lower level, more efficient way to handle time differences.

if($this->userIsAuthenticated())
{
   $this->session->timezone_offset = "-600"; // this value is coming from db
}

// inserting records
$data = array("created_time"=>time());
$this->db->insert("my_table", $data);

// fetching records
$date_from_db = $row->created_time;
$display_date = date("m/d/Y h:i a", $date_from_db - ($this->session->timezone_offset * 60));


Order Spreadsheet code cleaning project I am working on a bit of a multiple part project, the goal of the project is to have a spreadsheet full of order data (order number, customer data, products ordered, prices etc) merged so that the duplicate information is grouped up.

The above is an example of what the data looks like when it comes in.

I then got it to unmerge so that it looks a little cleaner.

The code used for these two steps was created with much help from this community and looks like this:

Option Explicit

Sub MergeCells()

Dim i As Long, c As Long, col As Variant

Application.DisplayAlerts = False
'Application.ScreenUpdating = false

col = Array("A", "B", "C", "D", "E", "F", "G", "H", "I", "J", "K", "L", "M", "N", "O", "P", "Q", "R", "S", "T", "U", "AA", "AB", "AC", "AD", "AE", "AF", "AG", "AH", "AI", "AJ", "AK")

For c = LBound(col) To UBound(col)
    For i = Cells(Rows.Count, col(c)).End(xlUp).Row - 1 To 2 Step -1
        If Cells(i, col(c)).Value = Cells(i, col(c)).Offset(1, 0).Value And Not IsEmpty(Cells(i, col(c))) Then
            Cells(i, col(c)).Resize(2, 1).Merge
            Cells(i, col(c)).HorizontalAlignment = xlCenter
            Cells(i, col(c)).VerticalAlignment = xlCenter
        End If
    Next i
Next c

Application.DisplayAlerts = True
'Application.ScreenUpdating = True
End Sub

Then this:

Sub fixMergedCells(sh As Worksheet)
'replace merged cells by Center Acroos Selection
'high perf version using a hack: https://stackoverflow.com/a/9452164/78522
Dim c As Range, used As Range
Dim m As Range, i As Long
Dim constFla: constFla = Array(xlConstants, xlFormulas)

Set used = sh.UsedRange
For i = 0 To 1  '1 run for constants, 1 for formulas
    Err.Clear
    On Error Resume Next
    Set m = Intersect(used.Cells.SpecialCells(constFla(i)), used.Cells.SpecialCells(xlBlanks))
    On Error GoTo 0
    If Not m Is Nothing Then
        For Each c In m.Cells
            If c.MergeCells Then
                With c.MergeArea
                    'Debug.Print .Address
                    .UnMerge
                    .HorizontalAlignment = xlCenterAcrossSelection
                End With
            End If
        Next c
    End If
Next i
End Sub

And finally:

Sub fixMergedCells(Optional sh As Variant)

If IsMissing(sh) Then Set sh = ActiveSheet

sh.Cells.UnMerge

End Sub

I am currently working on two final steps, one to make sure that the data that got merged and then split will appear for each relevant line. Then I want to highlight each row that contains an order number so that we can see clearly where one order starts and another ends.

My last step of the plan is to combine all of the codes into one macro, as I want it to be easy for someone to use.

Thanks for reading, I just wanted to share what I've gotten so far and see what y'all think of my plan :)

Welcome to Code Review! You've made a very good start at learning VBA and how to put together clean code. Many helpful folks will give you supportive comments and I'll jump in with mine...

Starting with your MergeCells routine:

The name MergeCells is already a method of the Range object. Naming your routine with this same name can be confusing, so I stronly recommend a different name such as MergeAdjacentNonEmptyCellsInColumn. It may seem overly long, but it's descriptive and makes your code an easy read.
You might consider using an array of columns as an input parameter to this Sub. That would give you the flexibility to reuse the Sub in the future on a different set of columns.
I have many bruises and scars from writing code the says Cells(... or Rows.Count without explicitly attaching those references to a specific worksheet. In your case, your Sub will only work on the currently active worksheet. If you create an input parameter for the target worksheet, that will make sure the routine works on the sheet you want for sure. To make it easier to read, just wrap your loop sections in a With clause.
Go ahead and declare your variables closer to their first use. It saves you from having to jump back and forth between their declaration and the executable code.
Please break up long lines such as your Array initialization. It makes it much easier to read. The typical column to wrap a long line is column 80, but you can choose your own as long as you're consistent.
I'm in favor of somewhat longer, more descriptive variable names. It makes your code read more like sentences that can (hopefully) clearly convey what that code is doing. So I use lastRowInColumn and similar names.

So:

Option Explicit

Sub test()
    Dim columnsToMerge() As Variant
    columnsToMerge = Array("A", "B", "C", "D", "E", "F", "G", "H", "I", _
                           "J", "K", "L", "M", "N", "O", "P", "Q", "R", _
                           "S", "T", "U", "AA", "AB", "AC", "AD", "AE", _
                           "AF", "AG", "AH", "AI", "AJ", "AK")

    MergeAdjacentNonEmptyCellsInColumn Sheet1, columnsToMerge
End Sub

Public Sub MergeAdjacentNonEmptyCellsInColumn(ByRef targetWS As Worksheet, _
                                              ByRef colArray() As Variant)
    Application.DisplayAlerts = False
    'Application.ScreenUpdating = false
    With targetWS
        Dim lastRowInColumn As Long
        Dim c As Long
        For c = LBound(colArray) To UBound(colArray)
            lastRowInColumn = .Cells(.rows.count, colArray(c)).End(xlUp).Row
            Dim i As Long
            For i = lastRowInColumn - 1 To 2 Step -1
                If .Cells(i, colArray(c)).value = .Cells(i, colArray(c)).Offset(1, 0).value And _
                   Not IsEmpty(.Cells(i, colArray(c))) Then
                    .Cells(i, colArray(c)).Resize(2, 1).Merge
                    .Cells(i, colArray(c)).HorizontalAlignment = xlCenter
                    .Cells(i, colArray(c)).VerticalAlignment = xlCenter
                End If
            Next i
        Next c
    End With
    Application.DisplayAlerts = True
    'Application.ScreenUpdating = True
End Sub

For your next Sub, you've named it fixMergedCells, but that is identical to the next sub. So I think this sub should be renamed to something like unmergeAndCenter.

A generally followed convention is to begin variable names with lower case and Sub/Function names with an uppercase letter. That would make your sub UnmergeAndCenter.
The variable m is a Range. But as I mentioned earlier, I see a single-letter variable as a looping index. So I'd rather see this named allMergedCells because that's what you get when you perform the Intersect action.

And so:

Sub test1()
    UnmergeAndCenter RawData
End Sub

Sub UnmergeAndCenter(sh As Worksheet)
    'replace merged cells by Center Across Selection
    'high perf version using a hack: https://stackoverflow.com/a/9452164/78522
    Dim constFla As Variant
    constFla = Array(xlConstants, xlFormulas)

    Dim usedArea As Range
    Set usedArea = sh.UsedRange

    Dim i As Long
    For i = 0 To 1                               '1 run for constants, 1 for formulas
        Err.Clear
        On Error Resume Next
        Dim allMergedCells As Range
        Set allMergedCells = Intersect(usedArea.Cells.SpecialCells(constFla(i)), _
                                       usedArea.Cells.SpecialCells(xlBlanks))
        On Error GoTo 0
        If Not allMergedCells Is Nothing Then
            Dim c As Range
            For Each c In allMergedCells.Cells
                If c.MergeCells Then
                    With c.MergeArea
                        'Debug.Print .Address
                        .UnMerge
                        .HorizontalAlignment = xlCenterAcrossSelection
                    End With
                End If
            Next c
        End If
    Next i
End Sub

Capitalize the first letter of your last Sub.
It's another long habit of mine (create your own style and habit) that I almost never write a compound statement (like an If) on one line. It may be tedious, but it makes my code more consistent to always use multiple lines. Makes it easier to maintain in the future (IMHO).

Finally:

Sub FixMergedCells(Optional sh As Variant)
    If IsMissing(sh) Then
        Set sh = ActiveSheet
    End If
    sh.Cells.UnMerge
End Sub


Parsing NSDictionary from NSJSONSerialization playerObject is an NSDictionary from JSONObjectWithData:options:error.

I'm trying to update a CoreData entity.

if ([results count] > 0)
{
    newPlayer = (Player *)[results objectAtIndex:0];
} else {
    //Add a new player
    newPlayer = [NSEntityDescription insertNewObjectForEntityForName:@"Player" inManagedObjectContext:temporaryContext];
}
//Assume these exist and are not null
newPlayer.email = [playerObject objectForKey:@"email"];
newPlayer.username = [playerObject objectForKey:@"username"];
newPlayer.continent = [playerObject objectForKey:@"continent"];

//Dates
NSDateFormatter *df = [[NSDateFormatter alloc] init];
[df setDateFormat:@"yyyy-MM-dd hh:mm:ss"];
newPlayer.created = [df dateFromString: [playerObject objectForKey:@"created"]];

//Optional properties
if ([[playerObject objectForKey:@"title"] isKindOfClass:[NSNull class]])
{
    newPlayer.title = nil;
} else {
    newPlayer.title = [[playerObject objectForKey:@"title"] stringValue];
}

Is there any way to make it less verbose?  Have I left myself open to any issues?

if ([results count] > 0)
{
    newPlayer = (Player *)[results objectAtIndex:0];
} else {
    //Add a new player
    newPlayer = [NSEntityDescription insertNewObjectForEntityForName:@"Player" inManagedObjectContext:temporaryContext];
}

This chunk of code could be replaced with:

newPlayer = (Player *)([results firstObject] ?: [NSEntityDescription insertNewObjectForEntityForName:@"Player" inManagedObjectContext:temporaryContext]);

if ([[playerObject objectForKey:@"title"] isKindOfClass:[NSNull class]])
{
    newPlayer.title = nil;
} else {
    newPlayer.title = [[playerObject objectForKey:@"title"] stringValue];
}

This chunk of code could also be slightly better.

NSString *newTitle = playerObject[@"title"];
newPlayer.title = [newTitle isEqual:[NSNull null]] ? nil : [newTitle stringValue];

The last comment that I'll make is a strong recommendation that you use constants for your keys rather than literal strings.  Defined constants will auto-complete for you, which is nice, but more importantly, it will eliminate misspellings entirely.

static NSString * const kKey_Player = @"Player";
static NSString * const kKey_Title = @"title";
// etc.

Then use it as such:

NSString *newTitle = playerObject[kKey_Title];

Under the covers, this actually doesn't change in the slightest what the code does.  Even when you use string literals, a memory location is still allocated for them.  The advantage here is that you eliminate programmer mistakes.


Program to compress a string of characters I was given this small task in an interview recently. I'm usually terrible at coding questions in interviews, because with the time constraint and the lack of google I usually overthink and rush stuff and just end up with a mess of a program.

The task was to write a program that would compress the length of string, and compare the length of the compressed string to the input string and return whichever one is smaller. Example would be: input string "aaabcccc" would be compressed to "a3b1c4".

I essentially split the string into a charArray and then counted each occurrence of a character and stored them into a hashmap, and looped through the map to build the new string. Just from a learning perspective, was there a better way to do this? I was given ~10 minutes to write it as well, it more to assess how I would solve the problem as opposed to the code itself. But regardless, I'd like a review of it:

import java.util.HashMap;
import java.util.Map.Entry;

public class Compressor {

    public static void main(String args []) {

        String randomString = "aaabccccc";

        HashMap<Character, Integer> map = countCharacters(randomString);
        String compressedString = createCompressedString(map);

        if(randomString.toCharArray().length < compressedString.toCharArray().length) {
            System.out.println(randomString);
        }
        else {
            System.out.println(compressedString);
        }
    }

    /**
     * Create hasmap to store character and count of occurrence
     * @param s
     * @return
     */
    private static HashMap<Character, Integer> countCharacters(String s) {

        HashMap<Character, Integer> characterCount = new HashMap<Character, Integer>();
        char[] characterArray = s.toCharArray();

        for(Character c : characterArray) {
            int newCount;
            Integer count = characterCount.get(c);
            if(count == null) {
                newCount = 1;
            }
            else {
                newCount = count + 1;
            }
            characterCount.put(c, newCount);
        }

        return characterCount;
    }

    /**
     * Convert hashmap into a string
     * @param map
     * @return
     */
    private static String createCompressedString(HashMap<Character, Integer> map) {

        String newString = "";
        for (Entry<Character, Integer> entry : map.entrySet()) {
            Character key = entry.getKey();
            Integer value = entry.getValue();

            newString += "" + key + "" + value;
        }

        return newString
    }
}

For better performance you should be able to eliminate the Hashmap and use a StringBuilder to count and build the compressed string in one operation:

public static String Compress(String input)
{
    StringBuilder sb = new StringBuilder();
    int i = 0;
    int limit = input.length() - 1;
    char next = '\0';
    while (i < limit)
    {
        int count = 1;
        next = input.charAt(i++);
        while (i <= limit && next == input.charAt(i))
        {
            count++;
            i++;
        }
        sb.append(next);
        sb.append(count);
    }
    if(i == limit)
    {
        sb.append(input.charAt(i));
        sb.append(1);
    }
    return sb.toString();
}

Caveat:  This code won't work with the full unicode set


Determining a value given the coordinates for numbers arranged in a triangular fashion Numbers are being stacked in the following manner:

7
4 8
2 5 9
1 3 6 10

The x-axis is horizontal and the y-axis is vertical. With that in mind, here are a few examples of the values and their coordinates:

2, 3 (coordinate) ==> 8 (value)
2, 2 (coordinate) ==> 5 (value)

I need to write a function \$fn(x, y)\$ which accepts a coordinate \$(x, y)\$ and returns the corresponding value. \$x\$ and \$y\$ can be any value between 1 and 100000 and I can only use standard python library.

Here is my brute force-ish attempt at solving this:

import time

def profile_it(fn, *args):
    start = time.time()
    result = fn(*args)
    end = time.time()
    print('{0} --> {1} in {2}sec(s)'.format(args, result, end-start))

def answer(x, y):
    result = 0
    max_limit = 100000
    x = max(min(x, max_limit), 1)
    y = max(min(y, max_limit), 1)
    counter = 1
    not_finished = True
    while not_finished:
        x_cord = 0
        y_cord = counter + 1
        for step in xrange(counter):
            result += 1
            x_cord += 1
            y_cord -= 1
            if x == x_cord and y == y_cord:
                return str(result)
        counter += 1

if __name__ == '__main__':
    profile_it(answer, 3, 2)
    profile_it(answer, 5, 10)
    profile_it(answer, 100, 100)
    profile_it(answer, 1000, 1000)
    profile_it(answer, 10000, 10000)

The above solution works reasonably well for smaller inputs. However, it takes too long to compute the result for larger inputs as evident from the following results:

(3, 2) --> 9 in 0.0sec(s)
(5, 10) --> 96 in 0.0sec(s)
(100, 100) --> 19801 in 0.0sec(s)
(1000, 1000) --> 1998001 in 0.148000001907sec(s)
(10000, 10000) --> 199980001 in 15.5760002136sec(s)

I'm sure there is a lot of room for improvement in my code. I'm looking for pointers on what kind of an algorithm I should use to solve this problem in a quicker fashion.

Based on some reading, I've attempted to use memoization using an OrderedDict to keep track of values that were computed, so for subsequent calls, I'm not re-calculating the values from scratch. However, that increased the time to solve this by a lot so I've discarded that approach.

I don't see why you should return the answer as a string.  That makes your mathematical function weird.

You shouldn't use flag variables like not_finished = True; they are almost never necessary, and in this case not_finished is totally superfluous, since it is always True.  Actually, this loop would be better written as

for counter in itertools.count(1):
    x_cord = 0
    …

Your solution is brute force.  I would characterize it as \$O(\max(x^2, y^2))\$.  Of course, performance is going to be a problem with large coordinates.  The solution is not to use memoization, but to work out a formula for the answer.

First, note that the numbers along the bottom (\$\mathit{fn}(x, 1)\$) are recognizable as the triangular numbers, and even if you didn't recognize them, you could easily figure out the formula.  Then, you can count backwards along the \$\nwarrow\$ diagonal.

$$\begin{align}
\mathit{fn}(x, 1) =&\ 1 + 2 + 3 + \ldots + x = \frac{x\ (x+1)}{2} \\
\mathit{fn}(x - 1, 2) =&\ \mathit{fn}(x, 1) - 1 \\
\mathit{fn}(a - 2, 3) =&\ \mathit{fn}(a, 1) - 2 \\
 \vdots& \\
\mathit{fn}(a - (y-1), y) =&\ \mathit{fn}(a, 1) - (y-1) \\
 \vdots& \ \scriptstyle{a\ =\ x+y - 1} \\
\mathit{fn}(x, y) =&\ \mathit{fn}(x + y - 1, 1) - (y - 1) = \frac{(x + y)(x + y - 1)}{2} - (y - 1)
\end{align}$$


Fun with CallerLineNumberAttribute: clever hack or terrible idea? One fun feature that was added (fairly) recently in .NET is the [CallerLineNumber] attribute, which, in conjunction with [CallerFilePath] and [CallerMemberName] can allow for some very nice logging methods.

I was thinking today, though, that they might be used for another interesting purpose.  Consider the case where you have some event that should trigger on a Boolean transition.  The classic implementation for this uses a field to keep track of the 'last state', then updates the field after performing the check.  The annoying thing, though, is that you need a flag for every case and each flag needs a name.  So I thought of something like this:

public sealed class IndexedBuilder<T>
    where T : class, new()
{
    private readonly Dictionary<int, T> _instances = new Dictionary<int, T>();

    public T Build(int index)
    {
        T instance;

        lock (_instances)
        {
            if (!_instances.TryGetValue(index, out instance))
            {
                instance = new T();
                _instances.Add(index, instance);
            }
        }

        return instance;
    }
}

public sealed class TransitionDetector
{
    private readonly IndexedBuilder<Context> _builder = new IndexedBuilder<Context>();

    public bool DetectUp(bool state, [CallerLineNumber] int lineNumber = 0)
    {
        return _builder.Build(lineNumber).DetectUp(state);
    }

    public bool DetectDown(bool state, [CallerLineNumber] int lineNumber = 0)
    {
        return _builder.Build(lineNumber).DetectDown(state);
    }

    private sealed class Context
    {
        private bool? _lastState;

        public bool DetectUp(bool state)
        {
            bool result = state && (_lastState == false);
            _lastState = state;
            return result;
        }

        public bool DetectDown(bool state)
        {
            bool result = !state && (_lastState == true);
            _lastState = state;
            return result;
        }
    }
}

public static class Program
{
    public static void Main()
    {
        var detector = new TransitionDetector();

        int x = 0;
        int y = 0;
        while (x < 10000)
        {
            if (detector.DetectUp(y > 100))
                Console.WriteLine("y > 100");

            if (detector.DetectDown(x < 500))
                Console.WriteLine("x >= 500");

            y++;
            x++;

            if (y > 1000)
                y = 0;
        }

        Console.ReadLine();
    }
}

This is a specific example of the more general pattern of allowing an instance to be bound to a particular location in the code, without explicitly naming or referencing the instance.  The biggest issue I see is that this breaks down if you attempt to perform multiple calls on the same line in the code or if you pass the TransitionDetector instance such that it gets called from another file, which means documentation required and there is a significant potential for incorrect use.

This is definitely an interesting use of [CallerLineNumber] but I think that the risk for people using it wrong resulting in hard to find bugs is to great. If you take an example from the issue you yourself mentioned.

if (detector.DetectUp(y > 100) ||
    detector.DetectUp(x < 500))
{...}

Would not as one might think have the same behavior as

if (detector.DetectUp(y > 100) || detector.DetectUp(x < 500))
{...}

Another gotcha I found was that lambda functions does not give the line or name of where they are called but where they are defined. This makes sense but might not be what the user expects. The example below prints Main: 10 and not HelpFun: 16.

using System;
using System.Runtime.CompilerServices;

namespace CallerLineNumberTest
{
    class Program
    {
        static void Main(string[] args)
        {
            HelpFun(() => PrintLineNr());
            Console.ReadKey();
        }

        private static void HelpFun(Action a)
        {
            a();
        }

        private static void PrintLineNr([CallerMemberName] string name = "",
            [CallerLineNumber] int lineNumber = 0)
        {
            Console.WriteLine(name + ": " + lineNumber);
        }
    }
}


Project Euler #3 Largest prime factor Given

The prime factors of 13195 are 5, 7, 13 and 29.
What is the largest prime factor of the number 600851475143 ?

Solution
def max_factor(num):
    """Find the maximum prime factor."""
    factor = 2
    while factor * factor <= num:
        while num % factor == 0:
            num /= factor
        factor += 1
    if (num > 1):
        return num
    return factor

print max_factor(33) #11
print max_factor(38) #19
print max_factor(600851475143) #6857

This problem is already discussed a lot. I am more interested in finding the running time complexity. Since input here is just a number how to relate the running time to the input size. Also, I sense that running time should be logarithmic but how to derive that exactly seems quite tough.

Also, I sense that running time should be logarithmic but how to derive that exactly seems quite tough.

Nope, running time is \$ O(\sqrt N) \$ worst-case. Consider the case of a prime number (or particularly bad cases, like the product of twin primes). You have to check \$ \sqrt N \$ possible values to find the answer. No way around that.

Code-wise, you have a bug, only minor/trivial comments otherwise. First the bug. The issue is here:

return factor

What is factor at the end? It's just the first number whose square is larger than whatever num has become. It's not necessarily a factor of the original value. It's just an index. As an example, max_factor(8) == 3, max_factor(9) == 4, etc. You need to keep track of which of the attempted factors actually are factors. Something like:

def max_factor(num):
    """Find the maximum prime factor."""
    best = None
    factor = 2
    while factor * factor <= num:
        while num % factor == 0:
            best = factor
            num /= factor
        factor += 1
    if (num > 1):
        return num
    return best

As others have pointed out, you don't do input validation. I don't really consider that hugely important here and it's perfectly fine to just require that the user passes reasonable numbers in. But it couldn't hurt to just make that explicit:

def max_factor(num):
    """Find the maximum prime factor."""
    assert num >= 2
    ...

Otherwise, you have a counting loop with a non-trivial condition. This is one of those things that's always annoying to express in Python. In C or C++, that'd be:

for (factor=2; factor*factor <= num; ++factor) { ... }

and we have everything on one line. In Python, we have three options, none of which I'm thrilled about. Yours:

factor = 2
while factor * factor <= num:
    ...
    factor += 1

Using itertools.count:

for factor in itertools.count(start=2):
    if factor * factor > num: break
    ...

Using itertools.takewhile and count():

for factor in itertools.takewhile(lambda f: f*f <= num, itertools.count(start=2)):
    ...

Yeah, even if we put everything on one line, I'm not sure that helps any. Meh.

Lastly, factor-checking. The factors you are checking, in order, are:

2, 3, 4, 5, 6, 7, 8, ...

That is pretty inefficient. First, once you check 2, you don't need to check any of the even numbers. Similarly for 3 and multiples of 3. A more efficient check would be:

2, 3 then 5, 7, 11, 13, 17, 19, 23, ...

Basically alternating adding 2 and 4 from then on out. We end up with just odd numbers that aren't multiples of 3. So we only have to check 2 numbers out of every 6. We could write a generator for that:

def potential_factors(num):
    yield 2
    yield 3

    fact = 5
    incr = 2
    while fact * fact <= num:
        yield fact
        fact += incr
        incr ^= 6

Which we can use:

def max_factor_mine(num):
    assert num >= 2

    def potential_factors():
        yield 2
        yield 3

        fact = 5
        incr = 2
        while fact * fact <= num:
            yield fact
            fact += incr
            incr ^= 6

    best = None
    for factor in potential_factors():
        while num % factor == 0:
            best = factor
            num /= factor

    return num if num > 1 else best

That's about as good as you're going to get with this approach. If you want better performance, you'd have to get a different algorithm. In this answer, I show an approach with Pollard's rho, which would give a dramatic performance improvement just by doing something completely different:

+---------------------+----------+--------------------+---------+
|                     | OP       | OP w/fewer factors | Pollard |
+---------------------+----------+--------------------+---------+
| 600851475143        |  0.003s  |  0.002s            |  0.092s |
| 145721 * 145723     |  0.298s  |  0.174s            |  0.018s |
| 1117811 * 1117813   |  2.286s  |  1.331s            |  0.262s |
| 18363797 * 18363799 | 40.379s  | 21.895s            |  0.825s |
+---------------------+----------+--------------------+---------+


Lightweight Tic-Tac-Toe in Python This is a completely functional python script of the Tic-Tac-Toe game. I've used python's set container for almost all data structure and operation. The code is small and somewhat readable. I would very much appreciate your opinion and suggestions to improve it in terms of speed, readability and refactoring wherever necessary. Please post in your answer any modifications needed in the algorithm used.

The grid is marked from 1 to 9 horizontally.
The TTT class handles all the tasks.
self.ploys is a dictionary containing all the solution combinations.

{1: [{1,2,3}, {1,4,7}, {1,5,9}], 2:[{1,2,3}, {2,5,8}], ...}

The Main Loop displays the output, cycles through each player's turn and checks the result.
self.PlayUser and self.PlayCom plays respective turns.

#!/usr/bin/env python3

import os

erase = 'cls' if os.name == 'nt' else 'clear'
# 3x3 grid and 8 solution sets
grid = set(range(1,10))
sols = ({1,2,3}, {4,5,6},
        {7,8,9}, {1,4,7},
        {2,5,8}, {3,6,9},
        {1,5,9}, {3,5,7})

class TTT(object):

    def __init__(self):
        # Valid combinations (shortens during game)
        self.ploys = {k:[s for s in sols if k in s] for k in grid}
        # Holds two sets; user moves and com moves
        self.umoves = set()
        self.cmoves = set()

    def GameOver(self, moves):
        """Returns True if the puzzle is solved, False if it's a tie."""
        if any(s.issubset(moves) for s in sols):
            return True
        elif (self.umoves|self.cmoves) == grid:
            return False

    def PlayUser(self):
        """Plays the user's turn"""
        umove = int(input())
        # Filter invalid moves
        while umove not in grid or (umove in self.umoves or
            umove in self.cmoves):
            umove = int(input("Invalid Move "))
        self.umoves.add(umove)
        # Modify ploys
        del self.ploys[umove]
        for p, s in self.ploys.items():
            for i in [i for i,n in enumerate(s) if umove in n]:
                del self.ploys[p][i]

    def PlayCom(self, cmove=None, best=None):
        """Plays the com's turn"""
        for p, s in self.ploys.items():
            # Check if com can win
            if any(n.issubset({p}|self.cmoves) for n in s):
                cmove = p
                break
            # Check if user can win
            elif any(n.issubset({p}|self.umoves) for n in sols if p in n):
                cmove = p
            # Get the next best move
            elif best is None or len(s) > len(self.ploys[best]):
                best = p
        cmove = cmove or best
        self.cmoves.add(cmove)
        del self.ploys[cmove]

    def Show(self, cols=3, com="X", usr="O", fill=" "):
        """Prints the grid in the command-line."""
        os.system(erase)
        for g in grid:
            if g in self.cmoves:
                print(com, fill, end="") if g % cols else print(com)
            elif g in self.umoves:
                print(usr, fill, end="") if g % cols else print(usr)
            else:
                # Replace g with fill for clean grid
                print(g, fill, end="") if g % cols else print(g)

    def Main(self):
        """Stars the game."""
        from itertools import cycle

        text = ("You Won", "You Lost")
        move = (self.umoves, self.cmoves)
        play = (self.PlayUser, self.PlayCom)

        for t, m, p in cycle(zip(text,move,play)):
            self.Show()
            p()
            self.Show()
            status = self.GameOver(m)
            if status is not None:
                print(t) if status is True else print("Game Over.")
                exit()

if __name__ == "__main__":
    game = TTT()
    game.Main()

The logic behind this section seems to be a little off.

for p, s in self.ploys.items():
    for i in [i for i,n in enumerate(s) if umove in n]:
        del self.ploys[p][i]

When you delete the value at self.ploys[p][i] this essentially means that:

self.ploys[p][i + 1] is now at position self.ploys[p][i]
self.ploys[p][i + 2] is now at position self.ploys[p][i + 1]
etc

Because the comprehension in the second line finds the indices to the original array, all the proceeding deletes will delete the element 1 index ahead of their target, the next delete will be 2 ahead, and so on.

To fix this you can just iterate through the loop backwards.

for p, s in self.ploys.items():
    for i in [i for i,n in enumerate(s) if umove in n][::-1]:
        del self.ploys[p][i]

Along with that, I think it would help readability to give variables in your loop meaningful names.

It's much less cryptic as:

for ploy_index, soln_list in self.ploys.items():
    for soln_index in [i for i, soln in enumerate(soln_list)
                      if umove in soln][::-1]:
        del self.ploys[ploy_index][soln_index]

I notice is that in PlayUser you write the following.

while umove not in grid or (umove in self.umoves or
    umove in self.cmoves):
    ...

These brackets are a little confusing. When line length is a concern, you should put the entire expression in brackets or use the \ continuation. And although not explicitly in PEP8, I think it also looks better to add extra indentation to easily identify the condition from the logic.

while (umove not in grid or
       umove in self.umoves or
       umove in self.cmoves):
    ...


Edit file name method - run time complexity I wrote a function in Java that edit file name, and replace each space char into dash char.
Currently I iterate all the files in a specific directory, iterate in each file name, creating a new file name, and replace the file in the directory.

  I guess that the current complexity is \$O(N*M)\$ with

  \$N\$ being the number of files in directory and
  \$M\$ being the number of chars in each file.

Can the run-time-complexity be improved?

public static void editSpace(String source, String target) {

    // Source directory where all the files are there
    File dir = new File(source);
    File[] directoryListing = dir.listFiles();

    // Iterate in each file in the directory
    for (File file : directoryListing) {

        String childName = file.getName();
        String childNameNew = "";

        // Iterate in each file name and change every space char to dash char
        for (int i = 0; i < childName.length(); i++) {

            if (childName.charAt(i) == ' ') {
                childNameNew += "-";
            } else {
                childNameNew += childName.charAt(i);
            }
        }

        // Update the new directory of the child
        String childDir = target + "\\" + childNameNew;

        // Renaming the file and moving it to a new location
        if (!(childNameNew.equals(""))
                && (file.renameTo(new File(childDir)))) {

            // If file copied successfully then delete the original file .
            file.delete();

            // Print message
            System.out.println(childName + " File moved successfully to "
                    + childDir);
        }

        // Moving failed
        else {
            // Print message
            System.out.println(childName + " Failed to move the file to "
                    + childDir);
        }
    }

}

Hello and Welcome to Code Review. The runtime complexity in terms of reading characters (n files * m characters) for substitution cannot be improved, you can use the String replace(char oldChar, char newChar) method:

String childName = file.getName();
String childNameNew = "";
for (int i = 0; i < childName.length(); i++) {
    if (childName.charAt(i) == ' ') {
        childNameNew += "-";
    } else {
        childNameNew += childName.charAt(i);
    }
}

You can substitute your block with one line:

String childNameNew = childName.replace(' ', '-');

From Java 7 it is discouraged using File renameTo method, you can use from class Files the move method and you can write the mv file part of your code in this way:

// Calculate oldPath and newPath for your file with Paths.get method
try {
    Files.move(oldPath, newPath, StandardCopyOption.REPLACE_EXISTING);
    System.out.println(oldPath + " File moved successfully to " + newPath);
} catch (IOException e) {
    System.out.println(oldPath + " Failed moved the file to " + newPath);
}


JS how to optimize function that finds the text value in gematria Hello guys i wrote this function code:

function gematria(text){
  var newText = text.toLowerCase(); // handle capital letters
  var startValue=0; //initialize value to zero
  for(let i=0 ; i<newText.length; i++){ // loop through every letter on the text
    let currentCharValue = newText.charCodeAt(i) - 96; // get the distance from the 'a'
    startValue +=currentCharValue;
  }
  return startValue
}

gematria("someText");

it works well but is there any way to do it more short? or better? how good is that code for doing the task?

The shortest approach is to parse the character with a base of 36 and get the delta of 9 for summing.

function gematria(text) {
    return [...text].reduce((s, c) => s + parseInt(c, 36) - 9, 0);
}

console.log(gematria("someText"));


Counting blanks, tabs, and newlines K&R 1.8 exercise:

  Write a program to count blanks,tabs, and newlines.

Is this a viable solution to the problem and is it properly formatted? I have never programmed in C before, so I just wanted to see if this is proper c programming as it were.

#include <stdio.h>

/* count blanks,tabs and new lines*/

int main(void)
{
    /* initializes blank,tab,newline and c
    sets them all to 0*/
    int blank,tab,newline;
    int c;
    blank = 0;
    tab = 0;
    newline = 0;

    while((c = getchar()) != EOF)
        if (c =='\n')
            ++newline;
        if (c == ' ')
            ++blank;
        if (c == '\t')
            ++tab;
        printf("Blanks: %d\nTabs: %d\nLines: %d\n", blank, tab, newline);
        return 0;
}

That is a great start.  It's almost perfect logically.

First thing that caught my eye however is the lack of indentation.  It could just be that something got lost in the transition between your code and your post here, I don't know.  But you should indent whenever starting a new block.  You indented the if bodies, that's good, but you missed the function body of main(), the while block.  Had you done this, you would have been able to identify the following problem in your code.

The body of the while loop consists only of the first if statement that is counting newlines.  You are effectively counting only newlines and not counting the blanks or tabs.  This is how you have your code written but let's include some correct indentation:

while((c = getchar()) != EOF)
    if (c == '\n')
        ++newline;
if (c == ' ')
    ++blank;
if (c == '\t')
    ++tab;

Do you see the problem now?

There's a couple of ways to fix this but let me start with what you should have done always.  If you ever have a block that would span multiple lines, you should always wrap the block in curly braces.  That way there's no confusion what goes where.  It's more forgivable to not include them if it's just a single line (just like you have in your individual if statements) but all three of them belongs to the while body so they should be in braces.

The other thing to about this code is that those three if statements are mutually exclusive.  Only one of them could ever be hit for a given character, c.  So you shouldn't have created a series of if statements like you did here, but instead a set of if/else blocks.  That way the following if statements don't get executed if the preceding one was satisfied.
This could have fixed the logical error for the while loop too but it's still important to include the braces regardless so there's no confusion.

That block should have looked more like this:

while((c = getchar()) != EOF)
{
    if (c == '\n')
        ++newline;
    else if (c == ' ')
        ++blank;
    else if (c == '\t')
        ++tab;
}

Now there's some minor style issues in your code.  You can follow these if you wish, it's just personal preference.  But it could help other people read your code easier with little chance for confusion.

Your variable declarations should go on separate lines and the initializations of the variable should occur in the declaration.  I'd also pluralize the variables since they are used as counters.  In your head, if you read out the values of the variables what makes more sense?  "I have 5 tab," or "I have 5 tabs?  The comment was off a bit, we're not initializing c here, just declaring it.

Do this:

/* initializes blanks, tabs, and newlines and sets them all to 0 */
/* declares c to hold the read-in characters */
int c;
int blanks = 0;
int tabs = 0;
int newlines = 0;

And to a lesser degree, a call to printf() with parameters should be printed a single line at a time.  The newline characters sprinkled in the format string makes it less clear how many lines this will print out.

Do this:

printf("Blanks: %d\n", blanks);
printf("Tabs: %d\n", tabs);
printf("Lines: %d\n", newlines);

So applying all of the above, your code would look like this:

#include <stdio.h>

 /* count blanks, tabs and newlines */

int main(void)
{
    /* initializes blanks, tabs, and newlines and sets them all to 0 */
    /* declares c to hold the read-in characters */
    int c;
    int blanks = 0;
    int tabs = 0;
    int newlines = 0;

    while((c = getchar()) != EOF)
    {
        if (c =='\n')
            ++newlines;
        else if (c == ' ')
            ++blanks;
        else if (c == '\t')
            ++tabs;
    }

    printf("Blanks: %d\n", blanks);
    printf("Tabs: %d\n", tabs);
    printf("Lines: %d\n", newlines);

    return 0;
}


Handler for bidirectional unit conversion form Building a form where values can be entered in either pounds or kilos.

Script is...

function onlyNumber(fld) {
     if(fld.value.match(/[^0-9.]/)) {
     fld.value=fld.value.replace(/[^0-9.]/g,'');
     }
}
function convertUnit(sense, value) {
    retValue = 0;
    if (isNaN (value)) { alert ('Non-numeric value');  return 0; }
    value = parseFloat (value);
    var factor = 0.4536;
    if (sense == 'kg2lb') {
         retValue = value/factor;
         retValue = Math.round(retValue);
    }
    else if (sense == 'lb2kg') {
         retValue = value*factor;
         retValue = Math.round(retValue);
    }
    return retValue;
}

Works fine but I have to repeat this on every input:

onblur="this.form.kg_va.value=convertUnit('lb2kg',this.value);"
onkeyup="onlyNumber(this);" onfocus="if(this.value == '0') { this.value = ''; }"

Is there some way to simplify the onblur code ?

For example, is there a way to have the function focus on the same var pairs? The var pair being lb_FOOBAR and kg_FOOBAR.

And what about the input functions that are exactly the same?

onkeyup="onlyNumber(this);" onfocus="if(this.value == '0') { this.value = ''; }"

Any way to avoid having to repeat them for each input? Form will have 12 fields.

Instead of inlining the script, use addEventListener instead. To retrieve the elements, you'd need something like getElementsByClassName or querySelectorAll to return an array-like structure of elements. You loop through them, and attach a listener each.

var formInputs = Array.prototype.slice.call(document.querySelectorAll('.form-input'));
formInputs.forEach(function(element){
  element.addEventListener('blur', function(){
    // blur code
  });
  element.addEventListener('keyup', function(){
    // blur code
  });
});

The Array.prototype.slice.call is to convert the array-like structure returned by querySelectorAll into a real array. That's so I can use forEach. But a regular for loop will do, since a NodeList (the array-like structure) has a length property.

Now instead of alert and return, why not throw an error instead?

if(isNaN(value)) throw new Error('Non-numeric value');

Also, instead of putting all your conversions in a single function and sending a "sense", why not create a function for each kind of conversion? It's more verbose, but you don't have to pile up everything in one function.


Rövarspråket Language Encoder and Decoder Used Python 3 for a solution to a Reddit /r/DailyProgrammer challenge.

When we Swedes are young, we are taught a SUPER-SECRET language that only kids know, so we can hide secrets from our confused parents. This language is known as "Rövarspråket" (which means "Robber's language", more or less), and is surprisingly easy to become fluent in, at least when you're a kid...
Rövarspråket is not very complicated: you take an ordinary word and replace the consonants with the consonant doubled and with an "o" in between. So the consonant "b" is replaced by "bob", "r" is replaced with "ror", "s" is replaced with "sos", and so on. Vowels are left intact. It's made for Swedish, but it works just as well in English.
Your task today is to write a program that can encode/decode a string of text into Rövarspråket.

Encoder:
import string

def main():
    word = input('Enter Phrase : ')
    final_word = rövarspråket(word)
    print(final_word)

def is_vowel(letter):
    VOWELS = ['A', 'E', 'I', 'O', 'U', 'Y', 'Å', 'Ä', 'Ö']
    if str.upper(letter) in VOWELS:
        return True
    else:
        return False

def rövarspråket(word):
    new_word = []
    for character in word:
        if character.isalpha() and not is_vowel(character):
            new_word.append(character + 'o' + str.lower(character))
        else:
            new_word.append(character)
    return ''.join(new_word)

if __name__ == '__main__':
    main()

Decoder program is identical to the Encoder except for a different central function of course:
def decode_rövarspråket(word):
    original_word = []
    i = 0
    while i <= (len(word) - 1):
        original_word.append(word[i])
        if word[i].isalpha() and not is_vowel(word[i]):
            i += 3
        else:
            i += 1
    return ''.join(original_word)

I'm newer to Python and one of my main concerns is writing code that is idiomatic and properly "Pythonic". Another is the loop in the decoder program. The algorithm I decided to go with revolves around iterating one character for vowels and three for consonants (to skip the o and repeated letter). Using a for-loop seemed like it would involve jumping through more hoops than just biting the bullet and using a while instead.

Your program has an unused import: import string. You can use tools like flake8, pylint to catch such errors.
I ran your code at pep8online and it pointed out 3 errors:

E302:3:1:expected 2 blank lines, found 1
E302:8:1:expected 2 blank lines, found 1
E302:15:1:expected 2 blank lines, found 1

Basically this means there should be two empty lines after imports, class definition, function definition. Check PEP 8 for more details.

VOWELS variable is well suited to be a global variable. Plus you can convert it to a string to make it easier to read: VOWELS = 'AEIOUYÅÄÖ'.
str.upper() is redundant as str objects already have upper methods, hence letter.upper() in VOWELS is all you need. Similarly str.lower() call can be changed to character.lower().
Note that the in check already returns boolean, hence unless you are trying to be extra explicit you can reduce those 3 lines to return letter.upper() in VOWELS.
You can assign word[i] to a variable inside the while loop to make it easier to read: character = word[i]. Plus i can renamed to better variable like index.
For case insensitive comparison you probably want to read up this thread: How do I do a case insensitive string comparison in Python?

Lastly add annotations to your function definitions so that it becomes more clear what type of arguments a function is expecting and what it returns. Function annotation can also be used to static analysis tools like mypy to catch errors beforehand.

VOWELS = 'AEIOUYÅÄÖ'

def main() -> None:
    word = input('Enter Phrase : ')
    final_word = rövarspråket(word)
    print(final_word)

def is_vowel(letter: str) -> bool:
    return letter.upper() in VOWELS

def rövarspråket(word: str) -> str:
    new_word = []

    for character in word:
        new_word.append(character)
        if character.isalpha() and not is_vowel(character):
            new_word.append('o' + character.lower())

    return ''.join(new_word)

def decode_rövarspråket(word: str) -> str:
    original_word = []
    i = 0
    while i <= (len(word) - 1):
        character = word[i]
        original_word.append(character)
        if character.isalpha() and not is_vowel(character):
            i += 3
        else:
            i += 1
    return ''.join(original_word)


Python line condenser function This is a function I just wrote that tries to condense a set of strings into grouped lines. It actually works, but looks ugly. Is there a better way to achieve the same thing?

Take 4 filtering empty strings first and dropping the if line on the final yield

def mergeToLines(strings, length=40, sep="    "):
    strs = (st for st in sorted(strings, key=len, reverse=True) if st)
    line = strs.next()
    for s in strs:
        if (len(line) + len(s) + len(sep)) >= length:
            yield line
            line = s
        else:
            line += sep + s
    yield line

The idea is that it takes a bunch of strings, and combines them into lines about as long as lineLength. Each line has one or more of the original strings in it, but how many isn't clear until I actually get to slotting them in. Also, note that if a single entry in the list is longer than lineLength, it's still added as its own line (dropping elements is not an option here).

RECURSIVE SOLUTION:
NOTE: it assumes that the input strings is already sorted. You could sort this at every call of the function, but this seems like unnecessary overhead.

def recursiveMerge(strings=[], current_line='', line_length=40, result=[]):
    """ Gather groups of strings into a line length of approx. 40 """

    # base case
    if not strings:
        result.append(current_line)
        return result

    else:
        next_line = strings.pop(0)

        # if current_line + next_line < line_length, add them
        # otherwise append current_line to result
        if len(current_line + next_line) < line_length:
            current_line = '    '.join([current_line, next_line]).lstrip(' ')
        else:
            result.append(current_line)
            current_line = next_line

        # recursively call function
        return recursiveMerge(strings, current_line = current_line, result=result)

ITERATIVE SOLUTION:

def myMerge(strings, line_length=40, res=[]):
    """ Gather groups of strings into a line length of approx. 40 """

    # sort the list of string by len
    strings = sorted(strings, key=len, reverse=True)

    # loop through the group of strings, until all have been used
    current_line = strings.pop(0)
    while strings:
        next_line = strings.pop(0)

        # if current_line + next_line shorter than line_length, add them
        # otherwise, append current_line to the result

        if len(current_line + next_line) < line_length:
            current_line = '    '.join([current_line, next_line])
        else:
            res.append(current_line)
            current_line = next_line

    # add any remaining lines to result
    res.append(current_line)

    return res