Redirecting to another page class Redirect {

    public function __construct() {
    }

    public function to($key, $page) {
        if(isset($page) && !empty($page)) {
            header('Location: ?' . $key . '=' . $page);
            exit();
        }
    }

}

With this class I can use this:

$foo->redirect('page', 'home');

instead of this:

header('Location: ?page=home);

Well, for starters, your redirect will never work if you pass in both $key and $page, as you're asking for the following:

1. is not isset($page)
2. is not empty($page)

Which means that the redirect will happen only when $page is NOT set and NOT empty. Because empty() calls isset() internally, you should only need this:

if(!empty($page))

Additionally, you're not managing your buffers. Ideally, you should use the ob_ functions as well as headers_sent() to check if headers have already been sent. If you don't manage your buffer, you're very likely to hit the infamous "Cannot modify header information - headers already sent" error. This post goes into further detail on how to fix that error, especially using the ob_ functions.

Aside from all that, your construct is doing nothing. If your Redirect class only has the to() method, perhaps you're better off making it a standalone function, or adding additional functionality.


Littlewood Polynomial Heatmap The following Python program generates heat maps of the roots of Littlewood polynomials. It works fine with a small number of roots, however I tried to use 2^21 in the first loop and it ate up 12 gigabytes of RAM on my computer. How can I make the code less memory-intensive while still allowing reasonably fast performance? Ideally, I would like to use this code to generate very large images but I cannot do so with the program in its current state. Any help would be greatly appreciated. Thanks!

import numpy as np
import matplotlib as mpl
import matplotlib.pyplot as plt
import matplotlib.cm as cm
from math import sqrt, sin, log10
def f(x): # function that maps [0,1] to itself for the color space so that values closer to 1 end up further from 1
    return sqrt(x)
def roots(n): # given n, returns a Littlewood polynomial with coefficients representing the binary representation of n
    roots = np.polynomial.polynomial.polyroots([int(i)*2-1 for i in bin(n)[2:]])
    return [[i.real,i.imag] for i in roots]
# get array of all possible roots and split values up into arrays x and y
temp=[]
for i in xrange(2**16):
    if i%100==0: print i
    for j in roots(i):
        temp.append(j)
x=[i[0] for i in temp]
y=[i[1] for i in temp]

# generate histogram of roots to make heatmap
gap=0.01
print len(np.arange(-2.5,2.5,gap))
normal_histogram = np.histogram2d(x,y,bins=np.arange(-2.5,2.5,gap),normed=True)

# generate image
from PIL import Image
normal_histogram = normal_histogram[0]
size=len(normal_histogram)
im = Image.new("RGB",(size,size))
im2 = Image.new("RGB",(size,size))
#color = [int(256*k) for k in cm.hot(histogram[i][j])[0:3]]
for i in range(len(normal_histogram)):
    print i
    for j in range(len(normal_histogram)):
        temp = int(normal_histogram[i][j]*256)
        im.putpixel((i,j),tuple([int(256*k) for k in cm.hot(f(normal_histogram[i][j]))][0:3]))
        im2.putpixel((i,j),tuple([int(256*k) for k in cm.nipy_spectral(f(normal_histogram[i][j]))][0:3]))
im.save("test.png")
im2.save("test2.png")

Things I've tried

using a txt file to store the polynomial roots
using matplotlib's ability to make heatmaps to plot the data

Here are a few changes that make image generation near instant, and slightly speed up root generation, and lower memory overhead.

def roots(n): # given n, returns a Littlewood polynomial with coefficients representing the binary representation of n
    return np.polynomial.polynomial.polyroots([int(i)*2-1 for i in bin(n)[2:]])

The only change here is to just return the roots rather than messing with them, so we pass less data around.

x, y = [], []
for i in range(0,2**16):
    if i%10000==0:
        print(i)
    root = roots(i)
    x.extend(np.real(root))
    y.extend(np.imag(root))

Here I removed temp to halve memory use, and used vectorization where applicable for small speed boosts.
the major changes are in the image generation

hot = np.asarray(128*cm.hot(normal_histogram)[:,:,0:3], dtype=np.uint8)
im = Image.fromarray(hot, mode="RGB")
spectral = np.asarray(128*cm.nipy_spectral(normal_histogram)[:,:,0:3], dtype=np.uint8)
im2 = Image.fromarray(spectral, mode="RGB")
im.show()
im2.show()

here, rather than making a blank image and filling it bit by bit, we make an image from an array created using vectorization. This takes 3 seconds with gap = 0.001 instead of over 16 times that before. This is far from optimized, vectorizing roots would help a lot, but that is hard.


Moving average in Rust I worked on implementing a simple moving average function in Rust, that works on a collection of year/revenue tuples:

fn compute_moving_average(window: &Box<&[(u32, u32)]>) -> (u32, f32) {
    let window_size = window.len();

    let current_year = window
        .iter()
        .nth((window_size as f32/ 2.0).floor() as usize)
        .unwrap().0;

    let sum = window
        .iter()
        .fold(0, |a, x| a + x.1) as f32 / window_size as f32;

    (current_year, sum)
}

fn extract_moving_average_for_year(year: u32, moving_average: &Vec<(u32, f32)>) -> Option<f32> {
    let x = moving_average
        .iter()
        .find(|a| a.0 == year);

    match x {
        Some(a) => Some(a.1),
        None => None,
    }
}

fn merge_moving_average(a: (u32, u32), avg: Option<f32>) -> (u32, u32, Option<f32>) {
    (a.0, a.1, avg)
}

fn main() {
    let vec = vec![
        (2003, 4),
        (2004, 6),
        (2005, 5),
        (2006, 8),
        (2007, 9),
        (2008, 5),
        (2009, 4),
        (2010, 3),
        (2011, 7),
        (2012, 8),
    ];

    let moving_average = vec
        .windows(5)
        .map(|a| compute_moving_average(&Box::new(a)))
        .collect::<Vec<_>>();

    vec
        .iter()
        .map(|a| merge_moving_average(*a, extract_moving_average_for_year(a.0, &moving_average)))
        .inspect(|a| println!("{:?}", a))
        .collect::<Vec<_>>();
}

Output:

(2003, 4, None)
(2004, 6, None)
(2005, 5, Some(6.4))
(2006, 8, Some(6.6))
(2007, 9, Some(6.2))
(2008, 5, Some(5.8))
(2009, 4, Some(5.6))
(2010, 3, Some(5.4))
(2011, 7, None)
(2012, 8, None)

I don't know why compute_moving_average takes a Box, much less why it would take a reference to one.
Prefer &[T] over &Vec<T>.
Why use floating point division with truncation when integer math does same, should be faster, and is shorter to write?
Don't use an iterator on a slice if you are just going to pull out the nth value; just index. Also bakes-in the unwrap.
I'd use Iterator::map and Iterator::sum.
Preserve the sum as an integer until the last moment, then convert it to floating point. This may or may not be a good idea depending on the input data.
Learn and love all the methods on Option and Result. For example, Option::map replaces the 4 lines of an explicit match.
merge_moving_average seems more trouble than it's worth; just inline it and provide better variable names.
There's no need to create a Vec; just pass in a slice of an array.
Can directly pass compute_moving_average with map, no closure or arguments needed.
It's more idiomatic to use |&a| in the closure argument instead of *a in the body when they are equivalent.
Iterator::inspect is more of a debugging tool than anything; use a for loop to drive an iterator for side effects.

fn compute_moving_average(window: &[(u32, u32)]) -> (u32, f32) {
    let window_size = window.len();

    let current_year = window[window_size / 2].0;

    let sum: u32 = window.iter().map(|&(_, val)| val).sum();
    let sum = sum as f32 / window_size as f32;

    (current_year, sum)
}

fn extract_moving_average_for_year(year: u32, moving_average: &[(u32, f32)]) -> Option<f32> {
    moving_average
        .iter()
        .find(|(yr, _)| yr == year)
        .map(|&(_, val)| val)
}

fn main() {
    let vec = [
        (2003, 4),
        (2004, 6),
        (2005, 5),
        (2006, 8),
        (2007, 9),
        (2008, 5),
        (2009, 4),
        (2010, 3),
        (2011, 7),
        (2012, 8),
    ];

    let moving_average = vec
        .windows(5)
        .map(compute_moving_average)
        .collect::<Vec<_>>();

    let moving_average = vec
        .iter()
        .map(|&(year, val)| (year, val, extract_moving_average_for_year(year, &moving_average)));

    for a in moving_average {
        println!("{:?}", a);
    }
}

Thoughts:

It would be interesting to see an implementation that doesn't use windows, but instead is based on an iterator.

If you know your slice is sorted, a binary search might be more efficient.


Flatten nested iterables I wrote a program sometime ago that reads csv files, sorts, reorders and groups their columns/rows' content (with itertools) and later labels the groups using a dictionary. The groups and dictionary are then flattened and rows of length N (columns) are uploaded to a spreadsheet. This is another function, not specific to that program, to use in this situation.

def digfrom(iterable, *, depth=None, strings=True):
    """
    Dig values from nested iterables, flattening
    the input iterable.

        >>> iterable = [['a'], 'bc', ('de', ['f'])]

        >>> list(digfrom(iterable))
        ['a', 'b', 'c', 'd', 'e', 'f']

    Depth may be limited and "exploding" strings
    is optional.

        >>> list(digfrom(iterable, depth=2))
        ['a', 'b', 'c']

        >>> list(digfrom(iterable, strings=False))
        ['a', 'bc', 'de', 'f']

    """

    exhausted = object()
    iterable_attr = '__iter__'
    iterator_attr = '__next__'

    iterators = [iter(iterable)]

    while iterators:
        it = next(iterators[-1], exhausted)

        if it is exhausted:
            iterators.pop()
            continue

        if hasattr(it, iterable_attr):
            string = isinstance(it, str)

            if not ((string and len(it) <= 1) or
                    (string and not strings)):
                it = iter(it)

        if hasattr(it, iterator_attr):
            iterators.append(it)
            iterators = iterators[:depth]
        else:
            yield it

Looks like decent code. I think you only got downvoted (once) because you alluded to how you wanted to use this function as part of a (gasp!) program — and then of course twice more because they saw you'd already been downvoted once. That's just how this site works. :)

The biggest problem with your code is that it uses the identifiers iterable, iterable_attr, iterator_attr, iterators, it, and iter, all with different meanings. That's too many its!

iterable_attr and iterator_attr could be called more like ITER_ATTR and NEXT_ATTR, because (A) the capitalization indicates that they're constants, and (B) the names indicate the values they hold. However, this is right up there with THREE = 3. When you see yourself giving symbolic names to global constants, it's time to DRY up your code:

    if hasattr(it, '__iter__'):
        string = isinstance(it, str)
        if not ((string and len(it) <= 1) or
                (string and not strings)):
            it = iter(it)
    if hasattr(it, '__next__'):
        iterators.append(it)
        iterators = iterators[:depth]

Actually, that logical expression buried in the middle there looks like it could stand some De Morgan's Laws. Also, string is a bad name for a boolean! Name your booleans after predicates: in this case, is_string would do. But let's see if we need it at all.

        is_string = isinstance(it, str)
        if not ((is_string and len(it) <= 1) or (is_string and not strings)):
            it = iter(it)

De Morgan 1:

        is_string = isinstance(it, str)
        if not(is_string and len(it) <= 1) and not(is_string and not strings):
            it = iter(it)

De Morgan 2:

        isnt_string = not isinstance(it, str)
        if (isnt_string or len(it) > 1) and (isnt_string or strings):
            it = iter(it)

Distributive law:

        isnt_string = not isinstance(it, str)
        if isnt_string or (strings and len(it) > 1):
            it = iter(it)

Refactor to emphasize the one special case we're trying to express:

        if (not hasattr(it, '__iter__')) or (len(it) <= 1):
            pass  # can't explode this guy
        elif (not strings) and isinstance(it, str):
            pass  # we're not exploding strings
        else:
            it = iter(it)  # explode it

The next two lines

        iterators.append(it)
        iterators = iterators[:depth]

look suspicious. We're appending to the array and then immediately truncating it? Couldn't we do that in some cleaner way?

By the way, that *, in the signature must be a Python-3-ism; I'm not sure what it means. In Python 2 you'd just omit it and it would still do what you wanted.

Your exhausted = object() ... if it is exhausted idiom is neat, if maybe a bit too cute. It beats catching the exception from next(iterators[-1]), anyway.

Unit tests (especially for depth=...) would go a long way here.


Getting essential computer info for network admins I recently had to install VNC on mass across a lot of computers on a domain and needed some info about their PC, for example:

Host name
Username
MAC Address
IPv4 Address
Domain name

Once all of this info is gathered, it is compiled into a text file and uploaded to a FTP server. To save files from being conflicted with the same name, I set each file to be named the name of the user.

This is the first ever script and first experience I have had with Powershell so please tell me if there is anything I can improve.

function Get-MACAddress {
    ipconfig /all | findstr "Physical" | Where-Object {$_.length -lt 58}
}
function Get-IPAddress {
    ipconfig | findstr "IPv4 Address"
}
function Get-HostName {
   get-content env:computername
}
function Get-UserName{
     get-content env:UserName
}
function Get-DomainName{
get-content env:USERDOMAIN
}
function CreateCSVdocument {
    Get-MACAddress | New-Item C:\Users\$($env:username)\Documents\$($env:username).txt -type file     -force
    Add-Content C:\Users\$($env:username)\Documents\$($env:username).txt " "

Get-IPAddress | Add-Content C:\Users\$($env:username)\Documents\$($env:username).txt
Add-Content C:\Users\$($env:username)\Documents\$($env:username).txt " "

Add-Content C:\Users\$($env:username)\Documents\$($env:username).txt "Hostname:"
Get-Hostname | Add-Content C:\Users\$($env:username)\Documents\$($env:username).txt
Add-Content C:\Users\$($env:username)\Documents\$($env:username).txt " "

Add-Content C:\Users\$($env:username)\Documents\$($env:username).txt "User:"
Get-UserName | Add-Content C:\Users\$($env:username)\Documents\$($env:username).txt
Add-Content C:\Users\$($env:username)\Documents\$($env:username).txt " "

Add-Content C:\Users\$($env:username)\Documents\$($env:username).txt "Domain:"
Get-DomainName | Add-Content C:\Users\$($env:username)\Documents\$($env:username).txt
}
function FTPUpload {
$Dir="C:\Users\$($env:username)\Documents\$($env:username).txt"

#ftp server
$ftp = "ftpserver"
$user = "username"
$pass = "password"

$webclient = New-Object System.Net.WebClient

$webclient.Credentials = New-Object System.Net.NetworkCredential($user,$pass)

#list every sql server trace file
foreach($item in (dir $Dir "$($env:username).txt")){
    "Uploading $item..."
    $uri = New-Object System.Uri($ftp+$item.Name)
    $webclient.UploadFile($uri, $item.FullName)
}

}
Get-DomainName
Get-IPAddress
Get-MACAddress
Get-HostName
Get-UserName
CreateCSVdocument
FTPUpload

It looks quite fine to me.

I'm mainly surprised by the massive duplication in the CreateCSVdocument function:

function CreateCSVdocument {
    Get-MACAddress | New-Item C:\Users\$($env:username)\Documents\$($env:username).txt -type file -force
    Add-Content C:\Users\$($env:username)\Documents\$($env:username).txt " "

Get-IPAddress | Add-Content C:\Users\$($env:username)\Documents\$($env:username).txt
Add-Content C:\Users\$($env:username)\Documents\$($env:username).txt " "

Add-Content C:\Users\$($env:username)\Documents\$($env:username).txt "Hostname:"
Get-Hostname | Add-Content C:\Users\$($env:username)\Documents\$($env:username).txt
Add-Content C:\Users\$($env:username)\Documents\$($env:username).txt " "

It would be better to introduce a variable there and use that, for example:

function CreateCSVdocument {
    $Dir="C:\Users\$($env:username)\Documents\$($env:username).txt"

    Get-MACAddress | New-Item $Dir -type file -force

    Add-Content $Dir " "
    Get-IPAddress | Add-Content $Dir
    Add-Content $Dir " "

    Add-Content $Dir "Hostname:"
    Get-Hostname | Add-Content $Dir
    Add-Content $Dir " "

Actually, this is still too repetitive.
Probably there is a better way to output all the text and pipe the whole thing to ... | Add-Content $Dir once at the end,
but I don't know how to do that in powershell.

The formatting is inconsistent:

In function definitions, sometimes you put a space between function name and { and sometimes you don't:

function Get-MACAddress {
function Get-UserName{

I recommend to put a space always, consistently.
Indentation is inconsistent:

You correctly indented the body of Get-MACAddress, Get-IPAddress and others
But you didn't follow the same logic in Get-HostName, Get-UserName, Get-DomainName and others


Advent of Code 2019: Day 6 (with Trees) I've been going through some 2019 AoC challenges and decided to solve Day 6 in Haskell with the help of Data.Tree.
In summary, the puzzle provides a list of orbits (edges) as input, resembling:
COM)B
B)C
C)D
D)E
E)F
B)G
G)H
D)I
E)J
J)K
K)L
K)YOU
I)SAN

where COM is supposedly the centre of all orbits (i.e. the root). We are tasked with parsing this and...

for Part 1: Find the total number of direct and indirect orbits. In the example,

B directly orbits COM
C directly orbits B (hence, indirectly orbiting COM)
D directly orbits C (hence, indirectly orbiting B and COM)
and so on...

for Part 2: Find the minimum number of orbital transfers. Basically, the number of traversals needed to get from the orbit of YOU to the orbit of SAN. In the example, the traversals are K -> J -> E -> D -> I. Hence, the minimum number of transfers is 4.

Here is my solution to both parts:
import Data.Tree

type Satellite = String
type STree = Tree Satellite
type Orbit = (Satellite, Satellite)

--  Part 1
main :: IO ()
main = interact $ show . countOrbits . fromOrbits . map parseOrbit . lines

--  Part 2
-- main :: IO ()
-- main = interact $ show . findMinimumTransfers "YOU" "SAN" . fromOrbits . map parseOrbit . lines

parseOrbit :: String -> Orbit
parseOrbit s = (takeWhile (/= ')') s, tail $ dropWhile (/= ')') s)

fromOrbits :: [Orbit] -> STree
fromOrbits orbits = construct "COM"
    where construct :: Satellite -> STree
          construct root = Node { rootLabel = root, subForest = map construct $ children root }

          children :: Satellite -> [Satellite]
          children sat = map snd $ filter ((== sat) . fst) orbits

countOrbits :: STree -> Integer
countOrbits = countOrbitsImpl 0
    where countOrbitsImpl :: Integer -> STree -> Integer
          countOrbitsImpl depth (Node rootLabel subForest)
                | length subForest == 0 = depth
                | otherwise = depth + (sum $ map (countOrbitsImpl (depth + 1)) subForest)

-- finds the minimum number of orbital transfers required between two targets
findMinimumTransfers :: Satellite -> Satellite -> STree -> Int
findMinimumTransfers tar tar' = findImpl 0
    where -- find the common node where targets are (possibly indirect) children
          findImpl :: Int -> STree -> Int
          findImpl depth (Node rootLabel subForest)
                | rootLabel == tar || rootLabel == tar' = depth - 1
                | length subForest == 0 = 0
                | otherwise =
                    let childResults = filter (/= 0) $ map (findImpl (depth + 1)) subForest
                    in  if length childResults == 2
                        then sum childResults - (depth * length childResults)   --  found common node
                        else sum childResults   --  propagate results

I'm itching for feedback on the recursion. I use it mainly to keep track of a node's depth and later return it as part of the result... but is there a "better" way to write this? Maybe with folds or applicatives?
I did think about keeping depth as part of a node's data (so that we might have type STree = Tree (Satellite, Int)), then maybe we could fold over that, but I didn't want to "bloat" the structure with redundant information.
Other feedback is also welcome. Thanks!
N.B. this is not a duplicate of AdventOfCode 2019 day 6 in Haskell as the implementation is different.

break and span
When we try to split a string in Haskell, we're a little bit out of luck if we only use the trusty Prelude and base. Handy functions like split or splitOn  are in the adaptly named split package, and parser combinators are completely other beasts and an oribtal (heh) laser cannon on this problem.
However, there are two functions that provide almost exactly what parseOrbit is trying to achieve: splitting a string on a single character:
span, break :: (a -> Bool) -> [a] -> ([a], [a])
span  f xs = (takeWhile f xs, dropWhile f xs)
break f xs = span (not . f)

We can simplify parseOrbit therefore to
parseOrbit :: String -> Orbit
parseOrbit s = let (a, _:b) = break (==')') s in (a, b)

However, you seem to prefer where, so let's use a where clause instead:
parseOrbit :: String -> Orbit
parseOrbit s = (a, b)
    where (a, _:b) = break (==')') s

Type signatures in where clauses
As we have seen above, (a, _:b) had no type signature. Type signatures in where clasues are usually omitted. There is some controversy about that, however there are some things to keep in mind:

GHC never warns about missing type signatures in where clauses
functions with type parameters cannot have a type without ScopedTypeSignatures (see this SO question for an example)
a change in the top level type signature might need a lot of changes in where clauses
if a function is complex enough to need a type, it might be reasonable to promote it into a top-level function. That way it can also be tested.

I personally therefore omit type signatures in where clauses (ST shenengians aside).


Using os.path.join in Python I want to reduce/shorten this part quite a lot. If you see that a method can be reduced or even erased please do not hesitate to suggest that. Any answer that rescue the code would be great.

(photos, identification, testers, id) = ([], [], {}, 0)

# Get the folders containing the training data
for (subdirectorys, directorys, files) in os.walk(dir):

    # Loop through each folder named after the subject in the photos
    for subdirectory in directorys:
        testers[id] = subdirectory
        subpath = os.path.join(dir, subdirectory)

        # Loop through each photo in the folder
        for file_name in os.listdir(subpath):

            # Skip non-image formates
            f_name, f_extension = os.path.splitext(file_name)
            if(f_extension.lower() not in
                    ['.png','.jpg','.jpeg','.gif','.pgm']):
                print("Skipping "+file_name+", wrong type given for the file")
                continue
            path = subpath + '/' + file_name
            lable = id

            # Add to training data
            photos.append(cv2.imread(path, 0))
            identification.append(int(lable))
        id += 1
(im_width, im_height) = (112, 92)

# Create a Numpy array from the two lists above
(photos, identification) = [numpy.array(lis) for lis in [photos, identification]]

Some notes:

(photos, identification, testers, id) = ([], [], {}, 0)

may become

photos, identification, testers, id = [], [], {}, 0

as parentheses are superfluous (and even it's recommended not use them).

for (subdirectorys, directorys, files) in os.walk(dir):

may become

for __, directorys, __ in os.walk(dir):

as the names subdirectorys and files are never used (not two consecutive underline symbols - __ - for ignoring their values).

Instead of maintaining the value of id manually (initializing it to 0 and then increase it by id += 1) you may use Pythonic way with help of enumerate() function - the very first line of you code will totally omit id:

photos, identification, testers = [], [], {}

and your for loop

    for subdirectory in directorys:

becomes

    for id, subdirectory in enumerate(directorys):

Two notes about your if statement

        if(f_extension.lower() not in
                ['.png','.jpg','.jpeg','.gif','.pgm']):

Python is not C/C++/Java, so outermost parentheses are superfluous.
Your may prepare your list of extensions in advance

    EXTENSIONS = '.png .jpg .jpeg .gif .pgm'.split()       # split() creates a list

(nice method for saving some typing, isn't it?), and then use it:

    if f_extension.lower() not in EXTENSIONS:

(note the use of uppercase letters for a constant list).


An entire file size retriever using a recursive os function in python I'm a beginner and i just started with automate the boring stuff with python course and reached his first os lesson, so i decided to make a recursive directory size finder since apparently os.path.getsize doesn't get the size of folders.
Need a way to improve my code or advice on coding tricks, to make it more efficient

import os
class FileSizeFinder:

    def calculate_directory_size_in_bytes(self, directory: str, totalsize=0):
        for filename in tuple(os.listdir(directory)):
            current_directory = os.path.join(directory, filename)
            if os.path.isdir(current_directory):
                totalsize += FileSizeFinder.calculate_directory_size_in_bytes("self", current_directory)
            if not os.path.isfile(current_directory):
                continue
            totalsize += os.path.getsize(current_directory)
        return totalsize

    def get_file_size(self, directory: str, totalsize=0):
        total_size_in_bytes = FileSizeFinder.calculate_directory_size_in_bytes("self", directory)
        if total_size_in_bytes > 1024000000:
            totalsize = "Size in GB: " + str(round(total_size_in_bytes / 1024000000, 3))
        elif total_size_in_bytes > 1024000:
            totalsize = "Size in MB: " + str(round(total_size_in_bytes/ 1024000, 3))
        elif total_size_in_bytes > 1024:
            totalsize = "Size in KB: " + str(round(total_size_in_bytes / 1024, 3))
        return totalsize

print(FileSizeFinder.get_file_size("self", r"E:\Utilities_and_Apps\Python"))

If there's no reason to make it a class, don't make it a class.  (A good clue is that your class doesn't have any instance variables, and that both of its methods could be static without changing the way they work.)
Both of your functions return the same information (formatted differently), but one calls it file_size and the other calls it directory_size even though it's the same number.  In addition, neither has a type annotation for the return value.  This all adds up to make it hard to figure out what each function does.  Maybe it'd be better to just have one function that gets the size and a seperate one that formats it as a string?
The totalsize parameter does not seem to serve any purpose as a parameter, and should be removed.
There's no reason to convert os.listdir into a tuple; you can iterate over it as-is.
current_directory is not necessarily a directory, so this name is confusing.
Your size formatting logic has an obvious gap in its if...elif chain.  Adding an else case fixes that.
This is a style thing, but I think f-strings are nicer looking than explicitly converting to str and using concatenation.
Your GB/MB/KB definitions are both copy+pasted and wrong.  Maybe this whole if...elif construction could be done more cleanly by turning the unit definitions into a table (where each unit size is defined exactly once) and iterating over the table?
Your recursive function would actually be simpler if it accepted any arbitrary path.  That way you wouldn't have to do as much logic inside the body of your iteration over the os.listdir results, and could actually just do it as a simple sum of recursive calls.
It's easier to test your code if you write a __main__ block that accepts a command line parameter!

Here's my massaged version of your code (does basically the same thing, but it uses the units as I've defined them rather than your hybrid 1024 * 10^N units -- having them defined the way I did it here makes it very easy to change if you'd rather make them 2^10, 2^20, etc).

import os
import sys

def recursive_get_size(path: str) -> int:
    """Gets size in bytes of the given path, recursing into directories."""
    if os.path.isfile(path):
        return os.path.getsize(path)
    if not os.path.isdir(path):
        return 0
    return sum(
        recursive_get_size(os.path.join(path, name))
        for name in os.listdir(path)
    )

def format_size(num_bytes: int) -> str:
    """Formats a size (given in bytes) into a human-readable string."""
    for unit_name, unit_size in [
        ("GB", 10**9),
        ("MB", 10**6),
        ("KB", 10**3),
    ]:
        if num_bytes > unit_size:
            return f"Size in {unit_name}: {round(num_bytes / unit_size, 3)}"
    else:
        return f"Size in bytes: {num_bytes}"

if __name__ == '__main__':
    print(format_size(recursive_get_size(
        sys.argv[1] if len(sys.argv) > 1 else r"E:\Utilities_and_Apps\Python"
    )))


Check for the presence of multiple files Given the following code, I have written a for loop that returns a key and value (0 or 1) of file names passed. 1 means present, and 0 means not present.

Is there a way to construct a flat map without having to use into?

(defn kind-stat
  [filename expected-type]
  (let [f (File. filename)
        s (cond
              (.isFile f)      "f"
              (.isDirectory f) "d"
              (.exists f)      "o"
              :else            "n" )]

      (if (= expected-type s)
        0
        1)))

(defn look-for
  [filename expected-type]
    (let [find-status (kind-stat filename expected-type)]
      find-status))

(defn all-files-present?
"Takes a list of real file names, and returns a sequence of maps
 indicating the file name and its availability status 1 for present
 and 0 for not present. Please note look-for returns 0 for success,
 so its return logic needs to be reversed for the return sequence
 of maps."
  [file-seq]
  (for [fnam file-seq
    :let [stat-map {(keyword fnam)
       (if (= (look-for fnam "f") 0)
           1
           0)}]]
   stat-map))

(into {}
 (all-files-present? '("Makefile" "build.sh" "real-estate.csv")))

{:Makefile 1, :build.sh 1, :real-estate.csv 0}

You could use zipmap:

(defn all-files-present?
  [file-seq]
  (let [f  #(bit-flip (look-for % "f") 0)]
    (zipmap (map keyword file-seq) (map f file-seq))))

or juxt:

(defn all-files-present?
  [file-seq]
  (let [f  #(bit-flip (look-for % "f") 0)]
    (into {} (map (juxt keyword f) file-seq))))

Note that I use bit-flip instead of the if to flip 1<->0.

It would be nicer if kind-stat would just return a boolen value, so you would simply use

(defn kind-stat
  [filename expected-type]
  (let [f (File. filename)]
      (= expected-type
        (cond
          (.isFile f)       "f"
          (.isDirectory f)  "d"
          (.exists f)       "o"
           :else            "n"))))

(defn all-files-present?
  [file-seq]
  (let [f  #(if (kind-stat % "f") 1 0)]
    (into {} (map (juxt keyword f) file-seq))))

I removed look-for, since it is virtually useless.


VBA Excel function to interpolate precipitation data I wrote this code to interpolate precipitation data. When I run this code, it works very well, but the problem is that it slows my computer down so I have to wait 5 mins to interpolate the data.

Is there any way to make this function work faster?

Function IDWW(Value1, Value2, Value3, Dist1,Dist2,Dist3)
    Dim a1 As Variant
    Dim b1 As Variant
    Dim a2 As Variant
    Dim b2 As Variant
    Dim a3 As Variant
    Dim b3 As Variant

    If Value1 <> "" And Dist1 <> "" Then
        a1 = Value1 / (Dist1) ^ 2
        b1 = 1 / (Dist1) ^ 2
    Else
        a1 = 0
        b1 = 0
    End If

    If Value2 <> "" And Dist2 <> "" Then
        a2 = Value2 / (Dist2) ^ 2
        b2 = 1 / (Dist2) ^ 2
    Else
        a2 = 0
        b2 = 0
    End If

    If Value3 <> "" And Dist3 <> "" Then
        a3 = Value3 / (Dist3) ^ 2
        b3 = 1 / (Dist3) ^ 2
    Else
        a3 = 0
        b3 = 0
    End If

    'Avoid a problem if all 3 distances are empty
    If b1 + b2 + b3 = 0 Then
        IDWW = 0
    Else
        IDWW = (a1+a2+a3) / (b1+b2+b3)
    End If
End Function

Naming

Your variable's names are pretty generic. Tell me what they are - use the name to give information - the characters are free.

The same goes for the function name - IDWW. I'm not sure what it should be doing.

I imagine you'd be good with names like

firstValue, secondValue, thirdValue, firstDistance, secondDistance, thirdDistance

I don't know what those are because they aren't given a Type. Your function also doesn't have a return Type. That means you're taking in 6 variants and returning a variant. That has impact.

Variant

When you don't define or type your variable, VBA will declare it as a Variant, which are objects:

  Performance. A variable you declare with the Object type is flexible
  enough to contain a reference to any object. However, when you invoke
  a method or property on such a variable, you always incur late binding
  (at run time). To force early binding (at compile time) and better
  performance, declare the variable with a specific class name, or cast
  it to the specific data type.

Total you are working with 12 Variants and a return Variant.

You also have 6 variables made of 1 letter and 1 number. The code is already pretty abstract, but seeing something like

For i = k To j Step -1
    For n = 1 To 26
        If k * n > j * n Then
            n = i * j
        Else
        n = i * k
        End If
    Next
Next

Gives me no confidence that I'll be able to decipher what's happening.

You also test like this twice -

If Value2 <> "" And Dist2 <> "" Then
If Value3 <> "" And Dist2 <> "" Then

It seems like these should be optional arguments in the function, otherwise you're forcing the user to submit nulls.

Private Function IDWW(ByVal firstValue As Long, ByVal firstDistance As Long, _
        Optional ByVal secondValue As Long, Optional ByVal secondDistance As Long, _
        Optional ByVal thirdValue As Long, Optional ByVal thirdDistance As Long) As Long

You can also give them default values. If you don't give them defaults, they choose the default value of their Type.

Optional ByVal secondValue As Long = 0, Optional ByVal secondDistance As Long = 0,

You were testing for "" which has a name = vbNullString. But what you should have been testing for is the equivalent for a Variant

If IsEmpty(var) then

Something else you aren't testing for is whether the user passes something else in the Variant, like a Range. Or maybe a negative number. These are all things that can be eliminated with the right Type.

Like here -

If Value2 <> "" And Dist2 <> "" Then
    a2 = Value2 / (Dist2) ^ 2
    b2 = 1 / (Dist2) ^ 2
Else
    a2 = 0
    b2 = 0
End If

If a2 and a3 were Long you wouldn't need to set them, they are by default 0.

At the end

'Avoid a problem if all 3 distances are empty
 If b1 + b2 + b3 = 0 Then
    IDWW = 0

Is that really what you're doing? Adding variants to get 0. But what if I submitted a negative distance? What if I submitted both a negative value and distance?


Reading properties from file during standalone application startup I've a standalone Java application, which basically starts and manages two socket servers. I'd like to configure server ports in a .properties file using the following class.

class ApplicationConfig {

    private static final Logger     LOG            = Logger.getLogger(ApplicationConfig.class.getName());
    private static final Properties APP_PROPERTIES = new Properties();

    static int defaultEventServerPort  = 9090;
    static int defaultClientServerPort = 9099;

    static {
        try {
            APP_PROPERTIES.load(ClassLoader.class.getResourceAsStream("/app.properties"));
            String eventServerPort = APP_PROPERTIES.getProperty("server.event.port");
            String clientServerPort = APP_PROPERTIES.getProperty("server.client.port");

            if (isValidNumeric(eventServerPort)) {
                defaultEventServerPort = Integer.valueOf(eventServerPort);
            }
            if (isValidNumeric(clientServerPort)) {
                defaultClientServerPort = Integer.valueOf(clientServerPort);
            }
        } catch (IOException ex) {
            LOG.log(
                    Level.WARNING,
                    "Unable to load server ports from properties file, going to use default port {0} for event server and {1} for client server",
                    new Object[]{defaultEventServerPort, defaultClientServerPort}
            );
        }
    }

   private static boolean isValidNumeric(String v) {
        if (v == null || v.length() == 0) {
            return false;
        }
        for (int i = 0; i < v.length(); i++) {
           if (!Character.isDigit(v.charAt(i))) {
              return false;
           }
        }
        return true;
   }
}

I really hate ApplicationConfig class, the static initialization block bothers me, but I am not able to find a better idea yet. How you would suggest to modify it?

And here is my main class

public class Application {

private static final Logger LOG = Logger.getLogger(Application.class.getName());

    private Application() {}

    public static void main(String[] args) {
        try {
            ExecutorService pool = Executors.newCachedThreadPool();

            EventServer eventServer = new EventServer(new Configuration(ApplicationConfig.defaultEventServerPort));
            ClientServer clientServer = new ClientServer(new Configuration(ApplicationConfig.defaultClientServerPort));

            pool.submit(eventServer);
            pool.submit(clientServer);
        } catch (IOException e) {
            LOG.log(Level.SEVERE, "Unable to start servers", e);
        }
    }
}

What I find bothering is not the ApplicationConfig class and the static initializer, in itself. It's the fact that defaultEventServerPort and defaultClientServerPort are global variables, which should be avoided.

What you should have are two constants defining the default values, 2 variables holding the actual values as instance fields.

private static final int DEFAULT_EVENT_SERVER_PORT = 9090;
private static final int DEFAULT_CLIENT_SERVER_PORT = 9099;

private int eventServerPort;
private int clientServerPort;

and the initialization of those 2 variables done in the constructor. The purpose of doing this in the constructor, instead of a static initializer, is that it lets you have proper instance fields.

The first concern is that there's duplicated code in this: the logic for extracting the port from the Properties object, converting it to an int or using the default value, will be the same for both ports. Therefore, it makes sense to create a method for that:

private static int getAsIntOrDefault(Properties properties, String key, int defaultValue) {
    String val = properties.getProperty(key);
    if (isValidNumeric(val)) {
        return Integer.parseInt(val);
    }
    return defaultValue;
}

Notice that instead of using Integer.valueOf, which returns an Integer object, we can directly use Integer.parseInt, which returns a primitive int. This way, we don't need to have a boxing, and then unboxing conversion.

There is another problem with how the Properties object is loaded: you have a potential memory leak!

APP_PROPERTIES.load(ClassLoader.class.getResourceAsStream("/app.properties"));

This is opening an InputStream, but it is never closed. Instead, use a try-with-resources construct:

try (InputStream is = ClassLoader.class.getResourceAsStream("/app.properties")){
    APP_PROPERTIES.load(is);
} catch (IOException ex) {
    // ...
}

With those changes, you can have the following:

ApplicationConfig() {
    try (InputStream is = ClassLoader.class.getResourceAsStream("/app.properties")){
        APP_PROPERTIES.load(is);
    } catch (IOException ex) {
         // do the logging
    }
    eventServerPort = getAsIntOrDefault(APP_PROPERTIES, "server.event.port", DEFAULT_EVENT_SERVER_PORT);
    clientServerPort = getAsIntOrDefault(APP_PROPERTIES, "server.client.port", DEFAULT_CLIENT_SERVER_PORT);
}

The try-catch block was also reduced so that it spans the minimal amount of code as possible. try-catch should be small and cover only the part of the code that can actually throw the exception you want to catch.

Last point, you can see the advantage of using a constructor here: later, you may want to remove hard-coding "/app.properties". With a constructor, you can easily now pass the path to the properties file, which would be complicated to do with a static initializer.

Nitpick: isValidNumeric won't return true for negative numbers, so you may want to rename it to isValidPositiveInteger. Also, does APP_PROPERTIES really need to be static final as well? I would imagine it is only used to fetch all of the configuration values once, and unused later on.


Checking for neighbours more elegantly in Conway's Game of Life My method for counting neighbors in my soon-to-be Game of Life implementation is very repetitive and I was wondering if this could be done more elegantly:

static int countNeighbours(Board b, int x, int y) {
    int count = 0;
    if (b.getTile(x - 1, y - 1)) {
        ++count;
    }
    if (b.getTile(x, y - 1)) {
        ++count;
    }
    if (b.getTile(x + 1, y - 1)) {
        ++count;
    }
    if (b.getTile(x + 1, y)) {
        ++count;
    }
    if (b.getTile(x + 1, y + 1)) {
        ++count;
    }
    if (b.getTile(x, y + 1)) {
        ++count;
    }
    if (b.getTile(x - 1, y + 1)) {
        ++count;
    }
    if (b.getTile(x - 1, y)) {
        ++count;
    }
    return count;
}

Because officially the board should be infinite, I don't need out-of-bounds checks in this code - I assume the board is infinite here -, but my Board implementation secretly looks like this:

// TODO: make the board 'infinite'
public boolean getTile(int x, int y) {
    if (x < 0 || y < 0 || x > getWidth() - 1 || y > getHeight() -1) return false;
    return tiles[y][x];
}

public void setTile(int x, int y, boolean val) {
    if (x < 0 || y < 0 || x > getWidth() - 1 || y > getHeight() -1) return;
    tiles[y][x] = val;
}

(Just to save you the time it costs to type 'your code will cause an IndexOutOfBoundsException for the tile on (0,0)'. This question is on the implementation of static int countNeighbours(Board, int, int).)

I'm stuck with Java 6 by the way.

You're right, there's an alternate way to do this, but, first, some Java standards:

1-liners should have braces. This is a code-style that is common to many langauges because it is more maintainable, and leads to fewer future bugs. Lines like:

if (x < 0 || y < 0 || x > getWidth() - 1 || y > getHeight() -1) return;

should be:

if (x < 0 || y < 0 || x > getWidth() - 1 || y > getHeight() -1) {
    return;
}

Don't use arithmetic when you can use a simpler operator. Back to:

if (x < 0 || y < 0 || x > getWidth() - 1 || y > getHeight() -1) { ... }

Which can be simplified to:

if (x < 0 || y < 0 || x >= getWidth() || y >= getHeight()) { ...}

Use plain booleans when they make sense (again, back to...):

if (x < 0 || y < 0 || x >= getWidth() || y >= getHeight()) { ...}

which can be put in a function like (transformed to uses && instead):

public boolean getTile(int x, int y) {
    return x >= 0 && y >= 0 && x < getWidth() && y < getHeight() && tile[y][x];
}

Note, how the standard boolean short-circuit evaluation ensures that the x and y are valid before the tile[y][x] is used.

OK, about that code duplication. A trick with these sorts of problems is to use an array of offsets. Consider your grid, which has 8 neighbours (in a relative y,x format):

-1,-1     -1,0    -1,+1

 0,-1     *us*     0,+1

+1,-1     +1,0    +1,+1

We can put these neighbour offsets in to an array:

private static final int[][] NEIGHBOURS = {
    {-1, -1}, {-1, 0}, {-1, +1},
    { 0, -1},          { 0, +1},
    {+1, -1}, {+1, 0}, {+1, +1}};

Then, your check method becomes:

static int countNeighbours(Board b, int x, int y) {
    int cnt = 0;
    for (int[] offset : NEIGHBOURS) {
        if (b.getTile(x + offset[1], y + offset[0]) {
           cnt++;
        }
    }
    return cnt;
 }


Selecting form controls that are either inside or outside a rectangle I have a function which loops over the controls on a form and checks to see if it's in a given rect, and then adds it or doesn't add it to a list<>.  The parameter bContained is used to determine if we want a list of "in the rect" or "not in the rect" controls.

List<Control> getControls(Control container, Rectangle rect, bool bContained = false)
{
    List<Control> Selected = new List<Control>();

    rect = RectangleToClient(rect);

    foreach (Control ctl in container.Controls)
    {
        //Do we want selected or non-selected controls
        if (bContained)
        {
            if (rect.IntersectsWith(ctl.Bounds)) Selected.Add(ctl);
            foreach (Control ct in ctl.Controls) Selected.Add(ct);
        }
        else
        {
            if (!rect.IntersectsWith(ctl.Bounds)) Selected.Add(ctl);
            foreach (Control ct in ctl.Controls) Selected.Add(ct);
        }
    }

    return Selected;
}

You can make things more concise with LINQ.  Here's one way that should work:

List<Control> getControls(Control container, Rectangle rect, bool bContained = false)
{
    rect = RectangleToClient(rect);
    return (from Control ctl in container.Controls
                  where rect.IntersectsWith(ctl.Bounds) == bContained
                  select new List<Control> { ctl }.Union(ctl.Controls.OfType<Control>()))
                  .SelectMany(x => x).ToList();
}

This basically does the same thing, filters the collection according to bContained.  Creates a collection of  lists that each contain the parent control and its sub-controls.  Then flattens them into one list.


Flipping True or False values to save typing and time I have a few columns in a DBGrid that I want to hide and show at different times depending on what the user wants to view.

Rather than writing the following code;

For i := 1 to DBGrid1.Columns.Count -1 do
  Begin

    If DBGrid1.Columns[i].FieldName = 'SOMETHING' then
      Begin
        if DBGrid1.Columns[i].visible then
        DBGrid1.Columns[i].visible:=false else
        DBGrid1.Columns[i].visbile:=true;
      End;

    If DBGrid1.Columns[i].FieldName = 'ANOTHERFIELD' then
      Begin
        if DBGrid1.Columns[i].visible then
        DBGrid1.Columns[i].visible:=false else
        DBGrid1.Columns[i].visbile:=true;
      End;

    { And so on... for another 8 columns }

  End;
End;

I've got the following function;

Function Flip(CurrentState:Boolean):Boolean;
Begin

  If CurrentState then
     Result:=False else
     Result:=True;

End;

Then I use the function above like this;

For i := 1 to DBGrid1.Columns.Count -1 do
  Begin

    If DBGrid1.Columns[i].FieldName = 'SOMETHING' then
    DBGrid1.Columns[i].visible := Flip(DBGrid1.Columns[Loop].visible;

    If DBGrid1.Columns[i].FieldName = 'ANOTHERFIELD' then
    DBGrid1.Columns[i].visible := Flip(DBGrid1.Columns[Loop].visible;

    { And so on... for another 8 columns }

  End;
End;

I can't address columns by their index directly because they move around, therefore I use the Field Name associated with it.

My question is this, is there are better and smarter way to hide and unhide many columns?

I'd replace the if-then-elses with DBGrid1.Columns[i].Visible := not DBGrid1.Columns[i].Visible - i.e. use the not keyword instead of your Flip function.

And you probably can store references to the relevant columns in an array, TList or similar. Then you don't need to loop all columns and check their names.


Splitting a string into words or double-quoted substrings For various reasons, I'm parsing a string, this code will explain what I'm after:

string baseString = "This is a \"Very Long Test\"";

string[] strings = baseString.Split(' ');

List<String> stringList = new List<string>();
string temp = String.Empty;
foreach (var s in strings)
{
    if (!String.IsNullOrWhiteSpace(temp))
    {
        if (s.EndsWith("\""))
        {
            string item = temp + " " + s;
            stringList.Add(item.Substring(1,item.Length - 2));
            temp = string.Empty;
        }
        temp = temp + " " + s;
    }
    else if (s.StartsWith("\""))
    {
        temp = s;
    }
    else
    {
        stringList.Add(s);
    }

}

stringList.ForEach(Console.WriteLine);

The output should be:

This
is
a
Very Long Test

Basically, given a string, it will split it on spaces, unless its grouped into speech marks, the same way the command line does it.

Any better way to do this code?

Seems like a job for a regular expression:

string baseString = "This is a \"Very Long Test\"";
var re = new Regex("(?<=\")[^\"]*(?=\")|[^\" ]+");
var strings = re.Matches(baseString).Cast<Match>().Select(m => m.Value).ToArray();

What the regular expression (?<=")[^"]*(?=")|[^" ]+ does is that it either finds a sequence of zero or more characters that are not " ([^"]*) preceded by a " ((?<=")) and followed by a " ((?=")) or a sequence of one or more character that are not " or a space ([^" ]+).

For the sample input, it gives the same output as your version. The code itself is much simpler, but the regular expression might be hard to understand, especially if you're not used to them.


Method for adding error messages to a dictionary given a key I want this method to be completely understandable just from looking at the code and comments only.

def add_error(error_dict, key, err):
    """Given an error message, or a list of error messages, this method
    adds it/them to a dictionary of errors.

    Doctests:
        >>> add_error({}, 'key1', 'error1')
        {'key1': ['error1']}
        >>> add_error({'key1': ['error1']}, 'key1', 'error2')
        {'key1': ['error1', 'error2']}
        >>> add_error({'key1': ['error1', 'error2']}, 'key2', 'error1')
        {'key1': ['error1', 'error2'], 'key2': ['error1']}
        >>> add_error({}, 'key1', ['error1', 'error2'])
        {'key1': ['error1', 'error2']}
        >>> add_error({}, 'key1', [])
        {}
        >>> add_error({'key1': ['error1']}, 'key2', ['error1', 'error2'])
        {'key1': ['error1'], 'key2': ['error1', 'error2']}
        >>> add_error({}, 'key1', 23) # doctest: +IGNORE_EXCEPTION_DETAIL
        Traceback (most recent call last):
            ...
        TypeError: The error(s) must be a string, or a list of strings.
        >>> add_error({}, 'key1', [23]) # doctest: +IGNORE_EXCEPTION_DETAIL
        Traceback (most recent call last):
            ...
        TypeError: The error(s) must be a string, or a list of strings.
        >>> add_error({}, 'key1', ['error1', 23]) # doctest: \
        +IGNORE_EXCEPTION_DETAIL
        Traceback (most recent call last):
            ...
        TypeError: The error(s) must be a string, or a list of strings.
    """
    if not isinstance(err, list):
        err = [err]

    if not key in error_dict and len(err) > 0:
        error_dict[key] = []

    for e in err:
        if not isinstance(e, string_types):
            raise TypeError(
                'The error(s) must be a string, or a list of strings.'
            )
        error_dict[key].append(e)
    return error_dict

Hopefully, the code along with the comment does the job well, but I would still appreciate review(s) of this method. One thing I always keep on wondering is whether this is too many doc-tests for such a simple method. Thanks.

Consider narrowing accepted types

This might not be possible based on the context of your code, but if it is: arguments sharing one of many different types hinders and complicates testability and maintainability. There are many different solutions to this that will help this situation; one is accepting variadic arguments - i.e.

def add_error(error_dict, key, *errs):

This is still easily invocable without needing to wrap a single error in a list.

Use x not in instead of not x in

i.e.

if key not in error_dict

Lose your loop

and also lose your empty-list assignment, instead doing

error_dict.setdefault(key, []).extend(err)

If you use the variadic suggestion above, your entire function becomes that one line.

Immutable or not?

Currently you do two things - alter a dictionary and return it - when you should only pick one. Either make a copy of the dict and return an altered version, or modify the dict and don't return anything.


Test if a key exists anywhere in a nested object I have complex objects containing nested objects. I need to check if a key exists anywhere in the nested object. I wrote this code as a makeshift solution. Is there any other way to search for the key?

function checkKey(obj, key) {
    if(key in obj){
        return obj[key];
    }
    else{
        for (k in obj){
            t = obj[k]
            if(typeof t === "object" && !Array.isArray(t) && t !== null){
                return checkKey(t, key);
            }
        }
    }
    return false
}

Test object:

obj = {
    "l1":"test333",
    "some":{
        "l2":"test2",
        "l22":"test3",
        "other":{
            "l3":["yellow"]
        }
    }
}

Search: checkKey(obj, "l3"); returns ["yellow"]

Your code has a couple of problems:

Semantic

You are using the construction key in obj, without checking if key is an actual property of obj. If obj is not an Object, but instead something resembling a class:

var PolarBear = function() {
  this.cuddly = true;
}

PolarBear.prototype.growl = function() {
  //nothing
}

Passing an instance of PolarBear to your function will return a function reference whenever you check if "growl" is a key. You could change this behaviour by checking if obj.isOwnProperty( key ) is true, or checking if obj is an Object.

Logic errors

Early return

If a key is an Array, and that array does not contain your key, your code will return anyway, even though you have not checked every key yet.

An example would be key "c" in the following Object. It is there, but your code would return false instead of 2, because the Object under key "a" does not contain a key "c".

{
  "a" : {
    "b" : 1
  }
  "c" : 2
}

Not exact key

Your code does not check if the key requested is the actual key we get. Some keys are transformed to an actual key. For example:

{
  "" : 1,
  "null" : 2,
  "false" : 3
}

Obviously, "" in obj would return true, but so would [] in obj, or null in obj, or false in obj, even though strictly speaking there is no key with that name. You could instead do Object.keys( obj ).indexOf( key ) > -1.

Not returning key of Object under Array

You are silently ignoring Arrays. I suspect you do this because you do not want to return keys in Arrays, but this also means that if you have an Object inside your Array, you'll silently ignore this. For example, the key "c" in the following Object will be ignored, and thus false will be returned

{
  "a" : [
    { "b" : 1 },
    { "c" : 2 }
  ]
}


Applying TrimSideSpaces() method to multiple string properties I am looking for generic ideas and guidelines of how to improve the style of my coding and make it more readable and robust.

public class TemplatePart : ITemplatePart
{
    #region Fields

    private readonly List<ITemplatePart> _options = new List<ITemplatePart>();
    private readonly List<ITemplatePart> _cables = new List<ITemplatePart>();

    #endregion // Fields

    #region Constructor

    #endregion // Constructor

    #region Creator

    public static TemplatePart CreateTemplatePart(string partName, string basePart)
    {
        return new TemplatePart
        {
            PartName = partName,
            BasePart = basePart
        };
    }

    public static TemplatePart CreateTemplatePart(DataTable table, DataRow row, int rowIndex)
    {
        return new TemplatePart
        {
            // as you can see, TrimSideSpaces() get's applied for every single content
            // being read from the sheet for each property.
            Key = rowIndex,
            PartName = TrimSideSpaces(row.Field<string>(Strings.TemplateSpreadSheet_Column_PartName)),
            BasePart = TrimSideSpaces(row.Field<string>(Strings.TemplateSpreadSheet_Column_BasePart)),
            Category = TrimSideSpaces(ExtractVendor(table.TableName))

        };
    }

    private static string TrimSideSpaces(string p)
    {
        var rgx = new Regex(@"\S.*\S");
        return rgx.Match(p).Value;
    }

    #endregion // Creator

    #region Public Properties

    public int Key { get; set; }
    public string PartName { get; set; }
    public string BasePart { get; set; }
    // This captures the tab the item is in.
    public string Category { get; set; }
    public double? PriceDom { get; set; }
    public double? PriceInt { get; set; }
    public IEnumerable<ITemplatePart> Options { get { return this._options; } }
    public IEnumerable<ITemplatePart> Cables { get { return this._cables; } }

    #endregion // Public Properties

    #region Private Helpers
    ....
    #endregion // Private Helpers
}

I have a similar concrete class with about 20 properties and I am thinking it might not be a good choice to apply methods on each property such as TrimSideSpaces() in this example.

I apologize if this is not a whole code which could compile.

single letter variables shouldn't be used except for loop variables
shortening variables names removes readability of the code
if you don't need List<T> specific methods consider to change List<ITemplatePart> _options to IList<ITemplatePart>. You should if possible code against interfaces instead of implementations.
instead of calling the TrimSideSpaces() method, which in reality does only return an empty string, you should just use the built in Trim() method of the String class which removes any leading and trailing space

Refactoring

By using the overloaded CreateTemplatePart() method

public static TemplatePart CreateTemplatePart(string partName, string basePart)
{
    return new TemplatePart
    {
        PartName = partName.Trim(),
        BasePart = basePart.Trim()
    };
}

public static TemplatePart CreateTemplatePart(DataTable table, DataRow row, int rowIndex)
{
    TemplatePart templatePart = CreateTemplatePart(row.Field<string>(Strings.TemplateSpreadSheet_Column_PartName),
              row.Field<string>(Strings.TemplateSpreadSheet_Column_BasePart));
    templatePart.Key = rowIndex;
    templatePart.Category = ExtractVendor(table.TableName).Trim();
    return templatePart;
}

Update based on the comments

  In regards to your 4th point, TrimSideSPaces() returns the content of the property without it's leading and trailing spaces. "which in reality does only return an empty string" how?

Using the method in your code the first test fails while the second one passes.

[TestMethod()]
public void TrimSideSpacesTest_WillFail()
{
    SnippetsCodeReview target = new SnippetsCodeReview();
    string value = "     g     ";
    string expected = "g";
    string actual = target.TrimSideSpaces(value);
    Assert.AreEqual(expected, actual);
}
[TestMethod()]
public void TrimSideSpacesTest_WillPass()
{
    SnippetsCodeReview target = new SnippetsCodeReview();
    string value = "     g     ";
    string expected = String.Empty;
    string actual = target.TrimSideSpaces(value);
    Assert.AreEqual(expected, actual);
}

  The suggested overloaded CreateTemplatePart() improves the code. how?

Improvement could be the less repetitions because we use the overloaded CreateTemplatePart() method in which we call the Trim().
Also, this "improvement" comes obviously with a lack of readability. This is partly because the Strings constants have some really long names.

An improved refactoring after renaming the overloaded methods to Create.

public static TemplatePart Create(DataTable table, DataRow row, int rowIndex)
{
    TemplatePart templatePart = Create(row);

    templatePart.Key = rowIndex;
    templatePart.Category = ExtractVendor(table.TableName).Trim();

    return templatePart;
}

private static TemplatePart Create(DataRow row)
{
    String partName = row.Field<string>(Strings.TemplateSpreadSheet_Column_PartName);
    String basePart = row.Field<string>(Strings.TemplateSpreadSheet_Column_BasePart);

    return Create(partName, basePart);
}

public static TemplatePart Create(string partName, string basePart)
{
    return new TemplatePart
    {
        PartName = partName.Trim(),
        BasePart = basePart.Trim()
    };
}


Leap year check in Java (functional style) I have done the leap year check on https://exercism.io already in a lot of languages. Today I came back to the exercise in Java and was playing around with some maybe more funny ways to do the check. The following more functional one kept me thinking of:

import java.util.stream.IntStream;

class Leap {

    boolean isLeapYear(final int year) {
        return IntStream.of(4, 100, 400)
            .filter(divisor -> year % divisor == 0)
            .count() % 2 == 1;
    }

}

What do you think? Is this readable? At least it seems to be extensible in case more rules will ever get added ;)

Another funny fact: due to prime factorization we actually could also use the IntStream.of(2, 25, 16) (but for sure that doesn't help in readability).

What do you think?

It certainly is clever. That is not always a good thing, because it might be harder to understand what is going on (you have to be at least as clever)

Is this readable?

Not really. I can't get from a glance that 100 is treated differently from 400 and why. Also there is no room for comments close to you code.

If you make a set of ifs you could do something like:

if (year % 400 ... ) // if 400 then a leap year
{..
..}

In your IntStream you could do something like:

   IntStream.of(  4,    //yes
                100,    //no
                400     //yes
               )

But then you kinda miss the 'one-liner' point of your solution.

Is it efficient?

Not per se. Most efficient would be to first check divisibility by 400, if true, return true, etc.

Prime factorization funny fact

This in incorrect; because it would label 1998 as leap year. Or do you mean something different?


Looking up the name and event for a ticket This code is from my Kohana project. How can I make it more beautiful? Should I use try-catch instead? How could I avoid writing exit() twice?

public function action_index()
{
    $id = $this->request->query('id');
    if (!empty($id)) {
        $ticket = ORM::factory('ticket')
            ->where('id', '=', $id)
            ->find();
        if (!empty($ticket)) {
            $event = ORM::factory('event')
                ->where('id', '=', $ticket->event_id)
                ->find();
            if (!empty($event)) {
                $this->template->ticket = $ticket->id;
                $this->template->name = $ticket->name;
                $this->template->event = $event->title;
            } else {
                exit();
            }
        } else {
            exit();
        }
    }
}

You can avoid writing exit() twice by adding a return statement at the deepest level, after passing all the checks. This should be equivalent to your code:

public function action_index()
{
    $id = $this->request->query('id');
    if (!empty($id)) {
        $ticket = ORM::factory('ticket')
            ->where('id', '=', $id)
            ->find();
        if (!empty($ticket)) {
            $event = ORM::factory('event')
                ->where('id', '=', $ticket->event_id)
                ->find();
            if (!empty($event)) {
                $this->template->ticket = $ticket->id;
                $this->template->name = $ticket->name;
                $this->template->event = $event->title;
                return;
            }
        }
        exit();
    }
}

That said, I find it a bit strange to exit() in the middle of a function.
It's an abrupt behavior in a program in general,
this kind of control is usually done at higher levels,
in order to do any necessary cleanup, such as releasing resources,
closing connections.


Frontmost window of the current application that passes a predicate check After resigning active state (going to the background), the mainWindow property of the main window in the app returns false.

Nevertheless, I need a to find that window even when my app is not active, so I came up with the following code, which works:

public let App = NSApplication.sharedApplication() // just some sugar

public extension NSApplication {

    public func frontmostWindow(_ accept: (NSWindow -> Bool)? = nil) -> NSWindow? {
        for windowNumber in NSWindow.windowNumbersWithOptions(0) as? [Int] ?? [] {
            if let window = windowWithWindowNumber(windowNumber) {
                if let accept = accept {
                    if accept(window) {
                        return window
                    }
                } else {
                    return window
                }
            }
        }
        return nil
    }
}

With this, I can now get the first window of my app in the current ordering that passes the provided predicate check (accept: NSWindow -> Bool). This is because I normally would not be interested in the menu bar or the status item, which all have associated windows that belong to my app (which is what I would get if I do not pass any predicate). As an example, say I have some:

class PimpedUpWindow: NSWindow, ExtraTranslucent {}

the frontmost instance of which I wish to find after the app launches and is no longer the active app. Using the extension above I can:

App.frontmostWindow { $0 is PimpedUpWindow }

or

App.frontmostWindow { $0 is ExtraTranslucent }

Of course, I can provide any other predicate of type NSWindow -> Bool, but I figure I will often enough want to just check the type or protocol conformance. So I might as well add two convenience methods for just those occasions:

public extension NSApplication {

    public func frontmost <T> (windowOfType: T.Type) -> T? {
        return frontmostWindow { $0 is T } as? T
    }

    public func frontmostWindow <P> (conformingTo: P.Type) -> NSWindow? {
        return frontmostWindow { $0 is P }
    }
}

Now, I can simply:

App.frontmost(PimpedUpWindow)

Which actually returns a PimpedUpWindow?, and:

App.frontmostWindow(ExtraTranslucent)

which returns an NSWindow?.

All ideas, criticisms, and comments very welcome!

There is a way of improving the code as desired in that it is possible to constrain the type T to T: NSWindow. Upon introducing this change, however, the function will indeed return the first NSWindow in the current ordering, whether or not it is of subtype T (too check this you must construct a test that can actually fail, i.e. by having an NSWindow of a another type above the window you are looking for - e.g. by adding a status item to your app). There is a workaround, however, as described in Optional binding succeeds if it shouldn't. Apart from the solution I stumbled upon in the question (of not constraining T), there is a much preferable option discovered by @vacawama in the answer to the linked question. The workaround is to generalise the variable to Any before type checking, e.g. as follows:

public func frontmost <T: NSWindow> (windowOfType: T.Type) -> T? {
    return frontmostWindow { $0 as Any is T } as? T
}

This improves the api considerably.


Bulk Email notifications in Java? I have written a service which uses the Java Mail Api to send bulk email notifications to the recipients. I feel that the way I implemented the solution is not really scalable when the number of recipients increases. I would like to know if there is a better way to make my solution more efficient.

This is what I tried so far

@Component
public class EmailService {

     public String  sendEmail(String [] to,String from, String subject, String msg) throws MessagingException {

         String status ="099";
         final  String password ="********";

           final String fromsender = from;
            Properties props = new Properties();
            props.setProperty("mail.transport.protocol", "smtp");
            props.setProperty("mail.host", "smtp.gmail.com");
            props.put("mail.smtp.auth", "true");
            props.put("mail.smtp.port", "465");
            props.put("mail.debug", "true");
            props.put("mail.smtp.socketFactory.port", "465");
            props.put("mail.smtp.socketFactory.class","javax.net.ssl.SSLSocketFactory");
            props.put("mail.smtp.socketFactory.fallback", "false");
            Session session = Session.getDefaultInstance(props,
            new javax.mail.Authenticator() {
               protected PasswordAuthentication getPasswordAuthentication() {
               return new PasswordAuthentication(fromsender,password);
           }
           });

           //session.setDebug(true);
           Transport transport;
        try {
            transport = session.getTransport();
             InternetAddress addressFrom = new InternetAddress(fromsender);

               MimeMessage message = new MimeMessage(session);
               message.setSender(addressFrom);
               message.setSubject(subject);
               message.setContent(msg, "text/plain");
               //message.addRecipient(Message.RecipientType.TO, new InternetAddress(to));
               for(int i =0;i<to.length;i++){
                   message.addRecipient(Message.RecipientType.TO, new InternetAddress(to[i]));

               }

               transport.connect();
               Transport.send(message);
               status ="000";
               transport.close();
        } catch (NoSuchProviderException e) {
            // TODO Auto-generated catch block

            e.printStackTrace();
        }

        return status;
        }

Indentation

Please please please please automatically indent your code. You're doing all readers (and yourself) a disservice by not following indentation conventions and enforcing them through your IDE.

Abstraction

This code can be much cleaner by simply extracting responsibilities into methods, especially those that encapsulate setup. Let's get the code into a final state as follows:

public String sendEmail(String[] to, String from, String subject, String msg) throws MessagingException {
    String status = "099";
    final Session session = buildMailSession(from, password);
    final MimeMessage message = buildMailMessage(session, from, to, subject, msg);
    final Transport transport;
    try {
        transport = session.getTransport();
        transport.connect();
        Transport.send(message);
        status = "000";
    } catch (NoSuchProviderException e) {
        e.printStackTrace();
    } finally {
        if (transport != null) {
            transport.close();
        }
    }
    return status;
}

This is signifcantly easier to comprehend and understand. It even makes clear, what status is. You should very much consider extracting those into named constants.

This is a good moment to also talk about documentation.

When extracting methods, that is a huge opportunity to include some "free" documentation through the method name. In case that is not sufficient, one should employ *javadoc" to clarify what the method does exactly:

/**
 * Sends an email to all the recipients in <tt>to</tt>
 *
 * @return {@link #SUCCESS_STATUS} if sending was successful,
 *     {@link FAIL_STATUS} otherwise
 */
public String sendEmail(..);

Signature changes

Instead of returning magic Strings, it would be much cleaner to return an enum value like so:

public SendResult sendEmail(...) {
    SendResult status = SendResult.SEND_FAIL;
    // ...
         status = SendResult.SEND_SUCCESS;
    // ...
    return status;
}

Now if we use the fact that I introduced a finally block, which is executed regardless of how the corresponding try-block is left we can simplify this to:

public SendResult sendEmail(...) {
    // ...
        Transport.send(message);
        return SendResult.SEND_SUCCESS;
    } catch (..) {
    // ..
    return SendResult.SEND_FAIL;
}

In addition to that you should consider changing the signature to the following:

public SendResult sendEmail(String msg, String subject, String from, String... to)

This makes use of "varargs" to allow invoking it as follows:

emailService.sendEmail(message, subject, from, "recipient@example.com", "recipient2@example.com");

Whether that's more useful... How would I know?


Prime Number Sequence generator I have just started using Python, and I am attempting to make a prime number sequence generator, where it will print the a specified amount prime numbers in terminal.  I have other versions of this also, and versions for the Fibonacci sequence, although, I will post just the version for prime numbers I specified above.

    P = 2
    Count = 1
    X = int(raw_input('choose number: '))

    def Main(P, X):
        while Count <= X:
            isprime = True
            for x in range(2, P - 1):
                if P % x == 0:
                    isprime = False
            if isprime:
                print P
                Count += 1
            P += 1

I am currently trying to optimize this code so that it runs as fast as possible, and then making small edits.  I did this by putting a get daytime now function at the beginning and the end of the sequence generator, and printing the time, then looping it all a specified amount of times.

from datetime import datetime as dt

P = 2
Count = 1
Count2 = 1
X = int(raw_input('choose number: '))

def Main(P, Count, X):
    t1 = dt.now()
    while Count <= X:
        isprime = True
        for x in range(2, P - 1):
            if P % x == 0:
                isprime = False
        if isprime:
            Count += 1
        P += 1
    t2 = dt.now()
    print ((t2-t1).microseconds)

while Count2 <= 20:
    Main(P, Count, X)
    Count2 += 1

As far as my knowledge of Python extends, I have optimized this code so it runs quite efficiently.  However, I want to know if anyone can help make this code any better, however, it needs to stay within one function (two if necessary), and it does the same type of thing.  Also, if it is possible to explain why it does better, that would be appreciated.  However, any comment or feedback would be nice.

A logical optimization here would be to remember the primes you have already calculated. Consider the theory:

  A prime is a number that is divisible by itself and 1 only

It follows that, to test if any number X is prime, you only need to find 1 prime number less than X which divides in to X without a remainder. There is no need to test non-prime numbers, because if a non-prime divides cleanly in to X, then the prime factors would also divide in to X.

So, if you keep a record of the previously calculated primes, then you only need to scan those values to see if they divide in to X. In essence, each time you print a prime, also add it to a list.

This will require 'seeding' the prime list with the value 2.

A second optimization is that a number X is only prime if it has a factor. A factor is a number, multiplied by another number, that is equal to the original value X.

The useful theory here, is that, as the value of the first factor increases, the value of the second factor decreases. There is a point when the first and second factors 'cross over' and the second factor becomes less than the first.

The cross-over point is the square-root of the number X. When you pass the square-root of the number, you have tested all the possible factors... there's no need to scan values larger than the root, because, if they were factors, you would have found them already by identifying the small factor that matches the larger factor.

Putting these two items together, you should modify your code to:

Have a special case for 2, which is prime, and store it in the seed array.
preserve the prime numbers you find in the same array.
for values larger than 2, you only need to use values from the pre-identified prime array to test for factors
you only need to look for prime factors that are less than, or equal to the square-root of the value.


Project Euler #10 in Ruby. Three ways to sum primes below 2 million Question:

  Add all prime numbers smaller than 2,000,000.

The first method is done by deriving own is_prime? method:

# First Method
def is_prime(n)
    if n <= 3
        return n > 1
    elsif (n % 2 == 0 || n % 3 == 0)
        return false
    else
        (5..Math.sqrt(n).ceil).step(6) do |i|
            if (n % i == 0 || n % (i+2) == 0)
                return false
            end
        end
        return true
    end
end

start_time = Time.now
range = 2000000
array_of_primes = []

(1..range).each do |n|
    if is_prime(n)
        array_of_primes << n
    end
end

answer = array_of_primes.inject(:+)
duration = Time.now - start_time

puts "Sum of primes below #{range} is #{answer}. Took #{duration} s to calculate using my first method."

Second Method is using Ruby's Prime.is_prime?(n) method:

# Second Method
require 'prime'
array_of_primes = []
start_time = Time.now
(1..range).each do |n|
    if Prime.prime?(n)
        array_of_primes << n
    end
end

answer = array_of_primes.inject(:+)
duration = Time.now - start_time

puts "Sum of primes below #{range} is #{answer}. Took #{duration} s to calculate using my Ruby's Prime.prime? method."

Third method is the simplest form - by using Ruby's Prime.each(2000000) method

# Third Method - Prime.each
start_time = Time.now
answer = Prime.each(range).inject(:+)
duration = Time.now - start_time

puts "Sum of primes below #{range} is #{answer}. Took #{duration} s to calculate using my Ruby's Prime.each method."

All methods gave the correct answer, but the interesting findings is they have drastically different performance, as shown below are the outputs of duration for the computation from each method.

Sum of primes below 2000000 is 142913828922. Took 3.694159 s to calculate using my first method.
Sum of primes below 2000000 is 142913828922. Took 39.370195 s to calculate using my Ruby's Prime.prime? method.
Sum of primes below 2000000 is 142913828922. Took 0.300214 s to calculate using my Ruby's Prime.each method.

As seen above, Best performing is using Prime.each method. I wonder if it already stores a list of prime numbers therefore it doesn't have to compute that long?

Comparing using my own is_prime method and Prime class' Prime.is_prime?(n) method. The Ruby method is taking suprisingly long time to compute the answer. >30s compare to ~3s. Any thoughts on this observation?

You should use implicit looping (filtering):

def primes_below(limit)
    (1..limit).select{|i| Prime.prime?(i)}
end

You should use the built-in profiler:

require 'benchmark'
puts Benchmark.measure{primes_below(limit).inject(:+)}


Modeling a Triangle, with a utility method to classify it Inspired by this question, I played a bit with the idea of modeling a Triangle, and a utility method to classify it:

class Triangle {

    private final int a;
    private final int b;
    private final int c;

    enum Type {
        EQUILATERAL, INVALID, ISOSCELES, SCALENE
    }

    public Triangle(int a, int b, int c) {
        if (!isSaneArguments(a, b, c)) {
            throw new IllegalArgumentException("All sides of a Triangle must be > 0");
        }
        if (!isTriangle(a, b, c)) {
            throw new IllegalArgumentException("Not a triangle: no side must be longer than the sum of the other sides");
        }
        this.a = a;
        this.b = b;
        this.c = c;
    }

    private static boolean isSaneArguments(int a, int b, int c) {
        return a > 0 && b > 0 && c > 0;
    }

    private static boolean isTriangle(int a, int b, int c) {
        return a < b + c && b < a + c && c < a + b;
    }

    public static Type classifyValidTriangle(int a, int b, int c) {
        final Type type;
        if (a == b && b == c) {
            type = Type.EQUILATERAL;
        } else if (b == c || a == b || c == a) {
            type = Type.ISOSCELES;
        } else {
            type = Type.SCALENE;
        }
        return type;
    }

    public static Type classify(int a, int b, int c) {
        if (!isSaneArguments(a, b, c) || !isTriangle(a, b, c)) {
            return Type.INVALID;
        }
        return classifyValidTriangle(a, b, c);
    }

    public Type classify() {
        return classifyValidTriangle(a, b, c);
    }
}

Unit tests:

public class TriangleTest {
    @Test(expected = IllegalArgumentException.class)
    public void testInvalidTriangleWithTooLongSide() {
        int a = 1;
        int b = a + 1;
        new Triangle(a, b, a + b + 1);
    }

    @Test(expected = IllegalArgumentException.class)
    public void testInvalidTriangleWithNegativeSide() {
        new Triangle(1, 2, 0);
    }

    @Test
    public void testScalene() {
        assertEquals(Triangle.Type.SCALENE, new Triangle(2, 3, 4).classify());
        assertEquals(Triangle.Type.SCALENE, Triangle.classify(2, 3, 4));
    }

    @Test
    public void testEquilateral() {
        assertEquals(Triangle.Type.EQUILATERAL, new Triangle(3, 3, 3).classify());
        assertEquals(Triangle.Type.EQUILATERAL, new Triangle(2, 2, 2).classify());
        assertEquals(Triangle.Type.EQUILATERAL, Triangle.classify(5, 5, 5));
    }

    @Test
    public void testIsosceles() {
        assertEquals(Triangle.Type.ISOSCELES, new Triangle(3, 3, 2).classify());
        assertEquals(Triangle.Type.ISOSCELES, new Triangle(3, 2, 3).classify());
        assertEquals(Triangle.Type.ISOSCELES, new Triangle(2, 3, 3).classify());
        assertEquals(Triangle.Type.ISOSCELES, Triangle.classify(2, 3, 3));
        assertEquals(Triangle.Type.ISOSCELES, Triangle.classify(3, 3, 2));
        assertEquals(Triangle.Type.ISOSCELES, Triangle.classify(3, 2, 3));
    }
}

What do you think? What would you do differently? How can it be better?

You need to pick a model, and stick with it. You have duplicated much of your logic at the static level and the instance level. Specific cases are the classify() instance method, and the classify(int,int,int) static method. There is no need for both.

Additionally, the logic split between the two validation methods isSaneArguments and isTriangle should all be in one method (I recommend isTriangle). There is no place you call them separately, so there is no reason to split them. isSaneArguments is a poor name anyway.... Finally, it's not exactly 'yoda conditions', but I prefer to do calculations on the left-side of a comparison operator like < or >. Bracing them makes it more readable in a chain of arguments...:

isTriangle(final int a, final int b, final int c) {
    // positive lengths, and sides that can meet
    return a > 0 && b > 0 && c > 0  && (a + b) > c && (a + c) > b && (b + c) > a;
}

The way your class is laid out lends itself to having static calls to get the properties of the triangles. It appears that the class is designed to regularly encounter invalid triangles, (hence Type.INVALID), so having a constructor throw an exception is unpleasant. I would make the constructor private, and do the validation outside the constructor, and would keep the methods static...

Note, classifyValidTriangle is public, but does no validation, (like the name says), this is odd.... to trust the caller that much. I would still expect validation of arguments, and an IllegalArgumentException if they are not met.

The classify method would also be better if written as an early-return mechanism, instead of holding the type variable....

Note that the actual Triangle instance does not do much, and there is no apparent purpose for it.

Neither your class, nor the embedded enum are public, this seems odd.

Finally, I rearranged the code to put the enum at the top, the static methods next, then the fields, and constructor:

public class Triangle {

    public enum Type {
        EQUILATERAL, INVALID, ISOSCELES, SCALENE
    }

    public static Triangle buildTriangle(int a, int b, int c) {
        if (isTriangle(a, b, c)) {
            return new Triangle(a, b, c);
        }
        throw new IllegalArgumentException("input values are not able to make a triangle");
    }

    public static Type classify(int a, int b, int c) {
        if (!isTriangle(a, b, c)) {
            return Type.INVALID;
        }
        if (a == b && b == c) {
            return Type.EQUILATERAL;
        }
        if (b == c || a == b || c == a) {
            return Type.ISOSCELES;
        }
        return Type.SCALENE;
    }

    private static boolean isTriangle(int a, int b, int c) {
        // positive lengths, and sides that can meet
        return a > 0 && b > 0 && c > 0  && (a + b) > c && (a + c) > b && (b + c) > a;
    }

    private final int a;
    private final int b;
    private final int c;

    private Triangle(int a, int b, int c) {
        this.a = a;
        this.b = b;
        this.c = c;
    }

}


Python Pig Latin Translator This is only my second Python script, so let me have it!  If anything seems off in style or implementation, please nit pick away.  I want to learn.  I know it could've been done in less lines of code without making separate functions and whatnot, but I wanted to approach it like I would an actual project.

#!/bin/usr/python
from sys import exit

def print_help():
    print("")
    print("--------------------------------")
    print(" Command   |  Description")
    print("--------------------------------")
    print(" -h        |  Print help list")
    print(" -x        |  Exit program")
    print("--------------------------------")
    print("")

def is_valid_word(word):
    return word and word.isalpha()

def is_valid_string(string):
    for word in string.split(" "):
        if not is_valid_word(word):
            return False
    return True

def get_pig_latin(string):
    vowels = "aeiou"
    built = ""
    for word in string.split(" "):
        if len(word) < 2 or word[0] in vowels:
            built += word + "tay"
        else:
            temp = word
            while temp[0] not in (vowels + "y"):
                temp = temp[1:] + temp[0]
            built += temp + "ay"
        built += " "
    built = built[0].upper() + built[1:]
    return built

def print_title():
    print("")
    print("--------------------------------")
    print("----- PIG LATIN TRANSLATOR -----")
    print("--------------------------------")
    print("Type -h for help")
    print("")

def run():
    commands = { "-h" : print_help,
                 "-x" : exit }
    print_title()
    while True:
        input = raw_input(":: ").strip().lower()
        if input in commands:
            commands[input]()
        elif is_valid_string(input):
            print("-> " + get_pig_latin(input) + "\n")
        else:
            print("   Unrecognized word or command.\n")

if __name__ == "__main__":
    run()

Not essential but I would find it neater to make a function that translates a single word, then join those together. You should also deal with the case of a word containing no vowels or 'y', which currently gets your programme stuck in an infinite loop.

def get_pig_latin(string):
    return ' '.join(map(translate_word, string.split(' '))).upper()

def translate_word(word):
    vowels = "aeiou"
    if len(word) < 2 or word[0] in vowels:
        return word + 'tay'
    elif contains_any(word, vowels + 'y'):
        while word[0] not in (vowels + 'y'):
            word = word[1:] + word[0]
        return word + 'ay'
    else:
        return '???'

Then add docstrings to your functions.

EDIT: It might be better to use a for loop instead...

def translate_word(word):
    if len(word) < 2 or word[0] in 'aeiou':
        return word + 'tay'
    for i, ch in enumerate(word):
        if ch in 'aeiouy':
            return word[i:] + word[:i] + 'ay'
    return '???'

You could also consider building the string validation into your translation function with a user-defined exception:

class TranslationError(Exception):
    def __init__(self, value):
        self.value = value

def translate(word):
    if word.isalpha():
        if len(word) < 2 or word[0] in 'aeiou':
            return word + 'tay'
        for i, ch in enumerate(word):
            if ch in 'aeiouy':
                return word[i:] + word[:i] + 'ay'
    # if there is no aeiouy or word is not alphabetic, raise an error
    raise TranslationError(word)

def run():
    commands = { "-h" : print_help,
                 "-x" : exit }
    print_title()
    while True:
        inp = raw_input(":: ").strip().lower()
        if inp in commands:
            commands[inp]()
        else:
            try:
                translation = ' '.join(map(translate, inp.split(' '))).upper()
                print("-> " + translation + "\n")
            except TranslationError as e:
                print("   Unable to translate:" + e.value + ".\n")


Random imgur image loader Just a little thing I made to load 20 random images from imgur. I looked at the way that imgur references images on its site, and I felt like I could probably generate a random string of letters and numbers that would, on occasion, produce a valid image URL. So I threw this together in PHP, because I am trying to learn PHP. It takes a while to get the 20 that it does, way longer for more. I would like to speed it up, and my current project in PHP is to learn more about classes etc, but I really have no idea where to start.

I would love some feedback! I know this looks real amateur hour, but I am a real amateur, so go easy on me!

BEWARE: not everything on imgur is worksafe, so if you decide to try this code out on your own server, the images returned are truly random with no filter, so no telling what you might see.

    <title>Random imgur Loader</title>

<?php
$gcode1=generateCode(5);
$url3="http://i.imgur.com/".$gcode1.".jpg";
?>

    <img src="<?=$url3?>" alt="">

<?php
$pagepath=$_SERVER["PHP_SELF"];
if ($_GET['numimg']=='') {
    $numimg=20;
} else {
    $numimg=$_GET['numimg'];
}
?>

<?=$numimg?> random imgur images

<?php

function generateCode($length=6) {
    $source='abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890';
    $code='';

    for ($i=0; $i<$length; $i++) {
        $code .= $source[(rand() % strlen($source))];
    }

return $code;
}

$ii=1;
while ($ii<=$numimg):
    $gcode=generateCode(5);
    $url="http://i.imgur.com/".$gcode.".jpg";
    $url2="http://www.imgur.com/".$gcode;
    $headerfile=get_headers($url2, 1);
    $http_code=$headerfile[1];
    $imgheader=get_headers($url, 1);
    $imgcode=$imgheader["Content-Type"];
    #echo $imgcode;

    if ($imgcode == 'image/gif') {
        $bcode='red';
    } else {
        $bcode='gray';
    }

    if($http_code!="HTTP/1.1 404 Not Found") {
        print("");
        echo(str_repeat(' ',4096));
        if ($ii % 5 == 0) {
            print("

");
        }
        flush();
        $ii++;
    }
endwhile;
?>

<?php print("
Get $numimg more!
");?>

Definitely separate your PHP from HTML as Alex suggested. You probably don't need to go the full MVC route for something so simple, but simply generating your PHP variables then outputing your HTML would make your code a lot more readable/manageable.

I like your idea of generating a random string and checking it, but Flambino's right, it will never be reliable (by design) - also, imgur probably hates you ;) A simpler approach would be to consume imgur's RSS feed: http://feeds.feedburner.com/ImgurGallery?format=rss

I was bored one morning, so I added something similar to the login page of one of my projects. It pulls one random image from lolcats' RSS feed and inserts it into the page. Here's the code that pulls the image:

$feed = $this->get('feed_parser'); // This is just a SimplePie object
$feed->set_feed_url('http://feeds.feedburner.com/lolcats/rss');
$feed->init();
$items = $feed->get_items();
$item = $items[array_rand($items)]; // Gets one random image, but can modify for more
$item = $item->get_content();

$this->get('feed_parser') is just a fancy way of getting a SimplePie object from the PIMPLE container - you could just instantiate a SimplePie object yourself (if you want code you can't test). After running this code, $item would be a PHP array (or some collection class) containing the details of one image. In my case, I then exposed this as JSON and a REST API endpoint for use by JavaScript, but you could just as easily have PHP output the appropriate HTML.


A little bash function, assigning attributes This function checks if an attribute has been passed and if not, asks user for input.

I'm new in bash scripting and would like to get some feedback. Is it okay or should I refactor this code somehow to make it more concise?

#!/usr/bin/env bash

fn() {
    if [ -z "$1" ]; then
        read -p "username:" username
    else
        username=$1
    fi

    echo "username is $username"
}

What you're doing is perfectly fine.

If you absolutely want to shorten it (at a slight loss of readability) for whatever reason you could do it this way (at least in bash):

fn() {
    [ -z "$1" ] && read -p "username:" username || username=$1
    echo "username is $username"
}

Output (in a bash environment):

$ fn() {
>     [ -z "$1" ] && read -p "username:" username || username=$1
>     echo "username is $username"
> }
$ fn
username:xx
username is xx
$ fn yy
username is yy
$


Customized method for dataframe in my analysis module Please download the file and save in home directory and extract it:

https://www.dropbox.com/s/swtw8bk35zr1i6d/analyse.7z?dl=0

I can get some warrants info.

import analyse
df = analyse.warrants.get('0788')

Now we got 0788's warrants info. It output df with better format:

analyse.tabulate.tabulate(df)

I want to simply the expression as df.tabulate(). How do I refactor the code in analyse directory?

Show content in tabulate.py:

import pandas as pd
from terminaltables import AsciiTable

def tabulate(df):
    rows = len(df)
    head = [df.columns.tolist()]
    nr = df.values.tolist()
    content = head + nr
    table = AsciiTable(content)
    data_str = table.table.split('\n')
    sep_line = data_str[0]
    transformed_str = []
    for ind in range(0,len(data_str)-1):
        if ind < 3 :
            transformed_str.append(data_str[ind])
        else:
            transformed_str.append(data_str[ind])
            transformed_str.append(sep_line)
    new_str = "\n".join(transformed_str) + '\n' + '\n'  + '[ {} records ]'.format(rows-1)
    print(new_str)

I want to refactor analyse.tabulate.tabulate(df) as df.tabulate(). It is the simplest way to use df.tabulate() to get well-formatted dataframe.

This if statement

    if ind < 3 :
        transformed_str.append(data_str[ind])
    else:
        transformed_str.append(data_str[ind])
        transformed_str.append(sep_line)

Can be rewritten as

    transformed_str.append(data_str[ind])
    if ind >= 3:
        transformed_str.append(sep_line)


HashMap Implementation (dictionary) in Python I attempted making a hashmap in python, and it was harder, due to some limitations but this is my version of dictionaries in python. Are there any way to simplify or do the same thing in less code along with tips and tricks?

class HashMap:
    def __init__(self, memory): # refers to the length of the bucket
            self.data = [None] * memory
            self.memory = memory

    def _hash(self, key):
        hashed_value = 0
        bucket_length = self.memory
        string_length = len(key)
        i = 0
        while i < string_length:
            hashed_value += (ord(key[i]) * i) % bucket_length
            if hashed_value > bucket_length-1:
                hashed_value %= bucket_length-1

            i += 1
        return hashed_value

    def set(self, key, value):
        address = self._hash(key)
        bucket = self.data[address]
        if not bucket:
            self.data[address] = [key, value, None] # None refers to next pointer
        else:
            while bucket[2] != None:
                bucket = bucket[2]
            bucket[2] = [key, value, None]

    def get(self, key):
        address = self._hash(key)
        bucket = self.data[address]
        if bucket:
            while bucket[2] != None or key != bucket[0]:
                bucket = bucket[2]
            if bucket:
                return bucket[1]
        raise KeyError

    def keys(self):
        keys_list = []
        bucket_list = self.data
        for bucket in bucket_list:
            current_bucket = bucket
            if bucket:
                while current_bucket != None:
                    keys_list.append(current_bucket[0])
                    current_bucket = current_bucket[2]
        return keys_list

Your implementation of get is wrong. The code is:

def get(self, key):
        address = self._hash(key)
        bucket = self.data[address]
        if bucket:
            while bucket[2] != None or key != bucket[0]:
                bucket = bucket[2]
            if bucket:
                return bucket[1]
        raise KeyError

The line while bucket[2] != None or key != bucket[0] says "keep traversing the link list as long as it's possible to do so, and if it's impossible, try to do it anyway if the key is wrong". Because of the boolean or, the condition bucket[2] != None means the loop will always step forward in the linked list if it's possible to do so - even if the current key is correct. On top of that, once the loop gets to the last element, if the key at that position does not match the given key, the loop will attempt to iterate once more, giving us:

TypeError                                 Traceback (most recent call last)
<ipython-input-7-a5939dc0e83e> in <module>()
----> 1 h.get("apple")

<ipython-input-1-4777e6d3506b> in get(self, key)
     31         bucket = self.data[address]
     32         if bucket:
---> 33             while bucket[2] != None or key != bucket[0]:
     34                 bucket = bucket[2]
     35             if bucket:

TypeError: 'NoneType' object is not subscriptable

The result is get fails with this error in every case except when the requested key is the last one in its slot.

The correct condition is of course while bucket[2] != None and key != bucket[0]. We then need to check afterwards that we got out of the loop because we found the right key, not because we ran out of buckets, giving us the implementation:

def get(self, key):
        address = self._hash(key)
        bucket = self.data[address]
        if bucket:
            while bucket[2] != None and key != bucket[0]:
                bucket = bucket[2]
            if bucket[0] == key:
                return bucket[1]
        raise KeyError