Retrieve high views per day questions with few answers I've been messing around with the SE Data Explorer and I've come up with a query that basically returns information about questions that have a high number of views per day, that have few or no answers.

SELECT
      TOP 500
      Question.ViewCount,
      DATEDIFF(dd, CONVERT(date,Question.CreationDate), CONVERT(date, GETDATE()) ) AS Days ,
      Question.Id AS [Post Link],
      (Question.ViewCount / DATEDIFF(dd, CONVERT(date,Question.CreationDate), CONVERT(date, GETDATE()) )) AS ViewsPerDay,
      Question.AnswerCount,
      (SELECT MAX (Answers.Score )
       FROM POSTS "Answers"
       WHERE Answers.ParentId = Question.Id
       AND   Answers.PostTypeId = 2
      ) "Highest Answer Score",
      Question.Tags
 FROM
      POSTS "Question"
WHERE Question.PostTypeId = 1
AND   Question.acceptedanswerid is null
AND   DATEDIFF(dd, CONVERT(date,Question.CreationDate), CONVERT(date, GETDATE()) ) > 10
AND   (Question.ViewCount / DATEDIFF(dd, CONVERT(date,Question.CreationDate), CONVERT(date, GETDATE()) )) > 5
ORDER BY
    AnswerCount ASC,
    "Highest Answer Score" ASC,
    ViewsPerDay DESC

It's been a while since I used SQL directly in anger, so any feedback's welcome.

I particularly don't like the way DATEDIFF(dd, CONVERT(date,Question.CreationDate), CONVERT(date, GETDATE()) ) is repeated in both the SELECT and the WHERE clauses, so I'd like some way of aliasing the check if possible.

Overall
I think your query is pretty nicely written and returns useful information.

Use some variables to refer to repeated values, for example, the GETDATE() reference could just be declared as a variable at the top and reused throughout.
DECLARE @now DATETIME2 = GETDATE();

You could also use variables to eliminate some magic numbers, such as the Posts.PostTypeId for questions and answers.

You might want to add a field to indicate when the question was actually posted. A day count is all fair and well, but knowing the actual post date might be good information as well.
SELECT
  /* ...*/
  Question.CreationDate AS DatePosted

You might want to rename the Days column to something a bit more descriptive, like DaysSinceFirstPosted or just DaysOld.

Use day instead of dd, they work the same but day makes the code easier to read.
You also don't need to convert datetime fields to date fields for calculations to work correctly, it's all factored in for you already, e.g.:
DATEDIFF(DAY, Question.CreationDate, @now) AS DaysSinceFirstPosted

ViewsPerDay would be better named as AverageViewsPerDay based on your calculations.

SELECT
    /* ... */
      (SELECT MAX (Answers.Score)
       FROM POSTS "Answers"
       WHERE Answers.ParentId = Question.Id
       AND   Answers.PostTypeId = 2
      ) "Highest Answer Score",

This would make more sense as a normal table join rather than an inline subquery:
SELECT
    /* ... */
    MAX(Answers.Score) AS [HighestAnswerScore],
    /* ... */
FROM
    Posts AS [Question]
    LEFT JOIN Posts AS [Answers]
        ON Answers.ParentId = Question.Id
        AND Answers.PostTypeId = @answer
WHERE
    /* ... */

If you plan on using quoted identifiers as you did, you would want to declare SET QUOTED_IDENTIFIER ON; at the top of your query. SE Data Explorer must have that set by default, yet it's not the case for some other databases. I personally find it more practical to use default square brackets for identifiers.

With all of the above applied, here is the new query:
DECLARE @now DATETIME2 = GETDATE();
DECLARE @question INT = 1;
DECLARE @answer INT = 2;
SELECT TOP 500
    Question.CreationDate AS QuestionPosted,
    Question.ViewCount AS QuestionViews,
    DATEDIFF(DAY, Question.CreationDate, @now) AS DaysSinceFirstPosted,
    Question.Id AS [Post Link],
    Question.ViewCount / DATEDIFF(DAY, Question.CreationDate, @now) AS AverageViewsPerDay,
    Question.AnswerCount,
    MAX(Answers.Score) AS HighestAnswerScore,
    Question.Tags
FROM
    Posts AS [Question]
    LEFT JOIN Posts AS [Answers]
        ON Answers.ParentId = Question.Id
        AND Answers.PostTypeId = @answer
WHERE
    Question.PostTypeId = @question
    AND Question.AcceptedAnswerId is null
    AND DATEDIFF(DAY, Question.CreationDate, @now) > 10
    AND (Question.ViewCount / DATEDIFF(DAY, Question.CreationDate, @now)) > 5
GROUP BY
    Question.CreationDate,
    Question.ViewCount,
    DATEDIFF(DAY, Question.CreationDate, @now),
    Question.Id,
    Question.ViewCount / DATEDIFF(DAY, Question.CreationDate, @now),
    Question.AnswerCount,
    Answers.Score,
    Question.Tags
ORDER BY
    AnswerCount ASC,
    HighestAnswerScore ASC,
    AverageViewsPerDay DESC;


Self-taught Pythonista: Any criticism welcome for this concurrent word count script! I've been teaching myself Python - my first programming language - for about two years now.

I recently discovered the concurrent.futures module and wanted to do something with it. What do you think about this script?

import re
import shutil
import string

from collections import Counter
from concurrent.futures import ProcessPoolExecutor
from itertools import chain, islice, zip_longest
from urllib.request import urlopen

# Regex to use for splitting text into words, dropping everything but
# alphabetic characters.
REGEX = re.compile(r"[{}{}{}]+".format(
        string.whitespace, string.digits, string.punctuation))

# http://docs.python.org/3/library/itertools.html#itertools-recipes
def grouper(iterable, n, fillvalue=None):
    "Collect data into fixed-length chunks or blocks"
    # grouper('ABCDEFG', 3, 'x') --> ABC DEF Gxx"
    args = [iter(iterable)] * n
    return zip_longest(*args, fillvalue=fillvalue)

def split_and_clean(line):
    """Returns an iterator of the words in line.

    Example:
        >>> list(split_and_clean("3, Four, five'"))
        ['four', 'five']

    Args:
        line: A string.

    Returns:
        A filter of the alphabetic words in the line.

    Raises:
        TypeError: Your input was of type <>. Must be a string.
    """
    try:
        return filter(None, re.split(REGEX, line.lower()))
    except AttributeError:
        input_type_str = str(type(line))[8:-2]
        error_message = "Your input was of type {}. Must be a string.".format(
                input_type_str)
        raise TypeError(error_message)

def wc_some_lines(lines):
    """Return a Counter containing the word count of several lines.
    Excludes any digital numbers or punctuation.

    Example:
        >>> wc_some_lines(["Line 1.", "Another line."])
        Counter({'line': 2, 'another': 1})

    Args:
        lines: An iterable of strings.

    Returns:
        A collections.Counter mapping words to their word counts.

    Raises:
        TypeError
    """
    return Counter(chain(*map(split_and_clean, lines)))

def main(num_processes=5):
    """Fetch Shakespeare's complete works and perform a concurrent word count.
    """
    url = "http://www.gutenberg.org/files/100/100.txt"
    filename = "shakespeare_complete_works.txt"
    try:
        text = open(filename)
    except IOError:
        with open(filename, "wb") as f:
            r = urlopen(url)
            shutil.copyfileobj(r, f)    # write r to f
        text = open(filename)

    # exclude the small print at the beginning and the end
    relevant_part = islice(text, 181, 124379)
    # group text into blocks of 5000 lines
    blocks = grouper(relevant_part, 5000, "")

    # perform actual word count in concurrent processes
    result = Counter()
    with ProcessPoolExecutor(num_processes) as pool:
        for part in pool.map(wc_some_lines, blocks):
            result.update(part)

    # output results
    for word, count in result.most_common(20):
        print("{}: {}".format(word, count))

if __name__ == "__main__":
    main()

PEP8 mentions that top-level constructs like functions should be separated by two lines. Hanging indents should have only one level of indentation (lines 43 and 61). Be careful about trailing whitespaces (lines 61 and 89).

I love functional style myself but it is often frowned upon in Python, and Counter(chain(*map(split_and_clean, lines))) or filter(None, re.split(REGEX, line.lower())) will be considered unreadable by some, and elegant by others.

Otherwise your code is awesome, well-written with beautiful docstrings and clever (filter() to drop empty strings, AttributeError and to the call to lower()). Thanks for sharing!


"Cookie Clicker Alpha" solution I am trying to learn Clojure for some time. In my experience, it has been rather too easy to produce write-only code.

Here is a solution to a simple problem with very little essential complexity. Input and output formats are extremely simple, too. Which means all complexity in it must be accidental. How to improve its legibility, intelligibility?

Is the decomposition of the problem into functions all right?

Also other specific problems:

How to input/output numbers? Is there any benefit to use read-string instead of Double/parseDouble? How to format floating point numbers to fixed precision without messing with the default locale?
How to avoid the explicit loop/recur, which is currently a translation of a while loop?
Are there definitions that should/shouldn't have bee private/dynamic?

  Problem

  You start with 0 cookies. You gain cookies at a rate
  of 2 cookies per second [...]. Any time you
  have at least C cookies, you can buy a cookie farm. Every time you buy
  a cookie farm, it costs you C cookies and gives you an extra F cookies
  per second.

  Once you have X cookies that you haven't spent on farms, you win!
  Figure out how long it will take you to win if you use the best
  possible strategy.

(ns cookie-clicker
  (:use [clojure.string :only [split]])
  (:require [clojure.java.io :as io]
            [clojure.test :refer :all]))

;;See http://code.google.com/codejam/contest/2974486/dashboard#s=p1

(defn parse-double [s] (java.lang.Double/parseDouble s))

(defn parse-row [line]
  (map parse-double (split line #"\s+")))

(defn parse-test-cases [rdr]
  (->> rdr
    line-seq
    rest
    (map parse-row)))

(def initial-rate 2.0)

(defn min-time [c f x]
  (loop [n 0 ; no of factories used
         tc 0 ; time cost of factories built
         r initial-rate ; cookie production rate
         t (/ x r)] ; total time
    (let [n2 (inc n)
          tc2 (+ tc (/ c r))
          r2 (+ r f)
          t2 (+ tc2 (/ x r2))]
      (if (> t2 t)
        t
        (recur n2 tc2 r2 t2)))))

(java.util.Locale/setDefault (java.util.Locale/US))

(defn ans [n t]
  (str "Case #" n ": " (format "%.7f" t)))

(defn answers [test-cases]
  (map #(ans %1 (apply min-time %2))
       (rest (range))
       test-cases))

(defn spit-answers [in-file]
  (with-open [rdr (io/reader in-file)]
    (doseq [answer (answers (parse-test-cases rdr))]
      (println answer))))

(defn solve [in-file out-file]
  (with-open [w (io/writer out-file :append false)]
    (binding [*out* w]
      (spit-answers in-file))))

(def ^:dynamic *tolerance* 1e-6)

(defn- within-tolerance [expected actual]
  (< (java.lang.Math/abs (- expected actual))
     *tolerance*))

(deftest case-3
    (is (within-tolerance
          63.9680013
          (min-time 30.50000 3.14159 1999.19990))))

(defn -main []
  (solve "resources/cookie_clicker/B-large-practice.in"
         "resources/cookie_clicker/B-large-practice.out"))

I have to admit, the actual "solving the problem" component of this is a little over my head. But, I thought I'd try to answer your questions and give you some style/structure feedback, for what it's worth :)

You can simplify your ns declaration like this:

(ns cookie-clicker
  (:require [clojure.string :refer (split)]
            [clojure.java.io :as io]
            [clojure.test :refer :all]))

(:require foo :refer (bar) does the same thing as :use foo :only (bar), and is generally considered preferable, especially as an alternative to having both :use and :require in your ns declaration)

I think Double/parseDouble is a good approach to parsing doubles in string form. Integer/parseInt is usually my go-to for doing the same with integers in string form. This is just a hypothesis, but Double/parseDouble might be faster and/or more accurate than read-string because it's optimized for doubles.

FYI, you can leave out the java.lang. and just call it as Double/parseDouble in your code. In light of that, you might consider getting rid of your parse-double function altogether and just using Double/parseDouble whenever you need it. The only thing is that Java methods aren't first-class in Clojure, so you would need to do things like this if you go that route:

(defn parse-row [line]
  (map #(Double/parseDouble %) (split line #"\s+")))

(Personally, I still like that better, but you might prefer to keep it wrapped in a function parse-double like you have it. It's up to you!)

I think needing to mess with the locale might be a locale-specific problem... I tried playing around with (format "%.7f" ... without changing my locale and it worked as expected. Granted, I'm in the US :)

I think the legibility issues you're seeing might be related to having too many functions. You might consider condensing and renaming things and see if you like that better. I would re-structure your program so that you parse the data into the data structure at the top, something like this:

(defn parse-test-cases [in-file]
  (with-open [rdr (io/reader in-file)]
    (let [rows (rest (line-seq rdr))]
      (map (fn [row]
             (map #(Double/parseDouble %) (split row #"\s+")))
           rows))))

(I condensed your functions parse-row, parse-test-cases and half of spit-answers into the function above)

Then define the functions that "do all the work" like min-time, and then, at the end:

(defn spit-answers [answers out-file]
  (with-open [w (io/writer out-file :append false)]
    (.write w (clojure.string/join "\n" answers)))

(def -main []
  (let [in  "resources/cookie_clicker/B-large-practice.in"
        out "resources/cookie_clicker/B-large-practice.out"
        test-cases (parse-test-cases in)
        answers (map-indexed (fn [i [c f x]]
                               (format "Case #%d: %.7f" (inc i) (min-time c f x)))
                             test-cases)]
    (spit-answers answers out)))

I came up with a few ideas above:

In your answers function you use (map ... (rest (range)) (test-cases)) in order to number each case, starting from 1. A simpler way to do this is with map-indexed. I used (inc i) for the case numbers, since the index numbering starts at 0.
I condensed (str "Case #" n ": " (format "%.7f" t))) into a single call to format.
I used destructuring over the arguments to the map-indexed function to represent each case as c f x -- that way it's clearer that each test case consists of those three values, and you can represent the calculation as (min-time c f x) instead of (apply min-time test-case).

As for your min-time function, I don't think loop/recur is necessarily a bad thing, and I often tend to rely on it in complicated situations where you're doing more involved work on each iteration, checking conditions, etc. I think it's OK to use it here. But if you want to go a more functional route, you could consider writing a step function and creating a lazy sequence of game states using iterate, like so:

(note: I'm writing step as a letfn binding so that it can use arbitrary values of c, f and x that you feed into a higher-order function that I'm calling step-seq -- this HOF takes values for c, f and x and generates a lazy sequence of game states or "steps.")

(defn step-seq [c f x]
  (letfn [(step [{:keys [factories time-cost cookie-rate total-time result]}]
            (let [new-time-cost (+ time-cost (/ c cookie-rate))
                  new-cookie-rate (+ cookie-rate f)
                  new-total-time (+ new-time-cost (/ x new-cookie-rate))]
              {:factories (inc factories)
               :time-cost new-time-cost
               :cookie-rate new-cookie-rate
               :total-time new-total-time
               :result (when (> new-total-time total-time) total-time)}))]
    (iterate step {:factories 0, :time-cost 0, :cookie-rate 2.0,
                   :total-time (/ x 2.0), :result nil})))

Now, finding the solution is as simple as grabbing the :result value from the first step that has one:

(defn min-step [c f x]
  (some :result (step-seq c f x)))


Somewhat esoteric if statement in a paginated feed I am working on the home page of a website that will have a paginated feed (much like a blog's home page).

One of the requirements is that when a user navigates to a non-existent page, he or she will be redirected to the last available page. For example, when the user navigates to a non-existent page such as page number 500, they will be redirected to the highest (last available) page number which might be something sensible like 10.

The code to fulfill this requirement easy enough looks like this:

private const int PageSize = 5;
public ActionResult Index(int pageNumber = 1)
{
    IEnumerable<Post> posts = repository.All();
    PagedList<Post> page = posts.Paginate(pageNumber, PageSize);

    if (page.Count == 0 && pageNumber != 1)
    {
        return RedirectToAction("Index", new { pageNumber = page.TotalPageCount });
    }

    return View(page);
}

My problem with this code is the following expression:

page.Count == 0 && pageNumber != 1

Personally I find this code hard to reason about (and I am the person who wrote it!).  How can I make the meaning of this expression clearer? Is there an alternative, more readable way  of implementing this logic perhaps?

The condition begins by evaluating whether the requested page has any posts - if the contents of the page are empty, the page effectively does not exist. The condition then checks whether the user is attempting to access the home page (the first page) or not. This check is important because if the database is empty, the user still needs to be able to access the first page - so that the view can display a nice error message.

You're checking that "page does not contain any posts and page number is not one".

Reversing the condition is often the easiest way to clarify it. The inverse condition would be "page contains at least one post, or page number is one".

I would make it look like this:

if (pagePosts.Any() || pageNumber == 1)
{
    return View(pagePosts);
}
else
{
    return RedirectToAction("Index", new { pageNumber = pagePosts.TotalPageCount });
}

Notice page renamed to pagePosts, also for clarity: it's not a page, it's the posts on the page that was requested. pagePosts is less ambiguous I find.


Read integers from coupled lines in a text file I'm trying to read a text file formatted as in this example:

5 3
3 1 5
6 6
6 1 3 2 5 4

Where the lines are coupled, with the line above always being of length equal to 2 and the line below having length equal to the the last digit in the line above.

(Unfortunately this is from an old note I took and I don't remember what those numbers are actually supposed to mean, so I'm limiting myself to store them in a multidimensional vector form where they can be easily recovered).

In the beginning I was tring to read the digit which represents the length below to execute a loop of finite length, but in the end this is the code I wrote:

#include <fstream>
#include <sstream>
#include <string>
#include <vector>
int main()
{
    std::string filname="data/file01.txt";
    std::ifstream file01(filname);
    std::vector<std::vector<int>> v;
    std::string line;
    while(getline(file01,line))
    {
        std::stringstream ls;
        ls<<line;
        int c;
        std::vector<int> t;
        while(!ls.eof())
        {
            ls>>c;
            t.push_back(c);
        }
        v.push_back(t);
    }
    return 0;
}

This gives me a 2-dimensional vector that can be easily accessed, but I don't like having to convert both the file and each string to a stream and I don't like using nested vectors.

Is there a way to achieve this goal with more simple and elegant code (even if it means to make the code more specific)?

You're turning the whole input file into a vector of vectors. That's fine, but you state yourself that your input has structure. It would be nice to have that structure reflected in your program:

since the lines are couple in pairs I'd like to see a code block that clearly processes a pair of lines
you should then eliminate the redundancy that the second number of the 1 st line equals the number of items on the second.


Find all pairs in an array whose absolute difference equals x I have an array A of integers with n values and an integer x. I need to find all the pairs in that array whose absolute difference is equal to x.

For example, take A = {13, 1,-8, 21, 0, 9,-54, 17, 31, 81,-46} and x = 8, my algorithm should print this:

0 & 3 with values 13 & 21
1 & 5 with values 1 & 9
2 & 4 with values -8 & 0
5 & 7 with values 9 & 17
6 & 10 with values -54 & -46

I worked out a solution, however, it is \$O(n^2)\$:

for (int i = 0 ; i < A.length; i ++)
    {
        for (int j = i; j < A.length; j++)
        {
          if (abs(A[i]-A[j]) == x)
             System.out.println("Indices " + i + " & " + j + " with values " + A[i] + " & "+ A[j]);
        }
    }

Without sorting first, is there any way to optimize this code to be faster? I know \$O(n^2)\$ isn't great but I can't think of a better way of doing it (without sorting first).
For my algorithm, the best case Omega is also \$Ω(n^2)\$, correct? I need to compare every value in any case.

I don't have the rep to comment, and I know you said without sorting first, but if you are already at worst case performance of O(n^2), and sorting in theory is O(nlogn), why not then sort if it wont hurt your big-o performance?

i.e.

A.sortAscending() // however done in java. in theory is O(nlogn)

int j = 0;
for (int i = j; i < A.len; i++) {
    while (j < A.len && (abs(A[i] - A[j++]) < x))
        ; // do nothing

    // so now we are guarenteed absdiff(arr[i], arr[j]) <= x.
    // as soon as it != x we can move on because as the array is sorted
    // the difference will only increase, thus not equaling x again
    while (j < A.len && (abs(A[i] - A[j++]) == x))
        resultArray.add((arr[i], arr[j])
}


Efficiency and correctness of my randomized selection algorithm Instead of picking a random pivot I'm shuffling the array beforehand. Does it count as randomization and is it efficient to shuffle or pick a random pivot and is my implementation pythonic?

Problem

Algorithm

from random import shuffle

def _partition(arr, lo, hi, pivot):
    p = arr[pivot]
    arr[hi - 1], arr[pivot] = arr[pivot], arr[hi - 1]
    i = lo - 1
    for j in xrange(lo, hi):
        if arr[j] <= p:
            i += 1
            arr[i], arr[j] = arr[j], arr[i]
    return i

def select(arr, lo, hi, spos):
    assert lo <= spos < hi
    shuffle(arr)  # here's your randomization.
    while True:
        pos = _partition(arr, lo, hi, lo)
        if pos == spos:
            return arr[pos]
        elif pos < spos:
            lo = pos + 1
        else:
            hi = pos

You should learn about how to benchmark your code, from my benchmark, shuffling makes the algorithm slower:

from random import shuffle,randint
import time

def _partition(arr, lo, hi, pivot):
    p = arr[pivot]
    arr[hi - 1], arr[pivot] = arr[pivot], arr[hi - 1]
    i = lo - 1
    for j in xrange(lo, hi):
        if arr[j] <= p:
            i += 1
            arr[i], arr[j] = arr[j], arr[i]
    return i

def select(arr, lo, hi, spos,shuffling=None):
    assert lo <= spos < hi
    if shuffling: shuffle(arr)  # here's your randomization.
    while True:
        pos = _partition(arr, lo, hi, lo)
        if pos == spos:
            return arr[pos]
        elif pos < spos:
            lo = pos + 1
        else:
            hi = pos

def random_list(length):
    return [randint(1,100) for _ in range(length)]

start = time.time()
results = []
for _ in range(10000):
    results.append(select(random_list(10),1,5,1,shuffling=True))
print("With shuffling: ",time.time()-start)

start = time.time()
results = []
for _ in range(10000):
    results.append(select(random_list(10),1,5,1,shuffling=False))
print("WithOUT shuffling: ",time.time()-start)


Traffic light sequence I have create a functioning automated traffic light sequence using an array and if statements. It all work correctly but I am wondering if there is anything more I can do to improve my code without changing to structure or way it works, so without the use of dictionaries, etc.

<!DOCTYPE html>

 <title> Traffic Light</title>

Traffic Light

 <canvas id="myCanvas" width="200" height="300"
 style="border:1px solid #000000;">
 Your browser does not support the HTML5 canvas tag.
 </canvas>

 <button onclick="changelight()">Click</button>

Ah, you moved your <h1> into the <body>. Still, what exactly are you looking for? File size (--> code reduction)? Speed optimization?

One simple thing would be using parameters to reduce code duplication:

var c = document.getElementById("myCanvas");
var ctx = c.getContext("2d");

ctx.rect(0, 0, 200, 300);
ctx.fillStyle = "grey";
ctx.fill();

var colours = ["red", "yellow", "green",  "red yellow"];
var current = colours[0];

function offlight(a1) {
    ctx.beginPath();
    ctx.arc(95, a1, 40, 10, 12 * Math.PI);
    ctx.fillStyle = "black";
    ctx.fill();
    ctx.stroke();
}

function drawLight(a1, fillParam) {
    ctx.beginPath();
    ctx.arc(95, a1, 40, 10, 12 * Math.PI);
    ctx.fillStyle = fillParam;
    ctx.fill();
    ctx.stroke();
}

function changelight() {
    if (current == colours[0]) {
        drawLight(50, "red");
        offlight(150);
        offlight(250);
        current = colours[4]
    } else if (current == colours[4]) {
        drawLight(50, "red");
        drawLight(150, "yellow");
        offlight(250);
        current = colours[2]
    } else if (current == colours[2]) {
        offlight(50);
        offlight(150);
        drawLight(250, "green");
        current = colours[1]
    } else if (current == colours[1]) {
        offlight(50);
        drawLight(150, "yellow");
        offlight(250);
        current = colours[0]
    }

}
setInterval(changelight, 1000);

Do you ever use colours? ("yellow")

You could also remove the whitespaces and shorten variable and function names. Also think about moving your JS into a separate file.

Here is a working fiddle.


Shell script that creates and deletes symbolic links Right now I am working on an install and uninstall script for my vim configuration files. The install script links files and directories and the uninstall script deletes those links. I just want to make sure that this code is somewhat safe, are there better ways of doing the same thing? Here is the code I have right now:

Install Script:

#!/bin/bash
# The install script for my vim dotfiles.
# Find the location of the script.
SCRIPT_DIRECTORY="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"

# The source directory and target directories.
SOURCE_LOCATION="$SCRIPT_DIRECTORY/src" # Contains the files and directories I want to work with.
TARGET_LOCATION="$SCRIPT_DIRECTORY/test-home" # The location I want to link to, in production this would be "~/"

# Link the files from source to the target with a dot appended to the front.
find $SOURCE_LOCATION -mindepth 1 -maxdepth 1 -printf "%P\n" | while read file; do
    echo "Linking $SOURCE_LOCATION/$file to $TARGET_LOCATION/.$file"
    ln -s "$SOURCE_LOCATION/$file" "$TARGET_LOCATION/.$file"
done

Uninstall Script:

#!/bin/bash
# The uninstall script for my vim dotfiles.
# Find the location of the script.
SCRIPT_DIRECTORY="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"

# The source directory and target directories.
SOURCE_LOCATION="$SCRIPT_DIRECTORY/src" # Contains the files and directories I want to work with.
TARGET_LOCATION="$SCRIPT_DIRECTORY/test-home" # The location I want to link to, in production this would be "~/"

# Remove the links the install script would create.
find $SOURCE_LOCATION -mindepth 1 -maxdepth 1 -printf "%P\n" | while read file; do
    echo "Removing $TARGET_LOCATION/.$file"
    rm -I "$TARGET_LOCATION/.$file"
done

If you're worried about it being safe, probably verifying that what you're deleting is a symlink and bailing out if not would be a good start:

if [[ ! -h $TARGET_LOCATION/.$file ]]; then
    printf >&2 "%s is not a symlink!\n" "$TARGET_LOCATION/.$file"
    return 1
fi

Also, your deploy script doesn't seem to much care if there are existing .vim/.vimrc files to deal with, so I have to wonder: why bother juggling the configs? If there's any possibility someone else's config will be there, best check for it

if [ -e "$TARGET_LOCATION/.$file" ]; then
    printf >&2 "%s exists, cowardly refusing to clobber it\n" "$TARGET_LOCATION/.$file"
    return 1
fi

You might also consider placing this at the top of your script:

set -e

This will cause your script to cease execution immediately if any command in the pipe returns a non-0 status and isn't caught by an explicit check (if/then, &&/||, etc -- see manpage for the gory details). Generally, I recommend to always write shell with this on, but especially so if you're doing something you're nervous will misbehave.

I am not sure about this:

SCRIPT_DIRECTORY="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"

Is that how you are getting an absolute path? I would be cautious, if for some reason that command fails, even set -e won't catch it (launched in a subshell), then you have an unset $SCRIPT_DIRECTORY. That may cause find to fail, but it might cause it to walk your CWD, too.. hard to tell at a glance. I personally use a set of functions I tote around that do path normalization and has proper abspath() function to use, but in the interest of keeping this simple, consider skipping the clever tricks and set an explicit path (assuming it's not going to be changing). If that's not an option, I would add a sanity check to make sure what I'm walking and copying is what I expect. This can be as simple as checking a file is there that wouldn't be in ~ already.

And just throwing this out there: Consider making your dotfiles permanent but with a different name, then use an alias/function to set the desired config, e.g.: myvim () { vim -u "$HOME/.my.vimrc" "$@"; }. Not sure of your situation, but it might be more pleasant/less sketchy way to handle it.


Finding strings inside of other strings in order in C A question on Stack Overflow recently intrigued me to the point of me implementing the functionality in C. Can you critique this algorithm and tell me what is good and bad about it?

This program will tell you if a string is inside of another string.

For example:

"elf" would be inside "self"
"hit" would be inside "chemistry"
"tim" would not be inside "chemistry"
"try" would be inside "chemistry"

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

/*
 * const char* const read only
 * const char* is read/write
 */
int find_str_in_str(const char* const base, const char* const sub)
{
        int base_len = strlen(base);
        int sub_len  = strlen(sub);
        char *tmp_sub = NULL;

        /* allocate enough mem for the max string length */
        if(base_len > sub_len) {
                tmp_sub = malloc(base_len + 1);
        }
        else {
                tmp_sub = malloc(sub_len + 1);
        }

        if(NULL == tmp_sub) {
                fprintf(stderr, "Runtime error (malloc)\n");
                exit(1);
        }

        int i = 0;
        int j = 0;

        for(; i < sub_len; i++) {
                for(; j < base_len; j++) {
                        if(base[j] == sub[i]) {
                                tmp_sub[i] = base[j];
                                /* the first occurance was found */
                                break;
                        }
                }
        }

        tmp_sub[i++] = '\0';

        if(0 == strcmp(sub, tmp_sub)) {
                free(tmp_sub);
                return 1;
        } else {
                free(tmp_sub);
                return 0;
        }
}

int main(int argc, char **argv)
{
        if(argc < 3) {
                fprintf(stderr, "Usage: %s %s %s\n", argv[0], "base", "derived");
                return EXIT_FAILURE;
        }

        fprintf(stdout, "Base :    %s\n", argv[0]);
        fprintf(stdout, "Sub  :    %s\n", argv[1]);

        if(1 == find_str_in_str(argv[1], argv[2])) {
                fprintf(stdout, "Result :    true\n");
        } else {
                fprintf(stdout, "Result :    false\n");
        }

        return EXIT_SUCCESS;
}

Allocating tmp_sub only needs to be the size of sub: if sub is found in base then it will be exactly the same length.

Using this fact, the entire algorithm can be simplified to remove the tmp_sub and calls to strcomp entirely.

In your nested for loops, remove the i loop and continue incrementing j each iteration. Increment i iff sub[i] == base[j]. Then, check i. If i == sub_len return true. If the loop exits (where j is equal to base_len) then return false.

int find_str_in_str(const char* const base, const char* const sub)
{
    int base_len = strlen(base);
    int sub_len  = strlen(sub);
    int i = 0;
    int j = 0;

    for ( ; j < base_len; j++) {
        if (sub[i] == base[j]) i++;
        if (i == sub_len) return 1; // we found all sub chars in order
    }
    return 0; // we reached the end of base without finding all chars
}

Insufficient karma to comment @William Morris
I had returned to write nearly what you have provided as a higher performance / more obscure solution as well. If you exchange the body of your for loop for the obscure trick I had mentioned above (but have now move below) you can avoid one more logic branch in your code.

static int find_str_in_str(const char *base, const char *sub)
{
    // Check arguments for empty string
    for (; *base != '\0' && *sub != '\0'; ++base) {
        sub += (int)(*base == *sub);
    }
    return *sub == '\0';
}

Feel free to edit my answer and remove this lower block if you choose to update your answer.


Drawing a color wheel... faster? Using the following code, I am able to neatly draw a color wheel, however, performance is barely acceptable on a high end machine. Now, I know some tricks I could employ to make it run far more smoothly, at the cost of a lot of flexibility, but I am especially interested in optimizing the code itself (thus keeping the flexibility). And of course in any generic feedback.

// Utility functions (ignore mostly)
function hsv2rgb(h, s, v) {
    var c = v * s;
    var h1 = h / 60;
    var x = c * (1 - Math.abs((h1 % 2) - 1));
    var m = v - c;
    var rgb;

    if (typeof h == 'undefined') rgb = [0, 0, 0];
    else if (h1 < 1) rgb = [c, x, 0];
    else if (h1 < 2) rgb = [x, c, 0];
    else if (h1 < 3) rgb = [0, c, x];
    else if (h1 < 4) rgb = [0, x, c];
    else if (h1 < 5) rgb = [x, 0, c];
    else if (h1 <= 6) rgb = [c, 0, x];

    return [255 * (rgb[0] + m), 255 * (rgb[1] + m), 255 * (rgb[2] + m)];
}
function hsv2rgbString(h,s,v){
    var rgb = hsv2rgb(h,s,v);
    rgb = rgb.map(Math.round);
    return "rgb("+rgb[0]+","+rgb[1]+","+rgb[2]+")";
}

// Code
var canvas = document.getElementById("picker");
var context = canvas.getContext("2d");
var rangeEl = document.getElementById('range');
var x = canvas.width / 2;
var y = canvas.height / 2;
var borderWidth = 2;
var totalRadius = canvas.width / 2 - borderWidth;
var type = "hsv";

// To trigger anti aliasing, performance is poor even without it
context.moveTo(x, y);

var drawPicker = function(){
    context.clearRect (0, 0, 150, 150);
    context.lineWidth = 1;

    for(var i=0;i<totalRadius;i++){
        var radius = totalRadius-i;

        if(type == "hsl"){
            var saturation = Math.floor(radius/totalRadius*100)/100;
            var lightness = rangeEl.value;
        }else if(type == "hsv"){
            var saturation = Math.floor(radius/totalRadius*100)/100;
            var value = rangeEl.value;
        }

        for(var angle=0; angle<=360; angle+=1){
            var startAngle = (angle-1) * Math.PI/180;
            var endAngle = (angle+1) * Math.PI/180;
            context.beginPath();
            context.arc(x, y, radius, startAngle, endAngle, false);
            context.closePath();
            if(type == "hsl"){
                context.strokeStyle = 'hsl('+angle+', '+saturation*100+'%, '+lightness*100+'%)';
            }else if(type == "hsv"){
                context.strokeStyle = hsv2rgbString(angle,saturation,value);
            }
            context.stroke();
        }
    }

    // Draw the final black border
    context.strokeStyle = "rgb(0,0,0)";
    context.lineWidth = borderWidth;
    context.beginPath();
    var borderRadius = totalRadius+borderWidth/2;
    context.arc(x, y, borderRadius, 0, 2*Math.PI, false);
    context.closePath();
    context.stroke();
}

drawPicker();
rangeEl.addEventListener("change",drawPicker);
<canvas id="picker" width="150" height="150"></canvas><br/>
<input id="range" type="range" min="0" max="1" value="1" step="0.01">

I am asking for a code review, but if anybody knows what's causing the white lines (the Moiré pattern) appear at a low value (the slider) then I would be forever grateful. Or rather, I get why they appear, I just don't know how to solve it... beyond doubling the resolution and downscaling.

I like the idea of drawing it using arc segments, but it's probably not the most efficient. There's no doubt some extra loss in coverting to a CSS-style rgb(...) string, which then has to be parsed again. And, as you say, it causes the Moiré artefacts. Otherwise, the code looks pretty good.

While I won't guarantee it's always faster (and there are some browser support issues), you can try drawing the the color wheel pixel by pixel to an ImageData object, and pushing that to the canvas all at once. It seems faster, but I've also left out clipping the data to a wheel shape here.

You might also consider a square color field (rather than a wheel). That what this is of course, but the corners of it aren't too useful. Photoshop, for instance, defaults to a square field with saturation / lightness along the axes, and the slider determining the hue.

But Photoshop also has an RGB mode, where two of the components are used for the axes, and the last one is controlled by the slider. If you can construct a field that only requires you to update 1 byte per pixel, that'd probably be faster still.

var canvas = document.getElementById("picker");
var context = canvas.getContext("2d");
var rangeEl = document.getElementById('range');

function drawPicker() {
  // grab the current ImageData (or use createImageData)
  var bitmap = context.getImageData(0, 0, 150, 150);

  for (var y = 0; y < 150; y++) {
    for (var x = 0; x < 150; x++) {
      // offset for the 4 RGBA values in the data array
      var offset = 4 * ((y * 150) + x);

      var hue = 180 + Math.atan2(y - 75, x - 75) * (180 / Math.PI);
      var saturation = Math.sqrt(Math.pow(y - 75, 2) + Math.pow(x - 75, 2)) / 75;
      var value = rangeEl.value;

      saturation = Math.min(1, saturation);

      var hsv = hsv2rgb(hue, saturation, value);

      // fill RGBA values
      bitmap.data[offset + 0] = hsv[0];
      bitmap.data[offset + 1] = hsv[1];
      bitmap.data[offset + 2] = hsv[2];
      bitmap.data[offset + 3] = 255; // no transparency

    }
  }

  // update the canvas
  context.putImageData(bitmap, 0, 0);
}

// setup
drawPicker();
rangeEl.addEventListener("change", drawPicker);

// =========

function hsv2rgb(h, s, v) {
  var c = v * s;
  var h1 = h / 60;
  var x = c * (1 - Math.abs((h1 % 2) - 1));
  var m = v - c;
  var rgb;

  if (typeof h == 'undefined') rgb = [0, 0, 0];
  else if (h1 < 1) rgb = [c, x, 0];
  else if (h1 < 2) rgb = [x, c, 0];
  else if (h1 < 3) rgb = [0, c, x];
  else if (h1 < 4) rgb = [0, x, c];
  else if (h1 < 5) rgb = [x, 0, c];
  else if (h1 <= 6) rgb = [c, 0, x];

  var r = 255 * (rgb[0] + m);
  var g = 255 * (rgb[1] + m);
  var b = 255 * (rgb[2] + m);

  return [r, g, b];
}
<canvas id="picker" width="150" height="150"></canvas>

<input id="range" type="range" min="0" max="1" value="1" step="0.01">

Alternative to that: Cheat! Make the CSS background of the canvas black, and fade the color wheel in front of it using the slider. That's one way to get away with just changing 1 byte per pixel, and letting the browser handle compositing.

var canvas = document.getElementById("picker");
var context = canvas.getContext("2d");
var rangeEl = document.getElementById('range');
var colorWheel = null;

// update the alpha bytes of the color wheel
function updatePicker() {
  var bitmap = context.getImageData(0, 0, 150, 150);
  for (var y = 0; y < 150; y++) {
    for (var x = 0; x < 150; x++) {
      var offset = ((y * 150) + x) * 4 + 3;
      colorWheel.data[offset] = rangeEl.value | 0;
    }
  }

  // update the canvas
  context.putImageData(colorWheel, 0, 0);
}

// setup
drawPicker();
rangeEl.addEventListener("change", updatePicker);

// =========

function hsv2rgb(h, s, v) {
  var c = v * s;
  var h1 = h / 60;
  var x = c * (1 - Math.abs((h1 % 2) - 1));
  var m = v - c;
  var rgb;

  if (typeof h == 'undefined') rgb = [0, 0, 0];
  else if (h1 < 1) rgb = [c, x, 0];
  else if (h1 < 2) rgb = [x, c, 0];
  else if (h1 < 3) rgb = [0, c, x];
  else if (h1 < 4) rgb = [0, x, c];
  else if (h1 < 5) rgb = [x, 0, c];
  else if (h1 <= 6) rgb = [c, 0, x];

  var r = 255 * (rgb[0] + m);
  var g = 255 * (rgb[1] + m);
  var b = 255 * (rgb[2] + m);

  return [r, g, b];
}

// create the initial color wheel
function drawPicker() {
  colorWheel = context.getImageData(0, 0, 150, 150);

  for (var y = 0; y < 150; y++) {
    for (var x = 0; x < 150; x++) {
      var offset = 4 * ((y * 150) + x);

      var hue = 180 + Math.atan2(y - 75, x - 75) * (180 / Math.PI);
      var saturation = Math.sqrt(Math.pow(y - 75, 2) + Math.pow(x - 75, 2)) / 75;

      saturation = Math.min(1, saturation);

      var hsv = hsv2rgb(hue, saturation, 1); // fixed value

      colorWheel.data[offset + 0] = hsv[0];
      colorWheel.data[offset + 1] = hsv[1];
      colorWheel.data[offset + 2] = hsv[2];
      colorWheel.data[offset + 3] = 255;
    }
  }
  context.putImageData(colorWheel, 0, 0);
}
#picker {
  background: black
}
<canvas id="picker" width="150" height="150"></canvas>

<input id="range" type="range" min="0" max="255" value="255" step="1">

Or better yet: Cheat completely. Just put an image of a color wheel on top of a black box, and fade the image using the slider. No canvas, no pixel manipulation, none of that.

var wheel = document.getElementById("wheel"),
    slider = document.getElementById("slider");

slider.addEventListener("change", function () {
  wheel.style.opacity = this.value;
});
#picker {
  width: 150px;
  background: black;
}

<input id="slider" type="range" value="1" min="0" max="1" step="0.01">

That's probably the simplest :)

Note: I haven't benchmarked any of the above. That's left as an exercise to the reader.


Array duplication in JavaScript I want to achieve this

var arr = [1,2,3];

var duplicateArr = duplicate(arr, 2) // will be [1,2,3,1,2,3]

var duplicateArr2 = duplicate(arr, 3) // will be [1,2,3,1,2,3,1,2,3]

I have implemented it like this:

function duplicate(arr, n) {
  var len = arr.length;
  if (n == 0 || n == 1) {
    return arr;
  }
  if (n > 1) {
    for (var j = 0; j < n - 1; j++) {
      for (var i = 0; i < len; i++) {
        arr.push(arr[i])
      }
    }
  }
  console.log(arr);
}
duplicate([1, 2, 3], 3);

I know this may not be the efficient way to do it.

You can avoid the nested for loops by adding the entire array at the same time. (I'm also using Janos' suggestion here)

function duplicate(arr, n) {
  let result = [];
  for(let i=0; i<n; i++) {
    result.push(...arr);
  }
  return(result);
}

We need the original array unmodified, so we can keep adding it, so I'm using a new result array. This has a few benefits. Firstly it's more common to leave the input array unmodified and return a new array, and secondly it returns an empty array when n=0, which I think makes more sense.

This uses the ES6 spread syntax. If you don't want to use ES6 you can replace

result.push(...arr);

with

result = result.concat(arr);


Three way Quicksort in Java What do you think about this implementation of quicksort? Is it possible to make it shorter?

/**
 * 3-way quicksort
 * <p/>
 * Choose a value an put the greater values to the right, lowers to the
 * left, and equals in the center. recursive. O(n^2) but avg O(n log n). Memory: In place
 */
public static void quickSort(int[] array) {
    shuffleArray(array);
    quickSort(array, 0, array.length - 1);
}

private static void quickSort(int[] array, int lo, int hi) {
    if (hi <= lo)
        return;

    int lowerIndex = lo;
    int greaterIndex = hi;
    int element = array[lo];
    int i = lo;

    while (i <= greaterIndex) {
        if (array[i] < element)
            swap(array, lowerIndex++, i++);
        else if (array[i] > element)
            swap(array, i, greaterIndex--);
        else
            i++;
    }

    quickSort(array, lo, lowerIndex - 1);
    quickSort(array, greaterIndex + 1, hi);
}

// O(n)
private static void shuffleArray(int[] ar) {
    Random rnd = new Random();
    for (int i = ar.length - 1; i > 0; i--) {
        int index = rnd.nextInt(i + 1); // random between 0 and i
        swap(ar, i, index);
    }
}

private static void swap(int[] array, int i, int j) {
    int a = array[i];
    array[i] = array[j];
    array[j] = a;
}

i should be initialized to lo + 1 this will reduce the while loop by 1
you should use braces {} for single if statements. This will make your code less errorprone.
instead of doing this recursively, you could do is iterativ by adding a while loop with (lo < lowerIndex - 1 && greaterIndex + 1 < hi) as condition.
for make the if..else if you shoul consider to swap the position of element and array [i] because if you imagine the array, index 0 will be on the left

if (element > array[i])


Validating a string for a positive number, null/empty string I have a method in which I am accepting a String clientId and that has below requirements:

clientId can be a positive number greater than zero. But if it is negative number or zero, then throw IllegalArgumentException with a message.
clientId cannot be a null or empty string. But if it is, then throw IllegalArgumentExceptionwith a message.
clientId can be a normal string as well. For example - it can be abcdefgh or any other string.

import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;

public Builder setClientId(String clientId) {
    checkNotNull(clientId, "clientId cannot be null");
    checkArgument(clientId.length() > 0, "clientId can't be an empty string");
    checkArgument(!clientId.matches("-\\d+|0"), "clientId must not be negative or zero");
    this.clientId = clientId;
    return this;
}

This code works fine. Is there any other better way to write above logic? I am using Google Guava preconditions class for this. I have lot of other methods as like above in which I am using Google Guava preconditions to do some sanity checking.

The code looks very good.

Check your requirements to see if "00", "0x0", etc are supposed to be a valid client ids.  Whatever the answer, write unit tests to capture the requirements and validate that you are meeting them.

As written, you are violating the single responsibility principle: this constructor is acting both as a coordinator (figuring out which checks to run) and as a calculator (computing whether or not the regular expression is a match).  I'd probably leave it, because the pattern is so common.

You are also (possibly) sharing some of the validation code for the "general" string based id (not null, not empty), which the rules specific to ClientId (not '0', not a negative integer).  If so, some refactoring is called for.

String.matches(regex) is just a wrapper around Pattern.matches(regex,input).  Given that you know that the regular expression isn't going to change from one call to the next, you might create a single static instance of the Pattern so that you aren't compiling it multiple times.  It does complicate the logic in the constructor somewhat

checkArgument(! pattern.matcher(clientId).matches(), "clientId must not be negative or zero");

I'd be more likely to do this if I moved the match calculation into a separate function/factory -- the performance gains are not likely to be measurable, and readability is important.

I would suggest that you consider is that your representation of the data, after it has passed all of its validation checks, should probably differ from the representation before the checks have been validated.

public class ClientId {
    // final, because we don't want code to later change the id to
    // something that isn't valid.
    private final String clientId;

    public ClientId(String clientId) {
        checkNotNull(clientId, "clientId cannot be null");
        checkArgument(clientId.length() > 0, "clientId can't be an empty string");
        checkArgument(!clientId.matches("-\\d+|0"), "clientId must not be negative or zero");

        this.clientId = clientId;
    }
}

class Builder {
    public Builder setClientId(String clientId) {
        return setClientId(new ClientId(clientId));
    }

    public Builder setClientId(ClientId clientId) {
        this.clientId = clientId;
        return this;
    }
}

Done this way, all of the code that interacts with a client id knows from the data type that the constraints on the data have already been verified.

Typically, when implementing a value type this way, you'll also want to replace the default implementations of equals() and hashcode() to make sure that you still have string like semantics available.

Less typically, you will find examples where the standard is to use marker interfaces to denote values.

public ClientId implements ValueObject<ClientId> { ... }

or even

public ClientId implements Id<ClientId> {...}

The marker interfaces will sometimes specify queries that the types must implement

public interface ValueObject<T> {
    boolean sameValueAs(T other);
}

You provide a bit of extra work in the class implementation, and you get some clarity/readability in the consumer code.

In some domains, you may need to worry about the fact that clever attackers can capture a reference to an incompletely constructed value type.  In that case, putting the data validation in the constructor won't suffice, you'll instead want to use a factory method to construct the object.  For instance:

public class ClientId {
    private final String clientId;

    private ClientId(String clientId) {
        this.clientId = clientId;
    }

    public static ClientId create(String clientId) {
        checkNotNull(clientId, "clientId cannot be null");
        checkArgument(clientId.length() > 0, "clientId can't be an empty string");
        checkArgument(!clientId.matches("-\\d+|0"), "clientId must not be negative or zero");

        // with the checks verified, we take advantage of the fact that this
        // method has access to the private constructor that is inaccessible
        // everywhere else.
        return new ClientId(clientId);
    }
}

Written this way, the checks happen before the call to the java.lang.Object constructor exits, which is a critical point in the lifetime of the potential reference.


Triangle calculator: calculate if it's a valid triangle and what type This is my first program and I wanted to know if there was any way to improve the design or cut back on unnecessary coding.

import java.util.Scanner;

public class Main {

    public static void main(String args[]) {

        int side1; int side2; int side3;
        int side1s; int side2s; int side3s;
        Boolean Triangle;
        String TriangleType;
        String Q1 = "What is the first side of the Triangle?";
        String Q2 = "What is the second side of the Triangle?";
        String Q3 = "What is the third side of the Triangle?";

        Scanner input = new Scanner(System.in);
        System.out.println(Q1);
        side1 = input.nextInt();

        Scanner input2 = new Scanner(System.in);
        System.out.println(Q2);
        side2 = input2.nextInt();

        Scanner input3 = new Scanner(System.in);
        System.out.println(Q3);
        side3 = input3.nextInt();

        if(side1 + side2 > side3) {
            Triangle = true;
            System.out.println("The triangle is true!");

        } else {
            Triangle = false;
            System.out.println("The Triangle is False!");
            return;

        }

        if(Triangle == true) {
            side1s = side1 * side1;
            side2s = side2 * side2;
            side3s = side3 * side3;

            if(side1s + side2s == side3s) {
                TriangleType = "Right";

                System.out.println("The Triangle is..." + TriangleType);

            } else if (side1s + side2s > side3s) {
                TriangleType = "Acute";

                System.out.println("The Triangle is..." + TriangleType);

            } else if(side1s + side2s < side3s) {
                TriangleType = "Obtuse";

                System.out.println("The Triangle is..." + TriangleType);

            } else {
                TriangleType = null;

                System.out.println("The Triangle is..." + TriangleType);

            }

        }
    }

}

Bug
Your code assumes that the user will enter the longest side last! This is a bug. Id the user enters the values 10, 1, and 1, it will declare that it is a triangle because 10 + 1 > 1
Functions!
Where are your functions?
One of the best ways to document your code, and make sense of it, is to have well-named functions. Your function is main, and.... well, that does not do a good job of telling us what the function does.
What you need is a function called something like boolean isValidTriangle(int sideA, int sideB, int sideC), then make that function "work".
A valid triangle is one whose shorter sides add up to more than the longest side, right? The trick is finding the longest side.... In this case, recursion is a trick that can help....
boolean isValidTriangle(int sideA, int sideB, int sideC) {

    // recursion trick, make sure sideA is the longest side!

    if (sideA < sideB) {
        // call the function with sideB first
        return isValidTriangle(sideB, sideA, sideC)
    }

    if (sideA < sideC) {
        // call the function wwith sideC first
        return isValidTriange(sideC, sideB, sideA)
    }

    // OK, sideA is the longest, for sure!
    return sideA <= sideB + sideC

}

Similarly, you can do the same sort of thing to classify the triangle types.


Regular expression to identify java function I am trying to identify a java function block addition  in a diff file (new line start with + in diff).
For that I am using regular expression.

It is working fine, however I need to identify all (if not, at least maximum) possible combinations of keyword used in java method declarations.

Here is my regular expression written in my java program

Pattern functionPatter = Pattern.compile("\\+\\s*(public|private|protected)?(\\s+static)?(\\s+final)?\\s+\\w+\\s+\\w+\\s*\\(.*?\\)\\s*(throws .*)+\\{.*\\}",Pattern.DOTALL);

Please suggest me if I am missing anything.

I have no idea what you are trying to match with \\s+\\w+\\s+\\w+\\s*.  If you are looking for a return type, then that won't do at all.

It appears that you are trying to match the function body using \\{.*\\}.  That won't work, because braces may be nested, and regular expressions cannot handle nesting.

Modifier keywords can appear in any order, and you are missing a few, such as:

synchronized
transient
volatile
strictfp

If you want to implement this correctly, don't just make assumptions based on code that you have been seeing.  Read the Java Language Specification.


Add one to a very large integer I was asked to implement this in an interview a while back. I didn't pass the interview, mostly likely because it took me far too long to do. However, I'm interested to know how well implemented the actual code is.

The task was to:

  Add one to a positive number represented as an array of integers.

The question is the same as this one. However, I have done it in JavaScript and my implementation is different (recursion rather than loops), so I hope they are sufficiently different.

var addToArray = function(n){
    var result = [];

    function carryOne(n) {
        if (n.length === 0) {
            result.push(1);
        }
        else {
            var length = n.length-1;
            if (n[length] < 9) {
                n[length] += 1;
                result.push(n);
            } else {
                result.push(0);
                carryOne(n.slice(0, length));
            }
        }
    }
    carryOne(n);
    result.reverse();
    var flatten = Array.prototype.concat.apply([], result);
    console.log(flatten);
}

I've just noticed I have missed the semicolons on the closing braces on the function definitions but I'm keeping it like that for honesty.

In general it works but I think the need to reverse and flatten the array at the end is quite inelegant, so I suspect there is a better solution.

The whole code is overengeneered, very much so in my opinion. The code can be reduced to:

function digitsIncrement(digits) {
    return toDigits(fromDigits(digits) + 1);
}

The two functions mentioned are easy to write, should take 1 to 5 lines each and be immediately understandable.

Remember to convert to a common format, calculate and convert back instead of working in a weird format.


p0-Pollard implementation I should implement p0-Pollard with C++14 and a good algorithm. How may I improve my code?

#include <iostream>
#include <assert.h>

template <typename T>
T gcd(T a, T b){
//    printf("step1\n");
    if(a<0) a=-a;
    if(b<0) b=-b;

    T c = 1;// -> decltype(a+b);
    while((a%2 == 0)&&(b%2 == 0)){
//        printf("step2\n");
        a=a>>1;
        b=b>>1;
        c*=2;
    }
//    printf("step3\n");
    while(a!=b){
        if(a%b==0)
            return c*b;
        if(b%a==0)
            return c*a;
        if(a%2 == 0)
            a=a>>1;
        if(a>b)
            a=(a-b);
        if(b>a)
            b=(b-a);
        if(b%2 == 0)
            b=b>>1;
    }
    return c*a;
}

template <typename T>
T fx(T x, T a, T c, T modN){
    auto tmp = (a*x*x + c)%modN;
//    assert(tmp==0); // for tested simple numbers always tmp==0
    return tmp;
}

template <typename T>
T PoPollard(T src){
    T x1=2, x2=2, a=1, c=1, d=1;
    while(d==1){
        x1=fx(x1, a, c, src);
//        std::cout << "x1 is:" << x1 << "\n";
        x2=fx(fx(x2, a, c, src), a, c, src);
//        std::cout << "x2 is:" << x2 << "\n";

        if(x2==0) return 1; // looks like when src is simple number f(f(x))==0

        if(x1==x2){
            x1=2, x2=2;
            a++;
            continue;
        }

        d=gcd(abs(x1)-abs(x2), src);
//        std::cout << "d is:" << d << "\n";
    }
    return src/d;
}

int main()
{
    std::cout << "Hello gcd:" << gcd(256,16) << "\n";
    std::cout << "Testing Po-Pollard:" << PoPollard(4253) << "\n";
//    printf("Hello world! %ld\n", gcd(256,16));
//    printf("Testing Po-Pollard: %ld\n", PoPollard(4253));
    return 0;
}

A few things strike me as fairly obvious possibilities to consider here.

Use functions where appropriate

if(a<0) a=-a;

I think I'd prefer to have a separate little function to compute the absolute value.

Be consistent

    while((a%2 == 0)&&(b%2 == 0)){
//        printf("step2\n");
        a=a>>1;
        b=b>>1;
        c*=2;
    }

I'd pick either bit-oriented or mathematically-oriented operations, and use them consistently. That would mean either using shifts and bit-tests throughout:

 while(((a & 1) == 0) && ((b & 1) == 0)){
        a >>= 1;
        b >>= 1;
        c <<= 1;
    }

...or else using mathematical operators throughout:

 while((a % 2 == 0) && (b % 2 == 0)){
        a /= 2;
        b /= 2;
        c *= 2;
    }

Also note the compound assignment operators--at least in my opinion, they're much clearer than things like A = A >> 1. Maybe it's just a background in math, but I've never like things like A = A + 1; that always look to me like they're just clearly false statements.

Better variable names

Most of your variable names are so short they're basically meaningless--a, b, d, x1, x2, and so on. I'd much rather see names that made their intended use/purpose easier to understand.

Type checking

Although it can quickly get into extremely advanced programming, I'd at least consider adding some type-checking to your templates. Just for example, I'd guess all the templates in this code really only make sense if T is some integer type, not floating point, a string, etc. That being the case, it's fairly easy to add at least minimal type-checking to enforce that. One simple form of that would use static_assert:

template <typename T>
T gcd(T a, T b){
    static_assert(std::is_integral<T>::value, "Only integer types allowed");

The code almost certainly won't compile if somebody attempts (for example) to get the GCD of two strings--but with the static_assert, you're pretty much guaranteed that at least part of the error message they get will say "Only integer types allowed". Without it, they're likely to get a giant avalanche of error message telling them about things like there not being an >> overloaded for string and int, followed by a 5-page list of the overloads of >> that it considered and rejected in trying to figure out a way to compile the code.

Note: there's work in progress on a set of extensions to C++ called "Concepts" to support this a little more directly in the language. Some compilers already have experimental support, in case you want to try it.


how to avoid switch-case How can I refactor this code to avoid switch-cases? Is there a way of changing commands without adding new case and make it dynamically...I want the solution not to depend on the amount of cases. For example, if I have 1000 commands..I should obviously write them all as cases?
I hope my problem is clear.

static void Main(string[] args)
{
    ConsoleHelp ch1 = new ConsoleHelp();
    Plus pl = new Plus();
    Minus mn = new Minus();
    Divide dv = new Divide();
    Multiply mlt = new Multiply();
    Sinus sin = new Sinus();
    Tangent tan = new Tangent();
    Square sq = new Square();
    Degree dg = new Degree();
    Percent pr = new Percent();

    while (true)
    {
        Console.Write("command> ");
        string com = Console.ReadLine();
        if (com != null)
        {
            switch (com.ToLower())
            {
                case "?":
                    ch1.Helpper();
                    break;
                case "plus":
                    pl.Sum();
                    break;
                case "minus":
                    mn.Minusvalue();
                    break;
                case "divide":
                    dv.Dividevalue();
                    break;
                case "multiply":
                    mlt.Multvalue();
                    break;
                case "sinus":
                    sin.Sinusvalue();
                    break;
                case "tangent":
                    tan.Tangentvalue();
                    break;
                case "square":
                    sq.Squarevalue();
                    break;
                case "degree":
                    dg.Degvalue();
                    break;
                case "percent":
                    pr.Pervalue();
                    break;
                case "c":
                    Environment.Exit(0);
                    break;

                default:
                    Console.WriteLine("The command is not supported. Enter one of the valid commands.");
                    break;
            }
        }
    }
    Console.ReadLine();
}

You can create your own Attribute to decorate all of your classes (e.g., Plus, ConsoleHelp, Minus, Divide, and even Exit).  In the sample below, the attribute is called SystemCommandAttribute.  It would require to specify what the name of the command would be as a constructor parameter (what the user would have to type on the command line).

The next step would be to have each of your command classes implement the same interface.  In the example code I used the name ISystemCommand.  That interface should have a single method:  Execute().  That way we know what to call when we want to invoke the command.

Then you can use reflection to search for all classes that have that attribute and load them into a dictionary of string to ISystemCommand.  The GetCommandsFromAssemblies() method handles instantiating each instance of ISystemCommand for you.

From there you can use the dictionary instead of the switch statement.

public class Program
{
    static void Main(string[] args)
    {
        Dictionary<string, ISystemCommand> commandToType = GetCommandsFromAssemblies();
        RunCommandLoop(commandToType);
    }

    /// <summary>
    /// Loop over all types currently loaded in the AppDomain looking for ISystemCommand
    /// </summary>
    /// <returns>A dictionary from command name to instance of command</returns>
    private static Dictionary<string, ISystemCommand> GetCommandsFromAssemblies()
    {
        var typesWithMyAttribute =
            from a in AppDomain.CurrentDomain.GetAssemblies()
            from t in a.GetTypes()
            let attributes = t.GetCustomAttributes(typeof(SystemCommandAttribute), true)
            where attributes != null && attributes.Length > 0
            select new
            {
                Instance = (ISystemCommand)Activator.CreateInstance(t),
                CommandName = attributes.Cast<SystemCommandAttribute>().Single().CommandName
            };

        Dictionary<string, ISystemCommand> commandToType = typesWithMyAttribute.ToDictionary(t => t.CommandName, t => t.Instance);

        return commandToType;
    }

    private static void RunCommandLoop(Dictionary<string, ISystemCommand> actions)
    {
        while (true)
        {
            Console.Write("command> ");
            string com = Console.ReadLine();
            if (com != null)
            {
                ISystemCommand action;

                if (actions.TryGetValue(com, out action))
                {
                    action.Execute();
                }
                else
                {
                    Console.WriteLine("The command is not supported. Enter one of the valid commands.");
                    break;
                }
            }
        }
        Console.ReadLine();
    }
}

public interface ISystemCommand
{
    void Execute();
}

public class SystemCommandAttribute : Attribute
{
    public SystemCommandAttribute(string commandName)
    {
        CommandName = commandName;
    }

    public string CommandName { get; set; }
}

[SystemCommand("plus")]
public class Plus : ISystemCommand
{
    public void Execute()
    {
        Sum();
    }

    private void Sum()
    {
        // your existing Sum logic here
        Console.WriteLine("Sum called");
    }
}

[SystemCommand("?")]
public class ConsoleHelp : ISystemCommand
{
    public void Execute()
    {
        Helper();
    }

    private void Helper()
    {
        // your existing Helper logic here
        Console.WriteLine("Helper called");
    }
}

[SystemCommand("c")]
public class ExitCommand : ISystemCommand
{
    public void Execute()
    {
        Environment.Exit(0);
    }
}

To add a new command in the future, all you would need to do would be to

create your command class
Inherit from ISystmeCommand
Add the SystmCommandAttribute

Your main program would not need to change at all.


Given a set of closed intervals, find the smallest set of numbers that covers all the intervals The Task:

  Given a set of closed intervals, find the smallest set of numbers that
  covers all the intervals. If there are multiple smallest sets, return
  any of them.

  For example, given the intervals [0, 3], [2, 6], [3, 4], [6, 9], one
  set of numbers that covers all these intervals is {3, 6}.

My solution:

const intervalls = [[0, 3], [2, 6], [3, 4], [6, 9]];
const getLargestMinAndSmallestMax = (acc, intervall, _, src) => {
    if (src.length === 0) { return []; }
    if (src.length === 1) { return intervall; }
    if (acc[1] === undefined || intervall[0] > acc[1]) {
      acc[1] = intervall[0];
    }
    if (acc[0] === undefined || intervall[1] < acc[0]) {
      acc[0] = intervall[1];
    }
    return acc;
  };
const smallestIntervallCoveringAllIntervalls = list => list.reduce(getLargestMinAndSmallestMax, []);

console.log(smallestIntervallCoveringAllIntervalls(intervalls));

Is there a faster and cleaner version of it?

Naming
First point is that I think the naming is getting a little verbose smallestIntervallCoveringAllIntervalls and getLargestMinAndSmallestMax

getLargestMinAndSmallestMax get largest min and smallest max, don't you mean get "range"? And its not get, the range is not known, you need to "find" it. findRange

smallestIntervallCoveringAllIntervalls This is when comments come into play. If you were part of a team that functions name would already be defined in the interface spec. However if it was an internal call (your code) you would use a smaller name and add a comment to give it the missing meaning as a reminder to you, or for someone that may later need to modify your code.

Spelling
intervalls should be intervals
Bad spelling is a gateway to bug hell. Others can not guess how you have misspelled a name, so always check if you are unsure as to the spelling of a name.
The spec
No project starts without a detailed specification regarding the problem. Part of a programmers job is to ensure that the spec is unambiguous.
If you have any questions that the spec can not answer then you should not start working on a solution. Rather a memo to the team leader, or email to the client with the points that need to be resolved.
So lets restate the problem

Given an array of intervals find the smallest interval that covers all intervals.

The intervals array will always contain one or more valid intervals.

An interval is covering another interval if one or more values in its range are the same. Examples A = [0, 1], B = [1, 2], C = [3, 4]. A covers B, C does not cover A and B

An interval is an array containing two unsorted integers, that have a range from min to max value inclusive. Examples [0, 1] has a range 0, 1. [3, -2] has a range  -2, -1, 0, 1, 2, 3. [2, 2] has a range 2.
The following [], [0], [1,2,3] are not valid intervals

The return must be a valid interval

Your code
You have a bug and return incorrect intervals in some cases.
Two examples of the bug

Given  the intervals [[10, 20], [10, -20]] your function returns [-20,10] which is in fact the largest range, the correct solution is [10, 10] (Yes unfair as [10,-20] is A over T)

Given intervals [[10,20]] you incorrectly return [10,20]. There are 11 possible correct solutions. Any of  [10,10], [11, 11], ..., [n, n] (n = 20)

The reason you are returning incorrect intervals is that you do not treat the first interval correctly, as shown in the second example above.
When finding min and max values of a set of anything with at least one item, you always treat the first item differently. The first items initializes the min or max.
You use Array.reduce so that means you need to include a test to see if you have the first interval.
The interval that covers a single interval always has a range length of one and is any of the values in the intervals range. eg [[10,20]] is covered by [10,10] or [20,20]
Fix using your logic
// int for interval
const smallestRange = (res, int) => {
    if (res.length === 0) {  return [int[0], int[0]] }
    if (int[0] > res[1]) { res[1] = int[0] }
    if (int[1] < res[0]) { res[0] = int[1] }
    return res;
};
const findSmallestRange = ints => ints.reduce(smallestRange , []);

If we now solve to include intervals that are backward (the other question said use Array.sort. NEVER! use sort for a two item array, in fact you only use sort if you can find no other way to sort any items)
Solve new spec
Solution using while loop and to spec outlined above.
Note that I use lowIdx + 1 & 1 as it is slightly faster than (lowIdx + 1) % 2. This is not a micro optimization, this is a known optimization, when all things equal always use the fastest method. (also note that % has lower precedence than + so requires the ()
Using Math.min and Math.max
"use strict";
// Renamed interval to set
function findCoveringInterval(sets) { // finds smallest
    var i = 0, min = sets[i][0], max = min;
    while (++i  < sets.length) { // must be ++i not i++
        const set = sets[i], lowIdx = set[0] < set[1] ? 0 : 1;
        max = Math.max(set[lowIdx], max);
        min = Math.min(set[lowIdx + 1 & 1], min);
    }
    return [max, min];
}

I prefer using ternary min max because Math.min/max has additional overhead checking against the length of its argument array. However this makes the function longer so it counts as a micro optimization. (I would need to test performances as I add JS complexity competing against native complexity)
"use strict";
function findCoveringInterval(sets) {
    var i = 0, min = sets[i][0], max = min;
    while (++i < sets.length) {
        const set = sets[i];
        let idx = set[0] < set[1] ? 0 : 1;
        max = set[idx] > max ? set[idx++] : (idx++, max);
        min = set[idx &= 1] < min ? set[first] : min;
    }
    return [max, min];
}


Calculating Standard Deviations We were discussing Standard Deviations in chat, so I decided to write a standard deviation calculator:

static double PopulationStandardDeviation(List<double> numberSet)
{
    double mean = numberSet.Average();

    return Math.Sqrt(numberSet.Sum(x => Math.Pow(x - mean, 2)) / numberSet.Count);
}

static double SampleStandardDeviation(List<double> numberSet)
{
    double mean = numberSet.Sum() / numberSet.Count;

    return Math.Sqrt(numberSet.Sum(x => Math.Pow(x - mean, 2)) / (numberSet.Count - 1));
}

How could these be improved?  Because they are both Standard Devation calculators, should I combine them into one method with a header of static double StandardDeviation(List<double> numberSet, bool isSample)?  Are they an optimal solution for the problem?

It's good to have 2 methods.
For the reason that @britishtea said in a comment,
which is so spot-on I'm just going to quote it verbatim:

  A method that takes a boolean value as a parameter and behaves differently depending on that value is not one method, but two methods! You should keep them separated. You couldc of coursec implement them using shared functionality, but your public API should offer two methods. :) – @britishtea

And indeed, this is how the .NET framework does it too
(See StandardDeviation() and StandardDeviationP(), thanks @200_success).

At the same time, of course don't repeat yourself.
Extract the common logic to a private helper method,
and rewrite your public methods in terms of the private one:

private static double StandardDeviation(List<double> numberSet, double divisor)
{
    double mean = numberSet.Average();
    return Math.Sqrt(numberSet.Sum(x => Math.Pow(x - mean, 2)) / divisor);
}

static double PopulationStandardDeviation(List<double> numberSet)
{
    return StandardDeviation(numberSet, numberSet.Count);
}

static double SampleStandardDeviation(List<double> numberSet)
{
    return StandardDeviation(numberSet, numberSet.Count - 1);
}

Lastly, the parameter name "numberSet" is a misnomer,
suggesting a Set, when it's really a List.


k-combinations in lexicographic order (using given algorithm) I already solved the problem, but it seems my code is too awkward, and with a different approach there could be a more elegant solution.
Notice that the general algorithm is described in the instructions:

This is the problem description:

  Given unsigned integers 0 ≤ k ≤ n, generate all the k-combinations of
  the n objects, numbered 1, 2, ... , and n, using the following
  algorithm:

  Starting with the combination 1, 2, ..., k, the next combination is
  found by scanning the current combination from right to left so as to
  locate the rightmost element that has not yet attained its maximum
  value. This element is incremented by one, and all positions to its
  right are reset to the lowest values possible.

  Each line contains a set of n and k which separated with at least one
  space.

  For each case, the first line is a prompt as following: “case 1:”. The
  next lines are all of the combinations. Notice that one line for one
  combination and list the results in ascending order. You don’t have to
  print any thing if the result is null. The last line of each case is
  an empty line, even if the result is null.

#include <iostream>
#include <algorithm>
#include <iterator>
#include <vector>

int combination_find(std::vector<int> combination, int max_val);
void combination_inc(std::vector<int>& combination, const int& pos, const int& max_val);

int main()
{
    int round = 0;
    int n, k;
    while (std::cin >> n >> k)
    {
        //init
        std::vector<int> combination;
        int i = 0;
        while (i < k)
        {
            combination.push_back(++i);
        }
        std::cout << "case " << ++round << ": " << std::endl;
        while(1)
        {
            //writes (upon conditional so that the print of empty vectors doesn't mess up the text formatting0
            if (!combination.empty())
            {
                std::copy(combination.begin(), combination.end(), std::ostream_iterator<int>(std::cout, " "));
                            std::cout << std::endl;
            }
            //check
            int pos = combination_find(combination, n);
            if (pos < 0) {break;}
            //increment
            combination_inc(combination, pos, n);
        }
        std::cout << std::endl;
    }
}

int combination_find(std::vector<int> combination, int max_val)
{
    if (combination.empty()) { return -1; }
    if (combination.back() != max_val)
    {
        return (combination.size() - 1);
    }
    else
    {
        combination.pop_back();
        return combination_find(combination, --max_val);
    }
}

void combination_inc(std::vector<int>& combination, const int& pos, const int& max_val)
{
    int val = combination[pos];
    for (unsigned i = pos; i < combination.size(); ++i)
    {
        combination[i] = ++val;
    }
}

combination_find() takes \$O(K^2)\$ time and space because of the pass by value. This is unnecessary overhead and can cause trouble when K is large. Instead we can easily find the position iteratively.

int combination_find(const std::vector<int> &combination, int max_val) {
  for (int i = combination.size() - 1; i >= 0; --i, --max_val) {
    if (combination[i] != max_val)
      return i;
  }
  return -1;
}

This also allows us to pass the vector by reference.

A few more minor points

combination_inc() does not need max_val as a parameter
The push_back()s in the beginning can be replaced with std::iota in C++ 11
Perhaps it is better to combine the find and inc into a single next_combination which returns true if the combination has been updated, similar to the behaviour of std::next_permutation. The behaviour of combination_inc to require pos as an argument and modifying it through main() is strange to say the least.


Insert missing records in a list with LINQ We have a table with periods, and other child tables with these periods and values for combinations of product + location.

Based on selected periods, I would like to add any missing rows in the child tables. I have come up with the following code, which seems to work just fine. But I would like to know if there is a simpler or more efficient way of doing this, since I have to repeat this for a number of tables.

public static void SeedActivityData(int startPeriod, double endPeriod, IList<DAL.OMS_StockStatus> stockStatus)
{
    DAL.EFDbContext dbFilter = new DAL.EFDbContext(0, stockStatus.FirstOrDefault().Product, startPeriod, endPeriod);
    var period = dbFilter.Periods.Select(b => b.PeriodID);
    foreach (var item in stockStatus)
    {
        IEnumerable<int> missingFAF = period.Except(dbFilter.OMS_Forecast_Adjustment.Where(a => a.SiteID == item.SiteID).Select(b => b.PeriodID));

        var modelFAF = db.OMS_Forecast_Adjustment;
        foreach (int periodID in missingFAF)
        {
            var modelItem = new OMS_Forecast_Adjustment { PeriodID = periodID, Product = item.Product, SiteID = item.SiteID, Value = 0 };
            modelFAF.Add(modelItem);
        }

        IEnumerable<int> missingRAF = period.Except(dbFilter.OMS_Receipts_Adjustment.Where(a => a.SiteID == item.SiteID).Select(b => b.PeriodID));

        var modelRAF = db.OMS_Receipts_Adjustment;
        foreach (int periodID in missingRAF)
        {
            var modelItem = new OMS_Receipts_Adjustment { PeriodID = periodID, Product = item.Product, SiteID = item.SiteID, Value = 0 };
            modelRAF.Add(modelItem);
        }

        db.SaveChanges();
    }
}

Looks to me like you should create an Add method to both the OMS_Forecast_Adjustment and OMS_Receipts_Adjustment classes.

you are really adding things to the db all wrong,  you are creating new objects in your code when you don't have to, I think that you really want to add items to a collection variable inside your db object.

it should be an InsertRecord method

this:

    var modelFAF = db.OMS_Forecast_Adjustment;
    foreach (int periodID in missingFAF)
    {
        var modelItem = new OMS_Forecast_Adjustment { PeriodID = periodID, Product = item.Product, SiteID = item.SiteID, Value = 0 };
        modelFAF.Add(modelItem);
    }

Would turn into this

foreach (int periodID in missingFAF)
{
    db.Insert_OMS_Forcase_Adjustment(periodID, item.prodeuct, item.SiteID, 0);
}

so you actually do your insertion inside of a method in the class that you already have.

This removes the creation of 2 extraneous local variables and puts responsibility where it belongs inside of a method in the object that needs to perform the action.

This can be done on both Foreach Loops


Loop through range, selecting grouped data? I have some info. that looks like this:

Header  Debit   Credit

aaaa    20  0

bbbb    0   60

cccc    30  0

xxxx    10  0
xxxx    0   40
xxxx    40  0
xxxx    0   10

oooo    500 0
oooo    0   52
oooo    0   500

pppp    10
pppp    0   10

(Note this starts in col. H)

The idea is that I want to go through this range, and where there are "blocks" of data (see the "xxxx" group), check for offsetting amounts.

I was able to put this together, to loop through the groups, but I'm not sure if it's the best way to do so.  I'm afraid there will be a time it skips a line which is unintended - any ideas/tips on improving it?

Sub strike_out_Matching_Debit_and_Credits()
Dim debitCol&, credCol&, lastRow&, startRow&, endRow&, i&
Dim rng As Range, cel As Range
Dim myWS As Worksheet

Set myWS = Sheets("Raw - edited")
With myWS
    debitCol = .Rows(1).Find(what:="Debit").column
    credCol = .Rows(1).Find(what:="Credit").column
    lastRow = .Cells(.Rows.Count, credCol).End(xlUp).row

startRow = 2
    For i = 2 To lastRow
        endRow = .Cells(i, debitCol).End(xlDown).row
        Set rng = .Range(.Cells(startRow, debitCol), .Cells(endRow, debitCol))

        rng.Select

        If rng.Cells.Count - WorksheetFunction.CountA(rng) = 0 And rng.Cells.Count > 1 Then
            ' We now have a block to work through
            check_Values rng, rng.Offset(0, 1)
            check_for_Offset_Values rng, rng.Offset(0, 1)
        End If
        startRow = .Cells(endRow, debitCol).End(xlDown).row
    Next i

End With
End Sub

Thanks for any ideas/advice/tips!

Edit: I appreciate the help so far! I've made notes on the naming conventions, etc.  Does anyone have any tips for tightening up the actual function of the code loop? That's what I'm mainly interested in.

Following on your (shared) need of tightening up your code:

Get rid of unused variables

As per your code you don't use neither creditCol nor cel, so get rid of them

Avoid useless variables

You don't need myWS and can simply go

With Worksheets("Raw - edited")

where you also want to use Worksheets instead of Sheets since the former is the collection of sheets with actual data in cells while the latter is the collection that takes "chart" sheets too.

You don't need any lastRow, startRow, endRow and i variable.

Just loop through a "well" selected range using SpecialCells property of Range object and one of your already declared variable of Range type for the task.

Use Areas property of Range object

this will let you loop through the actual "blocks" (as you're naming them) of the parent range, i.e. the groups of contiguos cells that range is made of

Choose proper row counter column

As already pointed out in previous answers, from your data example "Credit " column has blank cells that may prevent you from picking the actual last one, while "Header" column seems to be the best candidate (and of which you already know column index,being it "H")

So choose this latter or loop through all relevant columns and calculate the maximum lastRow between them (make a Function for this task)

Proper use of Find method of Range object

It always "remembers" and uses the last user's choice of some options, were it made from Excel UI or any VBA code.

So you'd better always specify those options and ensure they're those you actually need

Find(What:="Debit", LookIn:= xlValues, LookAt:= xlWhole, MatchCase:= False)

Final result

All what above toghether results in the following code

Sub strike_out_Matching_Debit_and_Credits()
Dim debitCol As Long
Dim block As Range

With Worksheets("Raw - edited")
    DebitCol = .Rows(1).Find(What:="Debit", LookIn:= xlValues, LookAt:= xlWhole, MatchCase:= False)
    For Each block In .Columns("H").SpecialCells(xlCellTypeConstants, xlTextValues).Areas
        If block.Count > 1 Then
            check_Values block.Offset(, debitCol - 8), block.Offset(, debitCol - 7)
            check_for_Offset_Values block.Offset(, debitCol - 8), block.Offset(, debitCol - 7)
        End If
    Next block
End Sub

This also assumes that:

"Debit" is there for sure in some cell of row 1
column "H" has at least one cell with text value

were these assumptions unsafe then you'd add some "entry" checks, possibly wrapped in a specific function returning a boolean to tell your main Sub whether to proceed or  exit


Handling arrow keys for a console menu I have a relatively long method, and I'm wondering:

How readable is it?
Is a method this long okay, or will it be bad later on?

Note: This method enables users to select from a console menu - UpArrow and DownArrow change option, and LeftArrow and RightArrow change amount.

Here is the method (from here):

    public static List<Item> Spend(string[] text,List<Item> items,int currency,out int currencyLeft,string singular=null,string textZero=null){
        Initiate(items.Count);
        var maxNameLength=items.Max(n=>n.Name.Length);
        var sellIsEnabled=items.All(v=>v.Value>=0);
        var maxCurrencyLength=Math.Max(items.Aggregate(currency,(i,j)=>j.Value).Length()+text.Take(3).Aggregate(0,(i,j)=>i+j.Length)+
            Math.Max(0,(singular=singular??text[1].RemoveLast()).Length-text[1].Length),textZero?.Length??0);
        var line=Top+1;
        var totalItems=items.Select(i=>new Item(i)).ToList();
        while(true){
            switch(Input){
                case UpArrow:
                    Option--;
                    Index--;
                    break;
                case DownArrow:
                    Option++;
                    Index++;
                    break;
                case LeftArrow:
                    if(totalItems[Option].Amount-items[Option].Amount>0){
                        totalItems[Option].Amount--;
                        currency+=items[Option].Cost;
                    } else if(sellIsEnabled&&totalItems[Option].Amount>0){
                        totalItems[Option].Amount--;
                        currency+=items[Option].Value;
                    } else{
                        Input=ReadKey(true).Key;
                        continue;
                    }
                    break;
                case RightArrow:
                    if(currency-items[Option].Cost>=0){
                        totalItems[Option].Amount++;
                        currency-=items[Option].Cost;
                    } else if(sellIsEnabled&&currency-items[Option].Value>=0&&totalItems[Option].Amount-items[Option].Amount<0){
                        totalItems[Option].Amount++;
                        currency-=items[Option].Value;
                    } else{
                        Input=ReadKey(true).Key;
                        continue;
                    }
                    break;
                case Enter:
                    currencyLeft=currency;
                    return totalItems;
                default:
                    Input=ReadKey(true).Key;
                    continue;
            }
            if(Input==LeftArrow||Input==RightArrow){
                totalItems[Option].Amount.WriteAt(maxNameLength+3,line,maxLength:totalItems[Option].Amount.Length()+2);
                (currency!=0||textZero==null?text[0]+(currency==0?"no":currency.ToString())+(currency==1?singular:text[1])+text[2]:textZero).WriteAt(0,Top+MaxLines+2,maxCurrencyLength);
            }
            Maths.RMod(ref Option,TotalOptions);
            Select.TurnPage();
            if(ThePage.Was(Turned)){
                var maxAmountLength=totalItems.Max(t=>t.Amount.Length());
                var top=Top;
                foreach(var i in Indices){
                    items[i].Name.WriteAt(2,++top,maxNameLength);
                    totalItems[i].Amount.WriteAt(maxNameLength+3,top,maxAmountLength);
                }
                for(var i=Indices.Count;i<MaxLines;i++) "".WriteAt(2,++top,1+maxNameLength+maxAmountLength);
            }
            if(Input==UpArrow||Input==DownArrow){
                ' '.WriteAt(Left,line);
                '>'.WriteAt(Left,line=Index+Top+1);
            }
            Input=ReadKey(true).Key;
            PagePlus=0;
        }
    }

There's a lot about this method that makes it very difficult to follow... Let's start with the signature:

public List<Item> Spend(/* ... */)

So, based on the name and return type I'm going to "Spend" something(?) and get a list of items. To me, I find it had to believe this method is going to have a single responsibility straight away. The parameters only confirm the suspicion:

string[] text, List<Item> items, int currency, out int currencyLeft, string singular = null, string textZero = null

Wow. So I'm passing in some "text", a list of items, an amount of currency and optionally a "singular" string (no idea what that is) and a textZero string (again no idea) AND I'm getting an output of how much currency I have left. This is ringing alarm bells - why am I passing in text to a function that lets me spend money... Why is it an array? Why am I passing in items? At the very least I know this method is: showing info to the user, changing how much currency I have and probably changing what items I own. That's 3 separate responsibilities already and we haven't even got into the method yet.

Let's start on that:

var maxCurrencyLength=Math.Max(items.Aggregate(currency,(i,j)=>j.Value).Length()+text.Take(3).Aggregate(0,(i,j)=>i+j.Length)+
        Math.Max(0,(singular=singular??text[1].RemoveLast()).Length-text[1].Length),textZero?.Length??0);

That should be a method by itself! It's basically the most unreadable variable declaration I've ever seen (sorry if that sounds harsh).

Then there's this:

var totalItems=items.Select(i=>new Item(i)).ToList();

Okay, you're deep cloning the items (I would guess) - no indication of why and you've made me try to figure out what you're doing.

while (true) {
    switch (Input)

Most people writing C# prefer braces on new lines. What is Input? A property that gets input from the user?

I could go on but that's not very constructive. What you need to do, is go back to the drawing board and come up with some classes to encapsulate all the different things going on here.

public class Shop
{
    public IList<Item> GetAvailableItems()
    {
        // ..
    }

    public int GetSellItemPrice(Item item)
    {
        // ..
    }
}

You'll need other classes too. Like a Player with an ItemBag (for storing items) and a Wallet (for storing money). Potentially, Transaction objects to pass into Shops/Wallets to change the amount of money etc.

So to answer your question:  the method length is the least of your worries, you need to break the problem up into smaller pieces and make each piece responsible for a single thing.


Find perfect squares in range I've just taken the codility test on finding perfect squares in a range. I thought it was pretty straightforward, but on submitting, I got that a 50% on correctness and 66% on performance.

The spec also mentioned that range will be between [-2147483648 ... 2147483647], expected worst-case time complexity is O(sqrt(abs(B))) and expected worst-case space complexity is O(1).

This is my code:

Solution1.java

public class Solution1 {

    public int solution(int A, int B) {
        int upperLimit = (int) Math.sqrt(B);
        int squares = 0;
        for (int i = 1; i <= upperLimit; i++) {
            if (i * i >= A && i * i <= B) {
                squares++;
            }
        }
        return squares;
    }
}

Solution1Test.java

import org.junit.Test;

import static org.junit.Assert.assertEquals;

public class Solution1Test {

    private final Solution1 solution1 = new Solution1();

    @Test
    public void given4_17_return3() {
        assertEquals(3, solution1.solution(4, 17));
    }

    @Test
    public void given1_17_return4() {
        assertEquals(4, solution1.solution(1, 17));
    }

    @Test
    public void given1_35_return5() {
        assertEquals(5, solution1.solution(1, 35));
    }

    @Test
    public void given1_36_return6() {
        assertEquals(6, solution1.solution(1, 36));
    }

    @Test
    public void given9_49_return5() {
        assertEquals(5, solution1.solution(9, 49));
    }

    @Test
    public void given3000_3300_return3() {
        assertEquals(3, solution1.solution(3000, 3300));
    }
}

I'm specially concerned (and confused) about the 50% on correctness.

PS: it this is off-topic just let me know and I'll ask on stackoverflow.

The only reasons I can think of that your code was deemed incorrect are that it doesn't consider 0 as a perfect square, and that you don't consider the possibility that B < A (although maybe the test defined B as being greater than or equal to A and this is not a negligence of you). As for the performance, here are some suggestions:

You don't need to check i * i <= B in the loop, because the termination condition of the for loop already required that i <= upperLimit, and with i being positive, there is no way that i*i > B if i <= sqrt(B).
Taking the above into account, it follows that once the loop reaches an i for which i * i >= A, there is no point in continuing the loop, because all future values of i will have squares that fall within the specified range.
Continuing this trail of thought, it turns out that the solution can simply be expressed as (int) (Math.floor(Math.sqrt(B)) - Math.ceil(Math.sqrt(Math.max(A, 0)))) + 1, provided B is non-negative and B >= A. This expression doesn't really do anything your code doesn't do except floating point arithmetics. I don't know if two calls to Math.sqrt() (which delegates to StrictMath.sqrt(), which is a native method) are faster than using integer arithmetics in a loop, but it's definitely more to the point, coding-wise.