Populating form contents from GET - especially radio buttons I have a form written in the usual way. After submitting, if there are errors in the form, the form page itself is the target of the redirect, with a query string appended. The query string contains the error messages, but also the values submitted with the form. The query string and its format are provided by a 3rd-party, so I cannot switch to using POST, nor make any other changes on their processing logic. I just get the string and make the best of it.

Since the URL is "different", the form's values aren't preserved. The browser treats it as a fresh form and therefore blanks it. Not good, since the user won't want to re-enter everything. So, the first step was deciding whether to use JavaScript or PHP to repopulate the form, and I went with PHP.

Consequently, the form now has entries that look a little something like this:

  How many years have you been in business?

    <input id="partner_business_years" name="partner_business_years" class="element text small" type="text" maxlength="255" value="<?php echo $_GET['partner_business_years']; ?>"/>

Is this considered a ridiculous approach? If I used JavaScript, I could use an iterator to run through all the fields in one little loop, instead of individually populating each value attribute with PHP. But server-side seems to be the right place to do this since the form doesn't otherwise rely on JavaScript.

The next question: I have some Yes/No questions in radio button form. The query string will therefore contain values like will_relocate=yes. In my mind, I want to just say "Select the appropriate radio based on the query string", but there's no real concept of selecting for radios... there's a shared name with different value options.

Here's the original HTML:

  <input id="relocate_1" name="will_relocate" class="element radio" type="radio" value="yes" />
Yes

  <input id="relocate_2" name="will_relocate" class="element radio" type="radio" value="no" />
  No

I can get it working by doing something like this:

<input id="relocate_2" name="will_relocate" class="element radio" type="radio" value="no" <?php if($_GET['will_relocate'] && $_GET['will_relocate'] == "no") { echo 'checked'; } ?>/>

(with a matching one for "yes"... and as many pairs of these as needed)

But it strikes me as hideous. I'm just a hack at PHP, and I'm sure I could break this out into a function, but I would still have a pair of <?php selectRadio('will_relocate') ?> - type calls throughout my document, which is barely better.

Any ideas? Is my approach just completely nasty from the get-go? Should I use JavaScript after all?

I'm open to JavaScript-based suggestions for form re-populating, but not as the sole validator (I don't have and wouldn't choose that option).

I went ahead with the one-by-one re-populating from GET (on a timeline and all that!). The form doesn't need to be highly reusable and won't be modified very frequently after roll-out, so in this particular case, it was a matter of 'getr done' rather than 'getr done the best way'.

For the radio buttons, I ended up with a series of these:

  <input id="partner_systemIntegration_1" name="partner_systemIntegration" class="element radio" type="radio" value="yes"  <?php if(($_GET['partner_systemIntegration']) && $_GET['partner_systemIntegration'] == "yes") { echo 'checked'; } ?> />
  Yes

  <input id="partner_systemIntegration_2" name="partner_systemIntegration" class="element radio" type="radio" value="no"  <?php if(($_GET['partner_systemIntegration']) && $_GET['partner_systemIntegration'] == "no") { echo 'checked'; } ?>  />
  No

It's klunky, but when I tried to break it out into a function it failed. I'm using WordPress and a plugin that allowed PHP in the post/page content itself. When trying to define and use a function, I got some sort of eval error/warning, so I'm just doing it one at a time. Not the epitome of elegance and not my ideal choice, but now I can move on.


HackerRank "Ransom Note" challenge I have solved the Ransom Note
 challenge on HackerRank using JavaScript / ECMAScript 6. The challenge goes like this:

A kidnapper wrote a ransom note but is worried it will be traced back to him. He found a magazine and wants to know if he can cut out whole words from it and use them to create an untraceable replica of his ransom note. The words in his note are case-sensitive and he must use whole words available in the magazine, meaning he cannot use substrings or concatenation to create the words he needs.
Given the words in the magazine and the words in the ransom note, print Yes if he can replicate his ransom note exactly using whole words from the magazine; otherwise, print No.
Input Format
The first line contains two space-separated integers describing the respective values of (the number of words in the magazine) and (the number of words in the ransom note).
The second line contains space-separated strings denoting the words present in the magazine.
The third line contains space-separated strings denoting the words present in the ransom note.

My solution uses JavaScript Map objects (as suggested by the name of the challenge) and passes all tests with no timeouts. However, the solution itself is very literal, as in, it removes out each word in the ransom note from the magazine, unless the needed word is not available.
Note that the portion of code above ignore above this line is included with all challenges on the site, I left it in for the sake of completeness, so that anyone can copy-paste the whole block into the challenge page, if they wish.
process.stdin.resume();
process.stdin.setEncoding('ascii');

var input_stdin = "";
var input_stdin_array = "";
var input_currentline = 0;

process.stdin.on('data', function (data) {
    input_stdin += data;
});

process.stdin.on('end', function () {
    input_stdin_array = input_stdin.split("\n");
    main();
});

function readLine() {
    return input_stdin_array[input_currentline++];
}

/////////////// ignore above this line ////////////////////

const addValueOrIncrementCount = (value, map) => {
    let count = map.get(value)
    if (count) {
        map.set(value, ++count)
    } else {
        map.set(value, 1)
    }
    return count
}

const decrementCountIfPossible = (value, map) => {
    let count = map.get(value)
    if (!count) {
        return null
    } else {
        map.set(value, --count)
        return count
    }
}

const mapWordCounts = inputString => {
    const output = new Map()
    let occurrences
    for (let word of inputString.split(" ")) {
        addValueOrIncrementCount(word, output)
    }
    return output
}

const isRansomComposedFromMagazine = (ransom, magazine) => {
    const ransomMap = mapWordCounts(ransom)
    const magazineMap = mapWordCounts(magazine)
    let ransomWordFromMagazine
    for (let [word, count] of ransomMap.entries()) {
        while (count > 0) {
            ransomWordFromMagazine = decrementCountIfPossible(word, magazineMap)
            if (ransomWordFromMagazine === null) {
                return false
            }
            count--
        }
    }
    return true
}

const getProgramInput = () => {
    const firstLine = readLine().split(' ')
    const magazineWordCount = parseInt(firstLine[0])
    const ransomWordCount = parseInt(firstLine[1])
    const magazine = readLine()
    const ransom = readLine()
    return {
        magazineWordCount: magazineWordCount,
        ransomWordCount: ransomWordCount,
        magazine: magazine,
        ransom: ransom
    }
}

const main = () => {
    const input = getProgramInput()
    console.log(isRansomComposedFromMagazine(input.ransom, input.magazine) ? "Yes" : "No")
}

This code reads like a story, very nice, though at times a bit verbose.

The value returned from addValueOrIncrementCount is never used,
so you could drop the return statement.

decrementCountIfPossible returns a count, or null if the count cannot be decremented. It would be simpler and more natural to use if it returned boolean, because the returned count is not used anyway.

In mapWordCounts the variable occurrences is declared but never used.

Note that the map of word counts for ransomMap is not really necessary.
It's possible to simply loop over the words, updating the magazineMap,
and return false whenever the magazineMap doesn't have the required word.

You could use the || operator to simplify addValueOrIncrementCount:

const addValueOrIncrementCount = (value, map) => {
    map.set(value, 1 + (map.get(value) || 0))
}


Binary Search Optimization Can anyone give me recommendations on how to improve my code?

using System;
namespace mybinsearch2
{
    class Program
    {
        static void Main(string[] args)
        {
            int n = int.Parse(Console.ReadLine());
            int key = int.Parse(Console.ReadLine());
            int[] arr = new int[n];  int min = 0;int max = n - 1;
            int mid = min + (max - min) / 2;int temp;
            for (int i = 0; i <= n - 1; i++)
            {
                arr[i] = i *5;
            }
            while (true)
            {
                temp = arr[mid];
                if (arr[mid] == key)
                {
                    Console.WriteLine("Position is: " + mid); break;
                }
                else if (arr[mid] < key) min = mid;
                else max = mid;
                mid = min + (max - min) / 2;
                if(arr[mid]==temp)
                {
                    Console.WriteLine("Not Found"); break;
                }
            }

       }
    }
}

If you have repeating values I think the second if(arr[mid]==temp) can fail.

Separate the create the test from the program (method).  Return a value from method and print it in the test.

The first if (arr[mid] == key) that is the least likely condition so put it last and  it can just be an else as you should test for < and > so = is all that is left.

You repeatedly reference arr[mid] when you already have it temp = arr[mid];

You have max = n - 1; but you don't use it in the create test loop

Put assignments on separate lines
This still has problems - this is just cleaning up existing code

static void Main(string[] args)
{
    int n = int.Parse(Console.ReadLine());
    int key = int.Parse(Console.ReadLine());
    int[] arr = new int[n];
    int min = 0;
    int max = n - 1;
    int mid = min + (max - min) / 2;
    int temp;
    for (int i = 0; i <= max; i++)
    {
        arr[i] = i * 5;
    }
    while (true)
    {
        temp = arr[mid];
        if (temp < key)
        {
            min = mid;  // this seems backwards but that is what you have
        }
        else if (temp > key)
        {
            max = mid;
        }
        else
        {
            Console.WriteLine("Position is: " + mid);
            break;
        }
        mid = min + (max - min) / 2;
        if (arr[mid] == temp)  // this still has problems
        {
            Console.WriteLine("Not Found");
            break;
        }
    }
}


Optimizing Multiplication Table I have a class that takes an array of integers and produces a multiplication table.

class MultiplicationTable
  def initialize(num_ary)
    @num_ary = num_ary.to_a
  end

  def generate
    a1 = @num_ary.clone
    a2 = @num_ary.clone

    rows = []
    rows << a1

    a2.shift

    a2.each do |x|
      cols = [x]
      a1[1..-1].each do |y|
        cols << (x * y)
      end
      rows << cols
    end

    rows
  end
end

So this:

MultiplicationTable.new(1.upto(5)).generate

produces this output:

[
  [1, 2, 3, 4, 5],
  [2, 4, 6, 8, 10],
  [3, 6, 9, 12, 15],
  [4, 8, 12, 16, 20],
  [5, 10, 15, 20, 25]
]

Here are my results from benchmarking:

                         user     system      total        real
100 entry table      0.000000   0.000000   0.000000 (  0.001295)
1,000 entry table    0.120000   0.010000   0.130000 (  0.132325)
10,000 entry table  15.290000   0.380000  15.670000 ( 15.747799)

Is there anything I can do to speedup/improve up this code?

You only need to compute half the table because it is symmetrical; for your line

a1[1..-1].each do |y|

instead do

a1[x..-1].each do |y|

and if you ever need to look up a * b where b > a, instead look up b * a

This will create a multiplication table that looks like this:

# old table
  1  2  3  4  5
1 1  2  3  4  5
2 2  4  6  8 10
3 3  6  9 12 15
4 4  8 12 16 20
5 5 10 15 20 25

# new table
  1 2 3  4  5
1 1 2 3  4  5
2   4 6  8 10
3     9 12 15
4       16 20
5          25

# final multiplication table
arr = [
  [1,2,3,4,5],
  [4,6,8,10],
  [9,12,15],
  [16,20],
  [25]
]

To access a*b where a >= b, the answer is in arr[a-1][b-a]
To access 3*4, the answer is in row 3-1, element 4-3 = arr[3-1][4-3] = arr[2][1] = 12
To access 5*2 (= 2*5), the answer is in row 2-1, element 5-2 = arr[2-1][5-2] = arr[1][3] = 10
To access 100x100 (assuming the range goes high enough), arr[99][0]

You'll need to do something slightly different if you are passing an array that isn't 1..x - in that case, first look up the index of each of the two numbers in the original array and use THEM in the arr[a-1][b-a] lookup.  I suspect you'll need to subtract 1 from the indices but I haven't worked it out in my head.


UVa 11340 Newspaper challenge (sum of letter values in an article) I am solving the UVA 11340 problem:

Problem Description
News agency pays money for articles according to some rules. Each
character has its own value (some characters may have value equals to
zero). Author gets his payment as a sum of all character’s values in
the article. You have to determine the amount of money that news
agency must pay to an author.
Input
The first line contains integer N (0 < N ≤ 5), it is a number of
tests. Each test describes an integer K (0 < K ≤ 100), the number of
paid characters. On next K lines there are table of paid characters
and its values (character values are written in cents). If character
can not be found in the table, then its value is equal to zero. Next,
there is integer M (0 < M ≤ 150000). Next M lines contain an article
itself. Each line can be up to 10000 characters length. Be aware of a
large input size, the whole input file is about 7MB.
Output
For each test print how much money publisher must pay for an article
in format ‘x.yy$’. Where x is a number of dollars without leading
zeros, and yy number of cents with one leading zero if necessary.
Examples: ‘3.32$’, ‘13.07$’, ‘71.30$’, ‘0.09$’.

Code
My code is written using Java. I used HashMap data structure. But I am getting Time Limit Exceeded (TLE) error from the online judge. I am not seeing any other place to make improvement for better timing. Can anyone help me to find my mistake that is causing TLE? Or is there a better algorithm to solve this problem?
import java.util.HashMap;
import java.util.Scanner;

public class UVA11340 {
    public static void main(String[] args) {
        Scanner sc=new Scanner(System.in);
        int t=sc.nextInt();
        while(t-->0)
        {
            int k=sc.nextInt();
            HashMap<Character, Integer> map=new HashMap<>();
            for(int a=0;a<k;a++)
            {
            String s=sc.next();
            int v=sc.nextInt();
            map.put(s.charAt(0),v);
            }
            int l=sc.nextInt();
            double cost=0;
            sc.nextLine();
            for(int b=0;b<l;b++)
            {
                String str=sc.nextLine();
                for(int i=0;i<str.length();i++)
                {
                    char c=str.charAt(i);

                    cost=cost+(map.get(c)==null?0:map.get(c));
                }
            }
            cost=cost/100;
            System.out.printf("%.2f$\n",cost);
        }
        sc.close();
    }
}

I am going to focus only on the time aspect of your code, not the style or anything else, as that is not what you have asked for. i hope there will be others who will comment on that too.
Obvious Culprits

Character and Integer. Primitive to Object boxing costs time! Plus, a HashMap only has amortized constant lookup time, not true constant lookup time. What you want here is a pure old int array, somehow indexable by chars. That's possible, pretty easily, in fact. Plus, you are reinitializing the HashMap on the turns of your test counter loop. You could just call clear() on it, if you end up using it at all. For an array, however, reinitializing it is the way to go when clearing it (reinitialization is amortized constant time (including GC), not linear like clearing the array. It's linear time in both cases for the HashMap, and there's additional GC overheads).

Lots and lots of I/O in a tight loop. That's always a bad idea, as I/O form most performance bottlenecks. Maybe you should take the input in separately before you do the processing?

(2) brings us to using separate methods. Now that your I/O and program logic have become loosely coupled, you can easily separate them into 2 functions. Trust me, a function call overhead is not too much of a problem here. (I don't do this is my "Improved (?) code below" as it's mostly a stylistic issue, but you should).

java.util.Scanner uses pattern matching and regexes under the hood. Those can be pretty slow. For faster I/O, you want java.io.BufferedReader and family.

Suggestion

You know, this is a financial calculation. doubles are liable to roundoff errors. You should be using java.math.BigDecimal for financial calculations and java.text.DecimalFormat for formatting numbers. However, in the interest of performance, I'll let this one slide. I don't do this in the code I provide below either.

You should really have some sort of Exception handling. You should also consider JDK 7's try-with-resources statement, which I use, to simplify resource cleanup.

Improved (?) Code
My implementation will trade time for space, which means that more memory will be consumed in exchange for faster execution speed. With the test input available on the UVA online debugging portal (the one for the UVA Online Judge), this code takes ~100ms (Measure-Command on PowerShell) or ~180ms (time through bash on WSL) to cover all test cases, including JVM start-up time. I use Java 1.8, like the UVA platform. The run times are safely much lower than the 1 second of runtime offered by the Online Judge.
import java.io.*;
public class UVA11340 {
    public static void main(String[] args) {
        try(BufferedReader br = new BufferedReader(new InputStreamReader(System.in))){
            int t = Integer.parseInt(br.readLine());
            //System.out.printf("Reading %d inputs\n", t);
            int tableSize = 0xFF;//Extended ASCII table size
            while(t-- > 0) {
                int k = Integer.parseInt(br.readLine());
                //System.out.printf("Reading %d characters for table\n", k);
                int[] table = new int[tableSize];
                for(int a = 0; a < k; ++a){
                    String in=br.readLine();
                    //System.out.println("Read map line:"+new String(in));
                    table[in.charAt(0)] = Integer.parseInt(in.substring(2/*in.indexOf(' ')+1*/, in.length()).trim()/*for bug in input*/);
                }
                int l = Integer.parseInt(br.readLine());
                //System.out.printf("Reading %d lines\n", l);
                double cost = 0.0;
                for(int b = 0; b < l; ++b){
                    String str = br.readLine();
                    int strLength = str.length();
                    //System.out.println("Read input line:"+new String(str));
                    for(int i = 0; i < strLength; ++i){
                        cost += table[str.charAt(i)];//lookup cost of char
                    }
                }
                System.out.printf("%.2f$\n", (cost / 100.0));
            }
        }
        catch(IOException ioe){
            ioe.printStackTrace();
        }
    }
}

I left in the debugging print statements as I feel that they make for reasonable documentation. Side note: Please use functions and proper variable names, like the ones I introduced.
Note:
This has not been tested with the UVA Online Judge, only on my PC.
PowerShell screenshot of timed run:

Bash on WSL screenshot of timed run:


Simplify form validation with function In jQuery, I have been reluctant to create a better form validation, and I feel the time has come for me to do so.

With that said, I have been using this particular type of form validation, and I would like to simplify it.

$('#saveSubmit').on('click', function()
{
  var criteria =
  {
    company: $('#company').val(),
    partnercode: $('#partnercode').val(),
    office: $('#office').val(),
    // few more
  }

  // form validation starts here
  if(criteria.company == "")
  {
    $('#compError').show();
    return false;
  }
  if(criteria.partnercode == "")
  {
    $('#compError').show();
    return false;
  }
  if(criteria.office == "")
  {
    $('#compError').show();
    return false;
  }
  // few more if statements
  else
  {
    // if the form parameters are good, send to processing script
    $.post('process/editUser.php', {criteria:criteria}, function(data)
    {
      if(data.indexOf("Error") >= 0)
      {
        $('#errorModal').modal('show');
        $('#message').text(data);
        return false;
      }
      else
      {
        $('#successModal').modal('show');
        $('#messageSuccess').text(data);
        $('#successModal').on('hidden.bs.modal', function()
        {
          $('#successModal').modal('hide');
          location.reload();
        });
      }
    });
  }
});

All of this works like how it should, but I want to make it less verbose, thus the need for a function.

I want to replace that huge if/else statement within that click event with a function.

You might break apart the code in separate functions, and create a function that opens and closes a modal with a custom text. I didn't check this code, hopefully you get some ideas for improvement!

$('#saveSubmit').on('click',()=>validateForm())

function validateForm()
{
  var criteria =
  {
    company: $('#company').val(),
    partnercode: $('#partnercode').val(),
    office: $('#office').val(),
    // few more
  }

  let error = ""
  if(criteria.company == "")
  {
    error += "No company specified";
  }
  if(criteria.partnercode == "")
  {
    error += "No partner code specified";
  }

  if(error == "") {
     postData(criteria);
  } else {
     showModal(error);
  }

}

function postData(criteria){
    $.post('process/editUser.php', {criteria:criteria},(data)=>checkData())
}

function checkData(data) {
      if(data.indexOf("Error") >= 0) {
        showModal("error " + data, false);
      } else {
        showModal("data posted!", true);
      }
}

function showModal(content, button) {

    $('#modal').show();
    $('#message').text(content);
    if(button){
        $('#modalbutton').show();
        $('#modalbutton').on('click',() => hideModal());
    }
}

function hideModal {
    $('#modal').hide();
}


Extensible Fizzbuzz in Haskell I am getting used to how things should be written/thought about in (idiomatic) Haskell. I wrote a FizzBuzz program that prints to the console. I wanted to generate things from the core info of the divisors and the strings that go with them, and an array of such rules to be processed. This is in some sense a port of my original Purescript question here (deleted), but I'm interested in Haskell conventions independently, especially when they might differ from Purescript.

All improvements are welcome, but I am focusing primarily on "is this a reasonable way a user of Haskell might organize things?". One thing I'm looking to understand is when/why I might prefer making something like Fizzrule a type or newtype of (Int, String) instead of the brand new datatype here. And honestly, mapMaybe is just a carryover from how I'm used to writing things in modern Wolfram Language, so I'm curious if it's the right thing here (maybe it's a special case of something more general I should be thinking about?). Finally, is mapM_ the most natural thing to use for a task like this?

module Main where
import Data.Foldable(fold)
import Data.List(null)
import Data.Maybe(mapMaybe)

data Fizzrule = Fizzrule {divisor :: Int, yell :: String}
rule3 :: Fizzrule
rule3 = Fizzrule 3 "Fizz"
rule5 :: Fizzrule
rule5 = Fizzrule 5 "Buzz"
rule7 :: Fizzrule
rule7 = Fizzrule 7 "Quux"

processrule :: Int -> Fizzrule -> Maybe String
processrule int rule = if int `mod` (divisor rule) == 0
  then Just (yell rule) else Nothing

fizzbuzz :: [Fizzrule] -> Int -> String
fizzbuzz rules int = if null strings then show int else fold strings
  where strings = mapMaybe (processrule int) rules

main :: IO()
main = mapM_ (putStrLn . fizzbuzz [rule3, rule5, rule7]) [1..100]

It would be more usual to replace the if null strings... conditional with a case, and concat is more commonly used than fold for lists:

fizzbuzz :: [Fizzrule] -> Int -> String
fizzbuzz rules int
  = case mapMaybe (processrule int) rules of
      [] -> show int
      strings -> concat strings

Similarly, processrule would more commonly be written with guards than an if-then-else:

processrule :: Int -> Fizzrule -> Maybe String
processrule int rule
  | int `mod` (divisor rule) == 0 = Just (yell rule)
  | otherwise                     = Nothing

For simple structures like Fizzrule, it's pretty common to dispense with field selectors and pattern match directly, so this would probably be further rewritten. Also, it's more usual to use single character arguments like n in preference to int, though str and lst are often used for strings and lists:

processrule :: Int -> Fizzrule -> Maybe String
processrule n (Fizzrule d str)
  | n `mod` d == 0 = Just str
  | otherwise      = Nothing

Then the mapMaybe and processRule functions aren't actually doing much, so it would be more usual to replace them with a list comprehension:

fizzbuzz :: [Fizzrule] -> Int -> String
fizzbuzz rules n
  = case [s | Fizzrule d s <- rules, d `divides` n] of
      [] -> show n
      strings -> concat strings
  where d `divides` m = m `rem` d == 0

Arguably, it might be more usual to keep the "map" in main pure and output all the lines at once.  (Even when run as a pure computation, it's all done lazily, so no worry that you need to wait for the computation to complete if you try to FizzBuzz the first billion integers or even run it on an infinite list [1..]).  Also, note the type signature spacing -- it's always IO () and never IO().

main :: IO ()
main = putStr . unlines $ map (fizzbuzz rules) [1..100]

Naming all the rules seems like overkill, and Fizzrule takes forever to type, so maybe:

{-# OPTIONS_GHC -Wall #-}

module Main where

data Rule = Rule Int String

myrules :: [Rule]
myrules =
  [ Rule 3 "Fizz"
  , Rule 5 "Buzz"
  , Rule 7 "Quux"
  ]

fizzbuzz :: [Rule] -> Int -> String
fizzbuzz rules n
  = case [s | Rule d s <- rules, d `divides` n] of
      [] -> show n
      strings -> concat strings
  where d `divides` m = m `rem` d == 0

main :: IO ()
main = putStr . unlines $ map (fizzbuzz myrules) [1..100]

That looks more idiomatic to me.

Writing the Rule as a newtype:

newtype Rule = Rule (Int, String)

would be silly.  The main point of newtype is to introduce a type and associated constructor that's "free" from a performance standpoint because it's erased at compilation time (while also allowing for certain conveniences related to automatically deriving type class instances).  However, in:

newtype Rule = Rule (Int, String)

the Rule constructor can be erased, but the pair constructor (normally written (,)) is still there.  If you just write a data type in the first place:

data Rule = Rule Int String

then that replaces the (,) constructor with the Rule constructor more explicitly.  There's no advantage here to newtype Rule -- it just requires extra typing when constructing or pattern matching values.

The choice between data Rule = ... and type Rule = (Int, String) is more a matter of personal taste.  I'd probably go with the data Rule type.

To summarize some of the principles used above:

if-then-else statements are rare in Haskell and are usually replaced by case constructs (or the implicit case construct of function definitions), possibly with the help of guards.  It's especially rare to see if null lst then... since this is almost always better written as a case.
Haskellers usually inline helper functions like processrule that are only used in one place and have no general applicability.  In constrast, Haskellers often define trivial functions like divides in where clauses where they improve readability.  I'm arguing here that the list comprehension is more directly readable than the function name processrule, but divides is more readable than the m `rem` d == 0 expression.  It's a judgement call.
At least for "small" data structures, pattern matching is usually used in preference to field selectors.
Pure code is often used in preference to impure code, including where you have an action where additional purity can be factored out (e.g., instead of mapM_ over multiple IO actions, map over pure values and pass that to a single IO action).


Sorting strings with certain restrictions I want to sort a string which contains words and integers with the restriction that if the nth element in the list is an integer it must remain an integer and if it is a word, it must remain a word.

Currently, what I am doing is splitting the input string on spaces, putting the words and integers into their own lists, sorting them appropriately by mapping to a different list, and then injecting the sorted words/integers back into the original input list.

I am trying to find comments on improve the code or looking for a different approach to the problem if there is a better one. Does anyone have any comments?

http://ideone.com/X2pPfg

justWords   = {}
words       = []

justNums    = {}
nums        = []

# Check if value in the input is an integer or a word
def is_number(s):
    try:
        float(s)
        return True
    except ValueError:
        return False

# Replace the values in the dictionary with the correctly sorted words/integers
def go_through(theDict, theList):
    counter = 0
    for k,v in theDict.iteritems():
        theDict[k]   = theList[counter]
        counter      = counter + 1
    return theDict

# Replace the values in the original input
 # list with correctly sorted values of the word/int dicts
def inject(theDict, theList):
    for k,v in theDict.iteritems():
        theList[k] = v
    return theList

if __name__ == "__main__":

    splitInput  = (raw_input("")).split()

    # Sort the words and numbers into their own lists as tuples
    for i,j in enumerate(splitInput):
        if is_number(j):
            justNums[i]  = j
            nums.append(j)
        elif not is_number(j):
            justWords[i] = j
            words.append(j)

    print("%s\n%s\n" % (justWords, justNums))

    words = sorted(words)
    nums  = sorted(nums)

    print("%s\n%s\n" % (words, nums))

    # Replace the values in the dictionaries with the values in the sorted list
    justWords = go_through(justWords, words)
    justNums  = go_through(justNums, nums)

    print("%s\n%s\n" % (justWords, justNums))

    # Inject correctly maped sorted words into the original list
    splitInput = inject(justWords, splitInput)
    splitInput = inject(justNums, splitInput)

    print("%s" % (' '.join(splitInput)))

Input:

car truck 8 4 bus 6 1

Output:

{0: 'car', 1: 'truck', 4: 'bus'}
{2: '8', 3: '4', 5: '6', 6: '1'}

['bus', 'car', 'truck']
['1', '4', '6', '8']

{0: 'bus', 1: 'car', 4: 'truck'}
{2: '1', 3: '4', 5: '6', 6: '8'}

bus car 1 4 truck 6 8

Numeric vs. lexicographic sort

Given this input: 8 4 6 1 12 55,
the output will be 1 12 4 55 6 8.

The question did not explicitly state so, but in the absence of specific instructions, I would have expected a numeric sort to be more appropriate for the numbers.

The root cause of the bug (if you do consider it to be a bug, that is) is that you detect whether each word looks like it could be a float, but you don't actually convert it to a float.  Therefore, the numbers are sorted as if they were strings.

I'm going to assume that numeric sorting is desired.

Style

Whenever you start with an empty list or dict, then populate it using a loop, consider using a list comprehension or dict comprehension instead.  Defining a variable "all at once" using a comprehension is more elegant, and possibly faster as well.

It's worth packaging the solution to the problem in a function, rather as freely floating code.  I suggest calling the function segregated_sort().

Suggested solution

Here's a much shorter way to write it.  Useful techniques include:

Use list comprehensions and dict comprehensions, as mentioned.
Use filter() to help build the equivalent of your nums and words.  (Here, I've generalized support beyond the two types int and str.  Note, however, that float and int would not intermingle with this approach.)
Use iter() to form a queue of sorted items for each type.  Each queue is drained by calling next() on it.

def segregated_sort(input_list):
    """Returns a sorted copy of the input list that maintains the same
    type of data at each index of the output as the type of data at that
    index in the input."""
    types = [type(datum) for datum in input_list]
    sorted_data_by_type = {
        t: iter(sorted(filter(lambda datum: type(datum) == t, input_list)))
        for t in set(types)
    }
    return [next(sorted_data_by_type[t]) for t in types]

def to_int_where_possible(words):
    """Returns a copy of the input list in which any word that can be
    an int is replaced with its int value."""
    def convert(word):
        try:
            return int(word)
        except ValueError:
            return word
    return [convert(word) for word in words]

if __name__ == '__main__':
    split_input = raw_input().split()
    result = segregated_sort(to_int_where_possible(split_input))
    print(' '.join(str(r) for r in result))

Addressing float/int lossiness

To address allow floats and ints to mingle, and to output the strings exactly as they were input, you can convert all numeric values into a Number class.

I've also incorporated @Veedrac's suggestion to abandon filter() in favour of a list comprehension.

class Number:
    def __init__(self, string):
        self.value = float(string)
        self.string = string

    def __str__(self):
        return self.string

    def __cmp__(self, other):
        return cmp(self.value, other.value) or cmp(self.string, other.string)

def segregated_sort(input_list):
    """Returns a sorted copy of the input list that maintains the same
    type of data at each index of the output as the type of data at that
    index in the input."""
    types = [type(datum) for datum in input_list]
    sorted_data_by_type = {
        t: iter(sorted(datum for datum in input_list if type(datum) == t))
        for t in set(types)
    }
    return [next(sorted_data_by_type[t]) for t in types]

def to_number_where_possible(words):
    """Returns a copy of the input list in which any word that looks
    numeric is converted to a Number."""
    def convert(word):
        try:
            return Number(word)
        except ValueError:
            return word
    return [convert(word) for word in words]

if __name__ == '__main__':
    split_input = raw_input().split()
    result = segregated_sort(to_number_where_possible(split_input))
    print(' '.join(str(r) for r in result))


Extension methods for methods and properties that don't use non-public data Generally speaking, I try and write my classes so they are highly cohesive.

Sometimes I have accessors (this problem isn't limited to accessors) which derive their value from non-public data only, which lowers cohesion. (http://www.ndepend.com/metrics.aspx#LCOM)

Take the following class example:

public class Person
{
    public DateTime Birth { get; set; }
    public DateTime? Death { get; set; }

    public double Age
    {
        get
        {
            return (Death ?? DateTime.Now - Birth).TotalDays / 365;
        }
    }
}

Ignoring the probable compile errors and lack of error checking, Age could easily be an extension method GetAge as it doesn't rely on non-public data.

The code would look as follows:

public class Person
{
    public DateTime Birth { get; set; }
    public DateTime? Death { get; set; }
}

public static class PersonExtensions
{
    public static double GetAge(this Person person)
    {
        return (person.Death ?? DateTime.Now - person.Birth).TotalDays / 365;
    }
}

What is a good approach to determine whether something should be an extension method or implemented in the class itself?

Some arguments against doing this that I find extension methods have lower visibility, you have to know the extension exists, while if it was a class property its existence would be obvious.

For languages that don't use extension methods (C++, php, etc.) the extension method could be written as a static method which accepts a Person as it's parameter.

My personal view is create an extension method if:

The class/interface you wish to extend is not created by you.
The class/interface is created by you but the behavior of the extension method is only required in an assembly that doesn't contain the class (e.g. class Person is defined in MyApp.dll, but GetAge() is only required in MyApp.UI.exe).
The behavior can be defined against a more generic abstraction than the class itself where the implementation is the same but the classes don't share a common base. e.g. create an ILifeSpan interface with DateTime DateOfBirth and DateTime DateOfDeath properties and create the extension method public static double GetAge(this ILifeSpan lifeSpan)

FYI extension methods are just a compiler trick, the generated IL just calls the static method, the this keyword just allows you to write less code in visual studio.

when you call person.GetAge();, it is compiled to PersonExtensions.GetAge(person);


Joining couples, red candidates, and blue candidates I have 3 tables:

    Table couples         consist of save_id, id_candidate_red, id_candidate_blue
    Table candidates_red  consist of save_id, id_candidate, enter_match
    Table candidates_blue consist of save_id, id_candidate, enter_match

And I am running the query

 SELECT  *
    FROM  couples as c

    LEFT JOIN candidates_blue as cb
        ON cb.save_id = 3
            AND cb.user_id = c.id_candidate_blue

    LEFT JOIN candidates_red as cr
        ON cr.save_id = 3
            AND cr.user_id = c.id_candidate_red

    WHERE c.save_id = 3
        AND cb.enter_match = 1
        AND cr.enter_match = 1

…which seems pretty simple to me, but as I have a big dataset behind my tables, it takes quite a while to execute.

I'm afraid there's not much we can do to help with this kind of question. There's not much obviously wrong. You likely need to run the query analyzer and add some missing indices and keys. Regardless, there's still an opportunity for improvement, albeit not much performance wise...

 SELECT  *

Do you really need every column? Even with proper keys/indices in place, this will typically force a table scan. Explicitly state only the fields you need to return. It will result in less I/O and could possibly turn a scan operation into a seek. If it doesn't, you'll need to find the missing index to be added.

The next thing you can do is remove the duplication.

LEFT JOIN candidates_blue as cb
    ON cb.save_id = 3
        AND cb.user_id = c.id_candidate_blue

LEFT JOIN candidates_red as cr
    ON cr.save_id = 3
        AND cr.user_id = c.id_candidate_red

WHERE c.save_id = 3
    AND cb.enter_match = 1
    AND cr.enter_match = 1

I suppose you added all the save_id = 3s in an attempt to speed up the query? Don't bother. It won't help. Specifying it once for the couples table is sufficient. Which actually brings me to an optimization. Your query is equivalent to using INNER JOIN, but without any of the benefits. You're not currently getting the unmatched records anyway, so you might as well switch. You might see a significant performance increase.

SELECT  * /* don't forget to specify the fields you actually want */
FROM  couples as c

INNER JOIN candidates_blue as cb
    ON cb.user_id = c.id_candidate_blue

INNER JOIN candidates_red as cr
    ON cr.user_id = c.id_candidate_red

WHERE c.save_id = 3
    AND cb.enter_match = 1
    AND cr.enter_match = 1

One last thing: Very nice formatting! It's extremely easy on the eyes.


Add spaces before uppercase letters I'm trying to convert some very simple C# string utility functions to F# as a learning exercise. Ignore the fact that the C# version is implemented as an extension method for the moment.

The function adds a blank space before any upper case letters (with the exception of the very first char) - so, "ATestString" becomes "A Test String":

return string.Concat(str.Select(x => Char.IsUpper(x) ? " " + x : x.ToString())).TrimStart(' ');

My first pass using F#:

let padWithSpace str =
    (str |> Seq.fold (fun acc elm -> if Char.IsUpper(elm) then acc + (sprintf " %c" <| elm) else acc + (string elm)) "").TrimStart([| ' ' |])

Is there a more concise, or more importantly, idiomatic way to do this in F#?

String.collect is equivalent to Select+Concat.

let padWithSpace =
    String.collect (fun c -> (if Char.IsUpper c then " " else "") + string c)
    >> fun s -> s.TrimStart()


Iterative equation solver in Python In order to solve a equation where the left hand side appears under an integral on the right hand side:

$$
B(p^2) = C\int_0^{p^2}f_1\left(B(q^2),q^2\right)\mathrm{d}q^2 + C\int_{p^2}^{\Lambda^2} f_2\left(B(q^2),q^2\right)\mathrm{d}q^2
$$

I have written the following code to do solve this equation iteratively:

import numpy as np
import time

def solver(alpha = 2.):

    L2 = 1 # UV Cut-off
    N = 1000 # Number of grid points
    maxIter = 100 # Maximum no. of iterations

    # The grid on which the function is evaluated
    p2grid = np.logspace(-10, 0, N, base=10)
    dq = np.array([0] + [p2grid[i+1]-p2grid[i] for i in range(len(p2grid)-1)])

    Bgrid = np.ones(N)
    Bgrid_new = np.empty(N) # Buffer variable for new values of B

    C = alpha / (4*np.pi)

    for j in range(maxIter):

        for i, p2 in enumerate(p2grid):
            # If lower and upper limit of an integral are the same, set to 0
            if i > 0:
                n = i + 1
                int1 = np.add.reduce((p2grid[0:n] * 3*Bgrid[0:n] /\
                        (p2 * (p2grid[0:n] + Bgrid[0:n]**2))) * dq[0:n])
            else:
                int1 = 0

            if i < N - 1:
                int2 = np.add.reduce((3*Bgrid[i:] /
                            (p2grid[i:] + Bgrid[i:]**2)) * dq[i:])
            else:
                int2 = 0
            # Write new values to buffer variable
            Bgrid_new[i] = C * (int1 + int2)

        # Calculate relative error (take the maximum)
        maxError = np.max(np.abs((Bgrid - Bgrid_new)/Bgrid_new))
        # Copy values from buffer to working variable
        Bgrid = np.copy(Bgrid_new)
        # If the change in the last iteration was small enough, stop
        if (maxError < 10**-4):
            break

    print "Finished in", j+1, "iterations, relative error:", maxError
    return p2grid, Bgrid/np.sqrt(L2)

t0 = time.clock()
p2grid, Bgrid = solver()
print "Time:", time.clock() - t0, " seconds"

I am wondering if there a speedups possible:

I am using np.add.reduce instead of np.sum, which is about 0.2 secs faster on my system
I tried to move the code where int1 and int2 are calculated to a separate function, no improvement here
Using [... for enumerate(p2grid)] instead of the for-loop seems not be faster either
The profiler says almost all time is still spent in the reduce method.

I can't see how this code can be optimized further (as most time is spent in a function of the NumPy lib), but I can't believe I wrote the most efficient code either.

If there are no optimizations possible at code level, is there anything else I should try or look into, like different interpreters or something like that?

You don't need line continuation characters inside brackets.

After splitting up your reduce lines, line_profiler says most of your time is actually in

(p2grid[0:n] * 3*Bgrid[0:n] / (p2 * (p2grid[0:n] + Bgrid[0:n]**2))) * dq[0:n]

and

(3*Bgrid[i:] / (p2grid[i:] + Bgrid[i:]**2)) * dq[i:]

Extracting those outside of the loop and pulling a division out of the sum:

t2 = dq * 3 * Bgrid / (p2grid + Bgrid**2)
t1 = t2 * p2grid

for i, p2 in enumerate(p2grid):
    # If lower and upper limit of an integral are the same, set to 0
    if i > 0:
        int1 = np.add.reduce(t1[:i+1]) / p2
    else:
        int1 = 0

    if i < N - 1:
        int2 = np.add.reduce(t2[i:])
    else:
        int2 = 0

    # Write new values to buffer variable
    Bgrid_new[i] = C * (int1 + int2)

gives a good speed boost.

You can also simplify the logic to:

val = 0

if i > 0:
    val = np.add.reduce(t1[:i+1]) / p2

if i < N - 1:
    val += np.add.reduce(t2[i:])

Bgrid_new[i] = C * val

But then note you have a sliding summation; you can do this beforehand with numpy.add.accumulate.

    t2 = dq * 3 * Bgrid / (p2grid + Bgrid**2)
    t1 = t2 * p2grid

    val1_accumulate = np.add.accumulate(t1)
    val2_accumulate = np.add.accumulate(t2[::-1])[::-1]

    for i, p2 in enumerate(p2grid):
        # If lower and upper limit of an integral are the same, set to 0
        val = 0

        if i > 0:
            val = val1_accumulate[i] / p2

        if i < N - 1:
            val += val2_accumulate[i]

        # Write new values to buffer variable
        Bgrid_new[i] = C * val

This is much faster.

Now this loop can probably be vectorized.

Bgrid_new = np.zeros_like(Bgrid)
Bgrid_new[+1:] += val1_accumulate[+1:] / p2grid[+1:]
Bgrid_new[:-1] += val2_accumulate[:-1]
Bgrid_new *= C

All in all, this seems to be a factor of 300-400 faster. You can do other optimizations like

dq = np.zeros_like(p2grid)
dq[1:] = np.diff(p2grid)

and removing the call to copy, but it should already be fast enough.

Then you should focus on cleaning up the code. Spacing is important as are appropriate variable names (snake_case and not-single-letter names). The trick is to make comments redundant:

num_grid_points = 1000 # Number of grid points

Note how the comment can now be removed.

Here's an attempt at a cleaner version. I've also added a couple more speed improvements, because I'm a hopeless addict:

def solver(alpha = 2.):
    uv_cutoff = 1
    num_grid_points = 1000
    max_iterations = 100

    # The grid on which the function is evaluated
    p2_grid = np.logspace(-10, 0, num_grid_points, base=10)

    dq = np.empty_like(p2_grid)
    dq[0], dq[1:] = 0, np.diff(p2_grid)

    b_grid = np.ones(num_grid_points)

    c = alpha / (4 * np.pi)

    for iteration_num in range(max_iterations):
        t2 = 3 * dq * b_grid / (p2_grid + b_grid*b_grid)
        t1 = t2 * p2_grid

        t1_cumsum = np.add.accumulate(t1)
        t2_cumsum = np.add.accumulate(t2[::-1])[::-1]
        t1_cumsum[0] = t2_cumsum[-1] = 0

        b_grid_new = c * (t1_cumsum / p2_grid + t2_cumsum)

        # Calculate relative error (take the maximum)
        max_error = np.max(np.abs((b_grid - b_grid_new) / b_grid_new))

        b_grid = b_grid_new

        if max_error < 10**-4:
            break

    print "Finished in", iteration_num + 1, "iterations, relative error:", max_error
    return p2_grid, b_grid / np.sqrt(uv_cutoff)

I'm not really sure how to rename these, though, because I'm not familiar with the maths.


Handling an openFileDialog in the View of my MVP WinForms project I have a WinForms project in which I am trying to implement the Passive View MVP pattern (meaning no business logic in my Views). Each form is a concrete View with an IView interface to which a Presenter is connected.

I do think i should handle some UI related logic in my View because I am otherwise adding needless complexity to try to handle this in the Presenter. Therefore, I have created a simple function in my View class which validates the result from the file dialog. I did not put this logic in the event handler of the button press because I wanted to avoid duplicate code since I have three of these buttons.

Please let me know if you think this is an appropriate way to implement this. I have posted the function, and the event (I have three button_Click events that each use this function) that utilizes this function below.

Button event that uses the GetFileName function:

// Gets the filename and fires the compliance standard adding event.
private void newComplianceStandardButton_Click(object sender, EventArgs e)
{
    string fileName = GetFileName();

    if (fileName != null)
    {
        AddingComplianceStandard?.Invoke(this, fileName);
    }

    Close();
}

Function that checks the dialog result and returns the filename:

// Opens the openfile dialog and checks the result. If the result is OK, the form is closed and the filename is returned.
public string GetFileName()
{
    DialogResult result = openFileDialog1.ShowDialog();
    string fileName = openFileDialog1.FileName;

    if (result == DialogResult.OK &&
        (Path.GetExtension(openFileDialog1.FileName) == ".txt" || Path.GetExtension(openFileDialog1.FileName) == ".csv"))
    {
        Close();
        return fileName;
    }
    else
    {
        MessageBox.Show("You have selected a file with an illegal extension. Please try again and select a *.txt or a *.csv file");
        return null;
    }
}

In the big if you have the following:

Path.GetExtension(openFileDialog1.FileName) == ".txt" || Path.GetExtension(openFileDialog1.FileName) == ".csv")

Why? You already put openFileDialog1.FileName in a fileName variable. Just use that.

The next thing I would do is define a list of valid extensions:

public readonly string[] ValidExtensions => new string[] { "txt", "csv" }

Then simply:

if (result == DialogResult.OK && ValidExtensions.Contains(Path.GetExtension(fileName).Substring(1)))

We use the .Substring(1) to remove the period.

That way, if you add support for a different file type, you just add it to ValidExtensions.


Haskell: Draw tabulated data given header and rows as lists I need to draw tabulated data given a header as list and rows as list of lists
This is what I came up with. I wouldn't say that I like it much.
Could you please have a look if it can be done better (I am sure it can) and generally point to any error and possible optimization. Maybe someone has done exact same thing before. Thanks.

import Data.Text (justifyLeft, unpack, pack)
import Data.Char (toUpper)

tDraw :: [String] -> [[String]] -> IO()
tDraw h rs = do
  putChar '\n'
  -- draw header
  mapM_ (\x -> putStr x) header
  putChar '\n'
  -- draw separator line
  mapM_ (\x -> putChar x) (replicate (cell * length header) '=')
  putChar '\n'
  -- draw rows
  mapM_ (\x -> row cell x) rs
  where
    cell = if l < 10 then 12 else l + 2
           where l = (maximum . map (\x -> length x) . concat) (h:rs)
    header = map (\x -> strToUp $ unpack $ justifyLeft cell ' ' (pack x)) h
             where strToUp = map (\c -> toUpper c)
    row n l  = do
      mapM_ (\x -> putStr $ unpack $ justifyLeft n ' ' (pack x)) l
      putChar '\n'

It might make sense to use a pretty-printing library to do this.  You can use pretty along with the extension pretty-ncols package; or possibly boxes would work as it's aimed at more 2D textual information.


Am I getting the builder pattern right? Despite being the clever Java coder I am, I noticed I know way too little about various design patterns. The builder pattern caught my eye as something to learn, as I have certainly seen my fair share of telescopic constructors already.

So, I decided to give it a try. The following is a really simple class represents a cell in a table of data. So far a cell can have text (its contents) and a URL (if it is to be rendered as a hyperlink), but I could see myself adding other parameters as well, so there's an inner static class called CellBuilder as well.

import java.net.MalformedURLException;
import java.net.URL;
import java.util.logging.Level;
import java.util.logging.Logger;

/**
 * A class that represents data in a table cell.
 */
public class CellData {
    /** The text that's displayed in the cell. **/
    private final String text;

    /** The URL to which the text should link to, if any. **/
    private final URL url;

    private CellData(CellBuilder builder) {
        this.text = builder.text;
        this.url = builder.url;
    }

    public String getText() {
        return text;
    }

    public URL getUrl() {
        return url;
    }

    /** A builder to avoid telescopic constructors. **/
    public static class CellBuilder {
        private String text = "";
        private URL url = null;

        public CellBuilder text(String text) {
            this.text = text;
            return this;
        }

        public CellBuilder url(String url) {
            try {
                this.url = new URL(url);
            } catch (MalformedURLException ex) {
                Logger.getLogger(CellData.class.getName()).log(Level.SEVERE, null, ex);
            }
            return this;
        }

        public CellData build() {
            return new CellData(this);
        }
    }
}

A new cell would then be created, for example, like this:

CellData cell = new CellData.CellBuilder().text("hello").url("http://www.example.com/foo").build();

Now, I don't know about you, but to me that doesn't look much more easier or cleaner than doing it the traditional way, by passing those arguments to the cell in its constructor. :( Yet another way would be to do it like this:

CellData cell = new CellData();
cell.setText("hello");
cell.setUrl("http://www.example.com/foo");

Why shouldn't I do it like that either? I wonder if this particular use case is still too simple to get the best out of this design pattern, or if I'm doing something else completely wrong?

Notice that this doesn't strictly conform to the architecture presented in Wikipedia, but that's because I read about this way from some other source where it was more like this, and the Wikipedia way with about five classes felt like too complicated for a simple matter like this.

Your building pattern is just right. But the situation (as elsewehere mentioned) to use the building pattern is not. Imagine a situation where there is a (in your case) celldata base class:

 class CellData {
 }

This class is the base of other, more specific, celldata class:

 class CellData_2  extends CellData {
 }

etc.

Now imagine the choice of which specific celldata you are requiring, depending on the given URL your builder method starts to look something like this:

 public CellData build (String URL) {
     if (URL complies to case 1) {
          return new CellData_1;
     ...
 }

That's where the builder pattern should be used.


Summing the digits of a sum of digits of a sum I'm a beginner in Python, and computer languages in general, and I'm totally stumped on how to format this iteration into a function. The iteration takes the sum of someone's birth year, month, and day and adds it together to create a sum of those numbers, and then the second iteration takes the first sum and adds those numbers together to create a final sum.

I have the users input their birthyear, month, and day (all converted to int) and this is the first sum (Example: A bday of 01/01/1997= 1999):

first_sum=b_yr + b_dy + b_mo

Then the first iteration takes the sum and adds the numbers together (Example: 1999 = 1+9+9+9 = 28):

z = first_sum
zstr1=str(z)
accum1=0
for x in zstr1:
    accum1 += int(x)
(accum1)

Then the second iteration takes the first sum and adds those numbers again to create the final sum (Example: 28 = 2+8 = 10):

str2=str(accum1)
accum2=0
for cnt2 in str2:
    accum2 += int(cnt2)

Despite your code being cumbersome, I would like to point out that you did well by writing for x in zstr1, iterating over the characters of the string.  Python beginners often try to iterate over character indexes instead, resulting in unnatural Python code.

I would decompose the problem differently than @jonrsharpe.  sum_digits(n) is a generally useful operation to define as a function.

def sum_digits(n):
    return sum(int(char) for char in str(n))

sum_digits(sum_digits(b_yr + b_mo + b_dy))

Actually, it's not necessary to conflate the summing and the digit breakdown.

def digits(n):
    return [int(char) for char in str(n)]

sum(digits(sum(digits(b_yr + b_mo + b_dy))))

The last line reads just like the problem description: "sum of the digits of the sum of the digits of the year, month, and day".

Here's another implementation of digits(n), using a generator.  This avoids stringification.

def digits(n):
    """Produce the base-10 digits of n from least to most significant."""
    while n:
        yield n % 10
        n /= 10


Class for comparing objects in Python Im curious how to improve this code. I don't like creating objects of four different types of reporters, because this could provide some problems with testing. How about the rest of this code, for example the if-statement below?

EDIT: This is a fragment of application to measure loading time of our website and few other competitors. If our site is slower than one of competitors application should send email, if our site is two times slower then application should send message via sms.

"""Module containing classes for comparing objects"""
import senders
import reporters

class Comparator:
    """Class for comparing objects"""

    def compare(self, compared, competitors):
        reporter = reporters.Reporter()
        rules = ComparatorExtensions(compared, competitors, reporter)

        send_with_email = senders.SendWithEmail()
        send_with_sms = senders.SendWithSms()
        save_to_file = senders.SendToFile()
        show_on_console = senders.ShowOnConsole()

        if rules.rule_valid_compared():
            if rules.rule_compared_smaller():
                send_with_email.send(reporter.format_report())

            if rules.rule_compared_two_times_smaller():
                send_with_sms.send(reporter.format_report())

        report = reporter.format_report()
        save_to_file.send(report)
        show_on_console.send(report)

class ComparatorExtensions:
    """Class for extensions for comparator class"""

    def __init__(self, compared, competitors, reporter):
        self.compared = compared
        self.competitors = (item for item in competitors if self.valid_item(item))
        self._reporter = reporter

    def valid_item(self, item):
        result = 'loading_time' in item
        if not result:
            self._reporter.report_invalid(item)
        return result

    def rule_compared_smaller(self):
        result = False
        for item in self.competitors:
            self._reporter.report_loading_time(item)
            if item.get('loading_time') < self.compared.get('loading_time'):
                self._reporter.report_smaller(self.compared, item)
                result = True

        return result

    def rule_compared_two_times_smaller(self):
        result = False
        for item in self.competitors:
            if (item.get('loading_time') * 2) <= self.compared.get('loading_time'):
                self._reporter.report_smaller_two_times(self.compared, item)
                result = True

        return result

    def rule_valid_compared(self):
        rule_valid = self.valid_item(self.compared)
        if rule_valid:
            self._reporter.report_loading_time(self.compared)
        return rule_valid

Full repository: https://github.com/adbo/benchmark

Use functions whenever possible

I think there are too many classes and you should consider using functions instead: it is easier to test (unit testing).

Use a logger

First thing: the code below make me think that you should use a logger instead of reinventing the wheel:

report = reporter.format_report()
save_to_file.send(report)
show_on_console.send(report)

Here is how you can define a root logger which can log in the console as well as in a file:

import logging

def setup_logger(log_dir, log_file_name):
    # root logger
    logger = logging.getLogger()

    formatter = logging.Formatter(
        "%(asctime)s [%(threadName)-12.12s] [%(levelname)-5.5s]  %(message)s")

    file_hdlr = logging.FileHandler(
        "{0}/{1}".format(log_dir, log_file_name))
    file_hdlr.setFormatter(formatter)
    logger.addHandler(file_hdlr)

    console_hdlr = logging.StreamHandler()
    console_hdlr.setFormatter(formatter)
    logger.addHandler(console_hdlr)

    logger.setLevel(logging.DEBUG)

This setup_logger function should be called at program startup, in your main, for instance:

if __name__ == "__main__":
    setup_logger("my/work/dir", "my_app.log")

You can use this logger anywhere: in each Python module, you can define a global variable named LOG, and use it like this:

LOG = logging.getLogger(__name__)

def my_function():
    LOG.info("log message")

See the How To documentation

wrong usage of generator

In your, competitors is a generator, not a tuple. After having iterate a generator the first time, then the generator is exhausted. It means that the second loop will end immediately.

self.competitors = (item for item in competitors if self.valid_item(item))

The call to rule_compared_smaller will iterate the competitors, then the call to rule_compared_two_times_smaller will do nothing because the competitors is exhausted.

You need to create a list:

self.competitors = [item for item in competitors if self.valid_item(item)]


Finding Common Items in arrays My goal is to get a list of related statementIds in order of most repeated.

In my database I have stems(Words):

+---------+-----------------+
|  Stem   |   StatementId   |
+---------+-----------------+
| Hello   | [2,46,62]       |
| Good    | [1,123,45]      |
| Morning | [23,2436,12312] |
+---------+-----------------+

My steps:

Get all statementIds

[6, 86],[6],[4],[3, 86],[...]
Create an empty object, and everytime i see a number,increase its value (or set to 1 if it hasen't been seen before)

 {2: 1, 3: 4, 4: 1, 6: 11, 85: 1, 86: 5, 87: 1, 89: 1}

Sort the object by the attribute and put it in an array of relevancy.

 ["6", "86", "3", "89", "87", "85", "4", "2"]

Full code:

getAllStatementIdFromStem = async (stems: string[]) => {
    const ids: { [key: number]: number } = {};
    await this.getBulkStem(stems).toArray((stems: Stem[]) =>
      stems.map(({ statements }) =>
        statements.forEach((statementId: number) => {
          typeof ids[statementId] === "undefined"
            ? (ids[statementId] = 1)
            : ids[statementId]++;
        })
      )
    );
    return Object.entries(ids)
      .sort((a, b) => a[1] - b[1])
      .reverse()
      .map(x => x[0]);
  };

Could I be doing something more effectively? I feel like I am doing way too many extra steps.

Overall looks good. I think following improvements can be suggested.

1.Instead of this

typeof ids[statementId] === "undefined"
  ? (ids[statementId] = 1)
  : ids[statementId]++;

How about this for more readability

ids[statementId] = (ids[statementId] || 0) + 1;

2.Instead of doing sort and immediately reverse. You can change sort function (with descending order) so that you can avoid reverse call.


Implement `Penultimate` in Shapeless I attempted to define Penultimate, i.e. a type class for shapeless.HList's that have a second to last element.

import shapeless.{HList, HNil, ::}

object Penultimate {
  type Aux[L <: HList, O] = Penultimate[L] { type Out = O }

  def apply[H <: HList](implicit ev: Penultimate[H]) = ev

  implicit def secondToLast[H, G]: Aux[H :: G :: HNil, H] =
    new Penultimate[H :: G :: HNil] {
      override type Out = H
      override def apply(in: H :: G :: HNil): Out = in.head
    }

  implicit def inductive[H, T <: HList, OutT](
     implicit penult: Aux[T, OutT]
  ): Penultimate[H :: T] = new Penultimate[::[H, T]] {

    override type Out = OutT

    override def apply(in: H :: T): Out = penult.apply(in.tail)
  }
}

trait Penultimate[H <: HList] {
  type Out
  def apply(in: H): Out
}

It appears to work:

scala> Penultimate[Int :: Int :: HNil]
res1: net.Penultimate[shapeless.::[Int,shapeless.::[Int,shapeless.HNil]]] = net.Penultimate$$anon$1@3c37902d

scala> Penultimate[Int :: HNil]
<console>:16: error: could not find implicit value for parameter ev: net.Penultimate[shapeless.::[Int,shapeless.HNil]]
       Penultimate[Int :: HNil]
                  ^

scala> Penultimate[HNil]
<console>:16: error: could not find implicit value for parameter ev: net.Penultimate[shapeless.HNil]
       Penultimate[HNil]
                  ^

But, is it a valid implementation of Penultimate? If not, what flaws does it have?

More static types

One big improvement you could make would be to have the Penultimate.apply method provide the output type in its signature. For example, with your current code you can write this:

scala> val instance = Penultimate[Int :: String :: HNil]
instance: Penultimate[shapeless.::[Int,shapeless.::[String,shapeless.HNil]]] = Penultimate$$anon$1@2819ef2f

scala> instance(1 :: "" :: HNil)
res0: instance.Out = 1

But not this:

scala> val x: Int = instance(1 :: "" :: HNil)
<console>:14: error: type mismatch;
 found   : instance.Out
 required: Int
       val x: Int = instance(1 :: "" :: HNil)
                            ^

If you want to keep track of the output type statically, you have to do it by hand:

val instance = implicitly[Penultimate.Aux[Int :: String :: HNil, Int]]

val x: Int = instance(1 :: "" :: HNil)

Which works, but ugh. It's a lot easier to change your implementation of apply:

def apply[H <: HList](implicit ev: Penultimate[H]): Aux[H, ev.Out] = ev

Now this works just fine:

scala> Penultimate[Int :: String :: HNil].apply(1 :: "" :: HNil): Int
res1: Int = 1

…well, at least it works for hlists with two elements. When we add more things get weird:

scala> val instance = Penultimate[Char :: Int :: String :: HNil]
warning: there was one feature warning; re-run with -feature for details
instance: Penultimate[shapeless.::[Char,shapeless.::[Int,shapeless.::[String,shapeless.HNil]]]]{type Out = ev.Out} forSome { val ev: Penultimate[shapeless.::[Char,shapeless.::[Int,shapeless.::[String,shapeless.HNil]]]] } = Penultimate$$anon$2@49393eeb

scala> val x: Int = instance('a' :: 1 :: "" :: HNil)
<console>:18: error: type mismatch;
 found   : instance.Out
    (which expands to)  ev.Out
 required: Int
       val x: Int = instance('a' :: 1 :: "" :: HNil)
                            ^

This time apply is fine, but the method that provides inductive instances isn't. You can fix it by giving its result type the same kind of treatment:

implicit def inductive[H, T <: HList, OutT](
   implicit penult: Aux[T, OutT]
): Aux[H :: T, OutT] = new Penultimate[H ::  T] {
  override type Out = OutT

  override def apply(in: H :: T): Out = penult.apply(in.tail)
}

Now the following will work just fine:

val instance = Penultimate[Char :: Int :: String :: HNil]
val x: Int = instance('a' :: 1 :: "" :: HNil)

Now we have useful result types for instances (i.e. result types that track the output type statically), and an improved apply that lets us make use of them.

Point of style 1: DepFn1

This is a relatively minor point, but you could define your type class like this:

import shapeless.DepFn1

trait Penultimate[H <: HList] extends DepFn1[H]

This is essentially exactly the same (the Out and apply are still there—you're just getting them from DepFn1), but it's less verbose and clearer about the intent.

Point of style 2: infix vs. not infix

I'd personally avoid writing ::[H, T] in one place and H :: T in others—for a type with a symbolic name like :: I'd always use the infix version. If you prefer the non-infix version, that's also fine, I guess. Mixing them isn't fine, though. :)

Point of style 3: don't just stick the override keyword everywhere

This is again a matter of personal preference, but I'd drop the override keyword from both the type member and the apply implementations in the anonymous new Penultimate class definitions, since your implementations aren't actually overriding anything.

Point of style 4: consistent type parameter names

In several places you use H as a type parameter name for a head type parameter, but in the Penultimate trait definition itself you use it apparently to stand just for "hlist". I'd use L, which also has the benefit of being consistent with the L in the Aux type member.

Point of style 5: explicit apply

Now we're getting into super-nitpick territory, but in general if writing out x.apply(y) isn't necessary to avoid y looking like an explicit implicit parameter, etc., I'd just use x(y).

Point of style 6: import order

More nitpickery: : precedes H, so it's likely that if you're using something like Scalastyle it's going to complain about the import.

Point of style 7: trait vs. abstract class

For a type class like this that will almost certainly never need to be mixed in with another trait, I'd prefer to make it an abstract class, for the reasons given here.

Point of style 8: unique instance names

I'm not sure how strongly I'd be willing to stand by this one if pressed, but I've developed a distaste for type class instance names that I can't reasonably expect to be globally unique. It's very unlikely anyone will be importing the contents of the Penultimate type class here, but if they did, having something called inductive in scope is kind of unhelpful. At the very least I'd probably tack a Penultimate suffix on each method name. Maybe that's unreasonable. I don't know—it probably doesn't matter much either way.

Other than that it looks pretty good to me!


Generating all variations of M*N matrix containing boolean values I've implemented an algorithm that generates all variations of a matrix of size \$M*N\$. My approach is that I see a matrix as a list where the position of elements can be calculated from the position in the matrix. By doing that I can reduce the problem into generating all variations of a list of length \$M*N\$.

The algorithm computes the variations recursively. I start with an empty list and pass it to my function. The list is copied. The copied list gets a 0 and the original list gets a 1 added. Then I call the function recursively with each of both resulting lists. The process is repeated until the length \$M*N\$ is reached. At this point, I have the set of all variations (size = \$2^{M*N}\$).

Now I can compute the arrays from those variations and compose the resulting matrices.

Here is the implementation:

import java.util.ArrayDeque;
import java.util.Arrays;
import java.util.Deque;

class Main {

    static void addVariations(Deque<int[]> stack, int[] variation, int index) {
        if (index >= 0) {
            // clone for next recursion
            int[] variationClone = variation.clone();

            // one gets 0, the other 1 at index
            variation[index] = 0;
            variationClone[index] = 1;

            // next recursion
            addVariations(stack, variation, index - 1);
            addVariations(stack, variationClone, index - 1);
        }
        else {
            stack.push(variation);
        }
    }

    static Deque<int[][]> getVariations(int M, int N) {
        int variationLength = M*N;

        // get all variations that the matrices are base on
        // there are n^r, 2^variationLength of them
        Deque<int[]> variations = new ArrayDeque<>();
        addVariations(variations, new int[variationLength], variationLength - 1);

        // container for resulting matrices
        Deque<int[][]> variationMatrices = new ArrayDeque<>();

        // for each matrix
        for (int i = variations.size() - 1; i >= 0 ; i--) {
            int[][] matrix = new int[N][M];
            int[] variation = variations.pop();

            // for each row add part of variation
            for (int j = 0; j < matrix.length; j++) {
                matrix[j] = Arrays.copyOfRange(variation, j*M, (j + 1)*M);
            }

            // and push the matrix to result
            variationMatrices.push(matrix);
        }
        return variationMatrices;
    }

    public static void main(String[] args) {
        int N = 2, M = 2;
        Deque<int[][]> variations = getVariations(N, M);

        variations.forEach(v -> {
            System.out.println("----");
            for (int i = 0; i < v.length; i++) {
                System.out.println(Arrays.toString(v[i]));
            }
            System.out.println("----");
        });
    }
}

I'd be very happy to get tipps on how to improve the code. I'm especially interested in style and readability of my code.

The expected output is the set of variations. Each variation is a matrix containing only 0s and 1s. Each matrix is represented as a 2-dimensional array where each row is an array.

So e.g. the output for a \$2*2\$ matrix is supposed to be:

[[0, 0], [0, 0]],
[[1, 0], [0, 0]],
[[0, 1], [0, 0]],
[[1, 1], [0, 0]],

[[0, 0], [1, 0]],
[[1, 0], [1, 0]],
[[0, 1], [1, 0]],
[[1, 1], [1, 0]],

[[0, 0], [0, 1]],
[[1, 0], [0, 1]],
[[0, 1], [0, 1]],
[[1, 1], [0, 1]],

[[0, 0], [1, 1]],
[[1, 0], [1, 1]],
[[0, 1], [1, 1]],
[[1, 1], [1, 1]]

The order doesn't matter.

The code does not produce permutations. It produces the subsets of \$M * N\$-strong set. Below assumes that it is the goal of the code.

Recursive solution is correct, but may take too much memory (there are exponentially many subsets, namely \$2^{N*M}\$ of them). I strongly recommend to switch to an iterative approach:

    bool next_subset(int [] subset) {
        for (int i = 0; i < subset.size(); i++) {
            if subset[i] == 1) {
                subset[i] == 0;
            } else {
                subset[i] = 1;
                return true;
            }
        }
        return false;
    }

which takes the same time, linear memory, and can be used to stream the subsets one-by-one.


Extracting files and directories from a provided list into separate lists This code gets a list of all files and directories from a specific location. Thes code below works.

List<string> FullList = new List<string>(LoadFiles("patch.txt"));
List<string> ShortCatalogList = new List<string>();
List<string> ShortFileList = new List<string>();

            for (int i = 0; i < FullList.Count; i++)
                {

                    if (System.IO.Path.HasExtension(FullList[i]))
                    {
                    ShortFileList.Add(FullList[i]);
                    }

                    if (!System.IO.Path.HasExtension(FullList[i]))
                    {
                        ShortCatalogList.Add(FullList[i]);

                    if (Directory.GetFiles(FullList[i]).Length > 0)
                        {
                            string[] subFile = Directory.GetFiles(FullList[i]);
                                for (int io = 0; io < subFile.Length; io++)
                                    {
                                        ShortFileList.Add(subFile[io]);
                                    }
                        }

                    if (Directory.GetDirectories(FullList[i]).Length > 0)
                     {
                            string[] subFolder = Directory.GetDirectories(FullList[i]);

                            for (int io = 0; io < subFolder.Length; io++)
                            {
                                FullList.Add(subFolder[io]);
                            }
                   }
                  }
                 }

I know that I need to protect from permission problem.

Most is said about this code in the other answer, but there is a simplification which can be used to beautify this code a lot.

The Directory class has two methods which returns IEnumerable<string> namely EnumerateFiles() and EnumerateDirectories().

The results of the calls to this methods can be used to call AddRange() on the ShortFileList and FullList.

Like so

List<string> fullList = new List<string>(LoadFiles("patch.txt"));
List<string> shortCatalogList = new List<string>();
List<string> shortFileList = new List<string>();

for (int i = 0; i < fullList.Count; i++)
{

    if (System.IO.Path.HasExtension(fullList[i]))
    {
        shortFileList.Add(fullList[i]);
        continue;
    }

    shortCatalogList.Add(fullList[i]);

    var files = Directory.EnumerateFiles(fullList[i]);
    shortFileList.AddRange(files);

    var directories = Directory.EnumerateDirectories(fullList[i]);
    fullList.AddRange(directories);

}


Monitor and video mode querying I am working on a large project that requires information about the monitor(s). Please don't be confused by the organization of the following structs. The design intent is to be cross-platform, so they don't contain any operating system-dependent types. Currently, I am working on the Windows implementation of the following functions.

Please bear with me as I am a beginner at C++. Even though I have been programming with it for a while now, I am completely self taught, and my understanding isn't comprehensive.

Is there anything I could be doing differently, or maybe better? Also, is there anything I am not understanding correctly? I'm open to any and all suggestions, tips, and criticisms.

/**
 * Monitor structure.
 */
struct tsMonitor
{
    /**
     * Windows specific data.
     */
    typedef struct SWin32 { std::string adapterName; std::string displayName; } _win32;

    std::string adapterName;            // Name of the adapter (GPU).
    std::string displayName;            // Actual monitor name.

    _win32 win32;                       // Windows adapter and display names. Fuck you Windows and your stupid "DeviceName" vs "DeviceString".

    unsigned short widthMM, heightMM;   // Physical dimensions in millimeters.
};

/**
 * Video mode structure.
 */
struct tsVideoMode
{
    unsigned short width, height;   // Dimensions in pixels.

    unsigned char refreshRate;      // Refresh rate.

    unsigned char colorDepth;       // Number of bits per pixel.
};

std::vector<tsMonitor> tsGetMonitors()
{
    std::vector<tsMonitor> returnVal;

    for(unsigned char adapterIndex = 0;; adapterIndex++)
    {
        DISPLAY_DEVICE adapter;

        ZeroMemory(&adapter, sizeof(DISPLAY_DEVICE));
        adapter.cb = sizeof(DISPLAY_DEVICE);

        if(!EnumDisplayDevices(NULL, adapterIndex, &adapter, 0)) break;
        if(!(adapter.StateFlags & DISPLAY_DEVICE_ACTIVE)) continue;

        for(unsigned char displayIndex = 0;; displayIndex++)
        {
            DISPLAY_DEVICE display;

            ZeroMemory(&display, sizeof(DISPLAY_DEVICE));
            display.cb = sizeof(DISPLAY_DEVICE);

            if(!EnumDisplayDevices(adapter.DeviceName, displayIndex, &display, 0)) break;

            HDC hDC = CreateDC("DISPLAY", adapter.DeviceName, NULL, nullptr);

            tsMonitor monitor;

            monitor.adapterName = adapter.DeviceString;
            monitor.displayName = display.DeviceString;

            monitor.win32.adapterName = adapter.DeviceName;
            monitor.win32.displayName = display.DeviceName;

            monitor.widthMM = (unsigned short)GetDeviceCaps(hDC, HORZSIZE);
            monitor.heightMM = (unsigned short)GetDeviceCaps(hDC, VERTSIZE);

            returnVal.push_back(monitor);
        }
    }

    return returnVal;
}

unsigned char tsGetMonitorCount()
{
    return (unsigned char)GetSystemMetrics(SM_CMONITORS); // NOTE: I don't exactly understand pseudo-monitors.
}

bool tsGetVideoMode(const tsMonitor& monitor, tsVideoMode* mode)
{
    DEVMODE devMode;

    ZeroMemory(&devMode, sizeof(DEVMODE));
    devMode.dmSize = sizeof(DEVMODE);

    if(!EnumDisplaySettings(monitor.win32.adapterName.c_str(), ENUM_CURRENT_SETTINGS, &devMode)) return false;

    mode->width = (unsigned short)devMode.dmPelsWidth;
    mode->height = (unsigned short)devMode.dmPelsHeight;
    mode->refreshRate = (unsigned char)devMode.dmDisplayFrequency;
    mode->colorDepth = (unsigned char)devMode.dmBitsPerPel;

    return true;
}

I have some general comments as I'm not too familiar with the Windows API.

Comments that are just tautologies are not very useful
typedef struct is a pointless idiom in C++, as C++ doesn't have tag names
Why are you using unsigned short (2 bytes wide) when the DEVMODE structure uses DWORD (32-bits wide)? In general, you should use the Windows typedef to stay consistent and avoid unnecessary type issues
You have some redundant casts, like in tsGetMonitorCount(). Also C-style casts can sometimes silence warnings which is not what you want
Why does tsGetVideoMode return a bool? I would expect it to return a video mode
The "Fuck you Windows" comment is unprofessional and unlikely to impress any readers of your code. I (and future readers) will also have no idea what the purpose of the comment is, so it's useless altogether


Bouncing-ball simulation I posted a couple questions about a week ago, but have almost gotten it working. This program is supposed to simulate the following:

Ask the user how many balls to drop
Drop one ball at a time
Allow it to bounce ten times moving either once to the right or once to the left each bounce
Then print out the even numbers (since a ball could never land on an odd) -10 - 10 with a 'o' to represent each ball.

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

/* Prototype for drop_Balls, int parameter is number of balls being dropped */
void drop_balls(int);

/* Gets the number of balls that need to be dropped by the user */
int get_num_balls();

int main()
{
    drop_balls(get_num_balls());
}

int get_num_balls()
{
    int num_balls;
    printf("How many balls should be dropped? ");
    scanf("%d", &num_balls);
    /* Ensure that it is atleast one ball */
    while(num_balls <= 0)
    {
        printf("You have to drop at least one ball! \n ");
        printf("How many balls should be dropped? ");
        scanf("%d", &num_balls);
    }
    /* Return the number of balls that will be dropped */
    return num_balls;
}

void drop_balls(int num_balls)
{
    /* Keeps track of how many balls landed where */
    int ball_count[21];
    /* What number ball are we on */
    int ball_num = 0;
    /* Seed the generator */
    srand(time(NULL));
    /* Do the correct # of balls */
    for(ball_num = 0; ball_num < num_balls; ball_num++ )
    {
        /* Start at 10 since its the middle of 21 */
        int starting_point = 10;
        /* Bounce each ball 10 times */
        for(starting_point = 10; starting_point > 0; starting_point--)
        {
           int number;
           /* Create a random integer between 1-100 */
           number = rand() % 100;
            /* If its less than 50 bounce to the right once */
           if(number >= 50)
           {
               starting_point++;
           }
           /* If its greater than 50, bounce to the left once */
           else
           {
               starting_point--;
           }
        }
        /* Add one to simulate one ball landing there */
        ball_count[starting_point]++;
    }

    /* Start at the -10 spot */
    int ball_place = -10;
    int x;
    /* Go through the 21 spots, but skip the odd ones, since they never get balls */
    for(x = 0; x < 20; x+2)
    {
        printf("\n %d: ", ball_place);
        int l = 0;
        /* Print out an 'o' for each ball */
        for(l = 0; l < ball_count[x]; l++)
        {
            printf("o");
        }
        /* Increase the spot, taking it from -10 - 10 */
        ball_place = ball_place + 2;
    }
}

Comments

You should use // comments for single-line comments.

I am not sure you are using comments properly. Comments shouldn't explain how, it should explain why.

/* Prototype for drop_Balls, int parameter is number of balls being dropped */
void drop_balls(int);

One can see that this is a prototype. void drop_balls(int number_of_balls); would probably provide as much information in a more compact way.

/* Return the number of balls that will be dropped */
return num_balls;

This does not look like a useful comment to me.

/* Keeps track of how many balls landed where */
int ball_count[21];

A bit of explanation on the 21 here would be useful... but it is not provided.

This leads me to the next section:

Magic numbers

You probably don't want to have hard-coded numbers all over the place. You could create a constant with value 21 and then:

starting_point = 10 would become starting_point = N /2.
for(x = 0; x < 20; x+2) would become for(x = 0; x < N; x+2).
int ball_place = -10; would become int ball_place = - N/2;

Code organisation

It would probably be better now to call srand in rand_balls as it shouldn't be its responsibility. You can do it from main() if you want.

Define variable in the smallest possible scope. For instance:

for(int ball_num = 0; ball_num < num_balls; ball_num++ )

Also, how is this loop:

for(starting_point = 10; starting_point > 0; starting_point--)

supposed to behave?  Don't we always have the variable set to 0 or -1 at the end?


ES6 how to avoid nested functions I have functions nested here in setTimeout

function startWorker (obj) {
  if (!code) {
    return setTimeout(2000, () => {
      startWorker(obj)
    });
  }

  console.log('worker started')
}

How do I correctly use startWorker as the callback without using more functions than needed.

What I'm trying to do:

I'm trying to run the same function with the same argument as the callback to a setTimeout inside without duplicating function calls.

I'm not sure I understand what you're asking, but what if you just use setInterval() instead of setTimeout()?

function startWorker(obj) {
  if (!code) {
    setInterval( () => {
      console.log(obj.action);
    }, 2000);
  }
}

const code = false;
const someObj = {
  action: "tick"
};

startWorker(someObj);


Caesar cipher cracker How can I refactor this with less code? This is homework and is cracking a Caesar cipher-text using frequency distribution.

I have completed the assignment but would like it to be cleaner.

#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#include <ctype.h>
#define TEXT_SIZE 100000  // Note, the longer the text the more likely you will get a good decode from the start.
#define ALEN 26         // Number of chars in ENGLISH alphabet
#define CHFREQ "ETAONRISHDLFCMUGYPWBVKJXQZ" // Characters in order of appearance in English documents.
#define ALPHABET "ABCDEFGHIJKLMNOPQRSTUVWXYZ"

/* Decode the given text using the given map and store the result in newtext */
void decode_text(char* text, char* newtext, char map[][2]) {
int i, j;

for (i = 0; i < strlen(text); i++) {
    // If the current character in text is not a letter, copy it to newtext as punctuation and digits should be maintained.
    if (!isalpha(text[i])) {
        newtext[i] = text[i];
        continue;
    }
    for (j = 0; j < ALEN; j++) {
        if (text[i] == map[j][1]) {
            newtext[i] = map [j][0];
        }
    }
}
}

char upcase(char ch){
    if(islower(ch))
    ch -= 'a' - 'A';
  return ch;
}

int main(int argc, char **argv){

// first allocate some space for our input text (we will read from stdin).

char* text = (char*)malloc(sizeof(char)*TEXT_SIZE+1);
char textfreq[ALEN][2];
char map[ALEN][2];
char newtext[TEXT_SIZE];
char ch, opt, tmpc, tmpc2;
int i, j, tmpi;

// Check the CLI arguments and extract the mode: interactive or dump and store in opt.

if(!(argc == 2 && isalpha(opt = argv[1][1]) && (opt == 'i' || opt == 'd'))){
    printf("format is: '%s' [-d|-i]\n", argv[0]);
    exit(1);
}

// Now read TEXT_SIZE or feof worth of characters (whichever is smaller) and convert to uppercase as we do it.

for(i = 0, ch = fgetc(stdin); i < TEXT_SIZE && !feof(stdin); i++, ch = fgetc(stdin)){
    text[i] = (isalpha(ch)?upcase(ch):ch);
}
text[i] = '\0'; // terminate the string properly.

// Assign alphabet to one dimension of text frequency array and a counter to the other dimension
for (i = 0; i < ALEN; i++) {
    textfreq[i][0] = ALPHABET[i];
    textfreq[i][1] = 0;
}

// Count frequency of characters in the given text
for (i = 0; i < strlen(text); i++) {
    for (j = 0; j < ALEN; j++) {
        if (text[i] == textfreq[j][0]) textfreq[j][1]+=1;
    }
}

//Sort the character frequency array in descending order
for (i = 0; i < ALEN-1; i++) {
    for (j= 0; j < ALEN-i-1; j++) {
        if (textfreq[j][1] < textfreq[j+1][1]) {
            tmpi = textfreq[j][1];
            tmpc = textfreq[j][0];
            textfreq[j][1] = textfreq[j+1][1];
            textfreq[j][0] = textfreq[j+1][0];
            textfreq[j+1][1] = tmpi;
            textfreq[j+1][0] = tmpc;
        }
    }
}

//Map characters to most occurring English characters
for (i = 0; i < ALEN; i++) {
    map[i][0] = CHFREQ[i];
    map[i][1] = textfreq[i][0];
}

// Sort the map lexicographically
for (i = 0; i < ALEN-1; i++) {
    for (j= 0; j < ALEN-i-1; j++) {
        if (map[j][0] > map[j+1][0]) {
            tmpc = map[j][0];
            tmpc2 = map[j][1];
            map[j][0] = map[j+1][0];
            map[j][1] = map[j+1][1];
            map[j+1][0] = tmpc;
            map[j+1][1] = tmpc2;
        }
    }
}

if(opt == 'd'){
    decode_text(text, newtext, map);
} else {
// do option -i
}

// Print alphabet and map to stderr and the decoded text to stdout
fprintf(stderr, "\n%s\n", ALPHABET);
for (i = 0; i < ALEN; i++) {
    fprintf(stderr, "%c", map[i][1]);
}
printf("\n%s\n", newtext);

return 0;
}

A few simple comments :

Listen to your compiler

The compiler can give you a lot of really interesting information. Activate all warnings (I use gcc -Wall -Wextra -std=c99 cipher.c -o cipher_out) and you'll see a few things to fix :

cipher.c: In function ‘decode_text’:
cipher.c:13:16: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
  for (i = 0; i < strlen(text); i++) {
                ^
cipher.c: In function ‘main’:
cipher.c:37:2: warning: implicit declaration of function ‘malloc’ [-Wimplicit-function-declaration]
  char* text = (char*)malloc(sizeof(char)*TEXT_SIZE+1);
  ^
cipher.c:37:22: warning: incompatible implicit declaration of built-in function ‘malloc’ [enabled by default]
  char* text = (char*)malloc(sizeof(char)*TEXT_SIZE+1);
                      ^
cipher.c:47:3: warning: implicit declaration of function ‘printf’ [-Wimplicit-function-declaration]
   printf("format is: '%s' [-d|-i]\n", argv[0]);
   ^
cipher.c:47:3: warning: incompatible implicit declaration of built-in function ‘printf’ [enabled by default]
cipher.c:48:3: warning: implicit declaration of function ‘exit’ [-Wimplicit-function-declaration]
   exit(1);
   ^
cipher.c:48:3: warning: incompatible implicit declaration of built-in function ‘exit’ [enabled by default]
cipher.c:53:2: warning: implicit declaration of function ‘fgetc’ [-Wimplicit-function-declaration]
  for(i = 0, ch = fgetc(stdin); i < TEXT_SIZE && !feof(stdin); i++, ch = fgetc(stdin)){
  ^
cipher.c:53:24: error: ‘stdin’ undeclared (first use in this function)
  for(i = 0, ch = fgetc(stdin); i < TEXT_SIZE && !feof(stdin); i++, ch = fgetc(stdin)){
                        ^
cipher.c:53:24: note: each undeclared identifier is reported only once for each function it appears in
cipher.c:53:2: warning: implicit declaration of function ‘feof’ [-Wimplicit-function-declaration]
  for(i = 0, ch = fgetc(stdin); i < TEXT_SIZE && !feof(stdin); i++, ch = fgetc(stdin)){
  ^
cipher.c:65:16: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
  for (i = 0; i < strlen(text); i++) {
                ^
cipher.c:112:2: warning: implicit declaration of function ‘fprintf’ [-Wimplicit-function-declaration]
  fprintf(stderr, "\n%s\n", ALPHABET);
  ^
cipher.c:112:2: warning: incompatible implicit declaration of built-in function ‘fprintf’ [enabled by default]
cipher.c:112:10: error: ‘stderr’ undeclared (first use in this function)
  fprintf(stderr, "\n%s\n", ALPHABET);
          ^
cipher.c:116:2: warning: incompatible implicit declaration of built-in function ‘printf’ [enabled by default]
  printf("\n%s\n", newtext);
  ^

You are missing :

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

And you should define i and j as unsigned int.

Format your code

The indentation seems a bit off in your question. I don't know if it's because you pasted the code in a wrong way or whatever but your favorite text editor can fix this easily.

Tell what you want to do

You are not describing anywhere how your program works : format is: program_name [-d|-i] is not enough if you don't explain what the options are for.

Also, it should be described somewhere that your program will affect only letters for instance.

Move variables to the smallest possible scope

It keeps things easier to understand and to split into smaller functions.

At this stage, the code looks like :

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <ctype.h>
#define TEXT_SIZE 100000  // Note, the longer the text the more likely you will get a good decode from the start.
#define ALEN 26         // Number of chars in ENGLISH alphabet
#define CHFREQ "ETAONRISHDLFCMUGYPWBVKJXQZ" // Characters in order of appearance in English documents.
#define ALPHABET "ABCDEFGHIJKLMNOPQRSTUVWXYZ"

/* Decode the given text using the given map and store the result in newtext */
void decode_text(char* text, char* newtext, char map[][2]) {
    for (unsigned int i = 0; i < strlen(text); i++) {
        // If the current character in text is not a letter, copy it to newtext as punctuation and digits should be maintained.
        if (!isalpha(text[i])) {
            newtext[i] = text[i];
            continue;
        }
        for (unsigned int j = 0; j < ALEN; j++) {
            if (text[i] == map[j][1]) {
                newtext[i] = map [j][0];
            }
        }
    }
}

char upcase(char ch){
    if(islower(ch))
        ch -= 'a' - 'A';
    return ch;
}

int main(int argc, char **argv){
    // first allocate some space for our input text (we will read from stdin).
    char* text = (char*)malloc(sizeof(char)*TEXT_SIZE+1);
    char opt;
    int tmpi;

    // Check the CLI arguments and extract the mode: interactive or dump and store in opt.

    if(!(argc == 2 && isalpha(opt = argv[1][1]) && (opt == 'i' || opt == 'd'))){
        printf("format is: '%s' [-d|-i]\n", argv[0]);
        exit(1);
    }

    // Now read TEXT_SIZE or feof worth of characters (whichever is smaller) and convert to uppercase as we do it.
    {
        unsigned int i;
        for(char ch = fgetc(stdin); i < TEXT_SIZE && !feof(stdin); i++, ch = fgetc(stdin)){
            text[i] = (isalpha(ch)?upcase(ch):ch);
        }
        text[i] = '\0'; // terminate the string properly.
    }

    // Assign alphabet to one dimension of text frequency array and a counter to the other dimension
    char textfreq[ALEN][2];
    for (unsigned int i = 0; i < ALEN; i++) {
        textfreq[i][0] = ALPHABET[i];
        textfreq[i][1] = 0;
    }

    // Count frequency of characters in the given text
    for (unsigned int i = 0; i < strlen(text); i++) {
        for (unsigned int j = 0; j < ALEN; j++) {
            if (text[i] == textfreq[j][0]) textfreq[j][1]+=1;
        }
    }

    //Sort the character frequency array in descending order
    for (unsigned i = 0; i < ALEN-1; i++) {
        for (unsigned j= 0; j < ALEN-i-1; j++) {
            if (textfreq[j][1] < textfreq[j+1][1]) {
                tmpi = textfreq[j][1];
                char tmpc = textfreq[j][0];
                textfreq[j][1] = textfreq[j+1][1];
                textfreq[j][0] = textfreq[j+1][0];
                textfreq[j+1][1] = tmpi;
                textfreq[j+1][0] = tmpc;
            }
        }
    }

    //Map characters to most occurring English characters
    char map[ALEN][2];
    for (unsigned int i = 0; i < ALEN; i++) {
        map[i][0] = CHFREQ[i];
        map[i][1] = textfreq[i][0];
    }

    // Sort the map lexicographically
    for (unsigned int i = 0; i < ALEN-1; i++) {
        for (unsigned int j = 0; j < ALEN-i-1; j++) {
            if (map[j][0] > map[j+1][0]) {
                char tmpc = map[j][0];
                char tmpc2 = map[j][1];
                map[j][0] = map[j+1][0];
                map[j][1] = map[j+1][1];
                map[j+1][0] = tmpc;
                map[j+1][1] = tmpc2;
            }
        }
    }

    char newtext[TEXT_SIZE];
    if(opt == 'd'){
        decode_text(text, newtext, map);
    } else {
        // do option -i
    }

    // Print alphabet and map to stderr and the decoded text to stdout
    fprintf(stderr, "\n%s\n", ALPHABET);
    for (unsigned int i = 0; i < ALEN; i++) {
        fprintf(stderr, "%c", map[i][1]);
    }
    printf("\n%s\n", newtext);

    return 0;
}

Split your code into re-usable functions

I just realised that vnp comment explains this already. nothing to add here.

A few tiny code comments

You don't need to use malloc in your case.

Also, you don't need a continue in your decode function : an else would do the trick here.


Find a value between two of the same characters in a string using Python I am attempting to make a python script that imports a given file.

Then on the import of the file, I am pulling a value from in the filename to determine what function to run.

I will then eventually extend this to pull all files from a folder rather than explicitly pass it a specific file.

The format of the file names is always the following:

blah_Value_blah.extension

I am wondering if there is a better and more efficient way to pull Value from the above example than what I have given below?

Here is my code:

from os.path import splitext, basename

under = '_'

base = basename(splitext(filename_goes_here)[0])
value = base[base.find(under)+len(under):base.rfind(under)]

I am aware I can merge my two lines of code above into one line but it would be very unsightly.

Examples of the filenames are:

//path/to/file/GAME_team_2017.csv
//path/to/file/GAME_player_2017.csv
//path/to/file/GAME_rules_2017.csv

The sample output of the above files would be:

'team'
'player'
'rules'

Rather than using str.find, you could better describe yourself using regex. However it's not that much of an improvement.

For example using the regex _(.+)_ on the basename of the file is all you need. If you think a file extension is going to have an _ then you may need the splitext.

This can get:

from os.path import splitext, basename
from re import search

base = basename(splitext(filename_goes_here)[0])
value = search('_(.+)_', base)
if value is not None:
    value = value.group(1)

If you're using Python 3.6, as noted in the comments by 200_success, you could change the last line to:

value = value[0]