Finding word acronyms for telephone numbers Having read most of Learn You a Haskell as well as parts of Real World Haskell, I decided to embark on an easy project to see if I can apply the Haskell that I've learned.

{-# LANGUAGE OverloadedStrings #-}

import Data.Maybe
import Text.Regex.TDFA
import qualified Data.ByteString.Char8 as B

main = do
       dictionary <- B.readFile "words.txt"
       putStrLn "What is your phone number?"
       phoneNumber <- getLine
       B.putStrLn (outputMessage (getMatches dictionary (toPattern phoneNumber)))

outputMessage :: Maybe [B.ByteString] -> B.ByteString
outputMessage Nothing   = "Some input was incorrect. Make sure no letters or numbers with 0 or 1 are being used."
outputMessage (Just []) = "Sorry. No matches were found."
outputMessage (Just x)  = "Matching words: \n" `B.append` ( B.unlines x)

getMatches _       Nothing        = Nothing
getMatches _       (Just "")      = Just []
getMatches words   (Just pattern) = if (words == B.empty)
                                    then error "Was not able to read dictionary file."
                                    else Just (getAllTextMatches (words =~ (B.pack ("\\b" ++ pattern  ++ "\\b")) ))

toPattern = parseList . map toRegex . readNumber

parseList list
    | Nothing `elem` list = Nothing
    | otherwise = Just (concat . catMaybes $ list)

toRegex :: (Data.String.IsString a1, Num a, Eq a) => a -> Maybe a1
toRegex 2 = Just "(a|b|c)"
toRegex 3 = Just "(d|e|f)"
toRegex 4 = Just "(g|h|i)"
toRegex 5 = Just "(j|k|l)"
toRegex 6 = Just "(m|n|o)"
toRegex 7 = Just "(p|q|r|s)"
toRegex 8 = Just "(t|u|v)"
toRegex 9 = Just "(w|x|y|z)"
toRegex _ = Nothing

readNumber :: [Char] -> [Integer]
readNumber = map $ read . (:"")

I'm interested in any suggestions for making my code more efficient/idiomatic.

This is pretty nice code.

I have two main concerns. The first one is about handling errors.
You are mixing tree approaches: Maybe, throwing explicit error, throwing implicit error (as in readNumber).
It's better to stick to Maybe or Either — they are explicit and you can handle them in pure code.

The second one is about dividing your program in meaningful functions each having single responsibility. Such functions are simpler and you can more easily compose them.

For example, getMatches checks if dictionary is not empty, adds \b to pattern and matches pattern.
It is better to split error handling from data processing (in this case by lifting dictionary check to main).

Another one is readNumber: it parses string into list of digits and implicitly checks for nondigits (throwing useless 'no parse' error).
You can skip this as you check for invalid symbols in toRegex.

Some other remarks:

the type of toRegex is so general that it almost useless. I think you actually meant Integer -> Maybe String;
in parseList you first scanning list for Nothing and then catMaybes scanning it again to remove Nothings. It is possible to do this in single pass by using sequence.

Here is my take:

{-# LANGUAGE OverloadedStrings #-}

import Text.Regex.TDFA
import qualified Data.ByteString.Char8 as B
import Data.Monoid ((<>))
import Control.Monad (when)
import System.Exit (die)
-- `die` was introduced in the latest base, you can replace it with
-- die err = putStrLn err >> exitFailure

main = do
  dictionary <- B.readFile "words.txt"
  when (dictionary == "") $ die "Your dictionary is empty."

  putStrLn "What is your phone number?"
  phoneNumber <- getLine
  case toPattern phoneNumber of
    Nothing ->
      die "Some input was incorrect. Make sure no letters or numbers with 0 or 1 are being used."
    Just pattern -> case getAllTextMatches $ dictionary =~ pattern of
      []  -> B.putStrLn $ "Sorry. No matches were found."
      res -> B.putStrLn $ "Matching words: \n" <> B.unlines res

toPattern :: String -> Maybe B.ByteString
toPattern str = do
  parts <- sequence $ map toRegex str
  return $ "\\b" <> B.concat parts <> "\\b"

toRegex :: Char -> Maybe B.ByteString
toRegex = flip lookup
  [('2', "(a|b|c)")    ,('3', "(d|e|f)")
  ,('4', "(g|h|i)")    ,('5', "(j|k|l)")
  ,('6', "(m|n|o)")    ,('7', "(p|q|r|s)")
  ,('8', "(t|u|v)")    ,('9', "(w|x|y|z)")
  ]


JavaScript shortcut to create elements I've created a personal API for creating elements for an HTML page. I would like feedback on efficiency, particularly in creating buttons and labels. Any and all feedback is appreciated and considered

tag.js

/*
* Shortcuts for creatings different tags
*/
function createLabel(text) {
    let label_tag = document.createElement('label');
    let label_tag_text = document.createTextNode(text);
    tabel_tag.appendChild(label_tag_text);
    return label_tag;
}

function createButton(type) {
    /* need to manually add .onclick when creating button */
    let button_tag = document.createElement('button');
    button_tag.type = type;
    return button_tag;
}

function createInput(type, id) {
    let input_tag = document.createElement('input');
    input_tag.type = type;
    input_tag.id = id;
    return input_tag;
}

function createDiv(id) {
    let div_tag = document.createElement('div');
    div_tag.id = id;
    return div_tag;
}

function createTag(tag) {
    /*
    * Used for creating basic tags (
,
,
, etc)
    * Any tag that doesn't often use an ID, TYPE, NAME, CLASS, etc
    */
    return document.createElement(tag);
}

You have a lot of repeated code. If you follow the same design you will end up with a huge list of create???() functions

You could simplify to a single function, and pass tag name and an optional properties containing element and style properties.

const createTag = (tag, props = {}) => {
    const style = props.style;
    if (style) { delete props.style }
    const el = Object.assign(document.createElement,props);
    if (style) { Object.assign(el.style, style) }
    return el;
}

Thus you can create tags

createTag("input", {type : "button", id : "buttonId"});
createTag("button", {value : "Click me"});
createTag("div", {textContent: "abc", id: "abcId", className: "abcDiv", style: {width: "10px"}});

If you did want to have a function to create each type then it would pay to put them together.

const createDOM = {
    label(...
    button(...
    input(...
    div(...
    // and so on
}


Select to pick quarter (date) An includable select to pick the first day of quarter:

<cfparam name="dtFirst" default="#now()#"><!--- first date in select --->
<cfparam name="selQtrName" default="selQtr"><!--- name attr of select --->
<cfparam name="selQtrCss" default=""><!--- special style css --->
<cfparam name="howManyOpts" default="10"><!--- how many quarters to go back --->
<cfparam name="selQtrDefault" default=""><!--- default selected date  current qtr: #right("0"&(1+(quarter(now())-1)*3), 2)#/01/#year(now())# --->
<cfset raQtoDt=["01/01/","04/01/","07/01/","10/01/"]>
<cfset qtrZWX=quarter(dtFirst)>
<cfset yrZWX=year(dtFirst)>
<cfoutput>
<select name="#selQtrName#" id="#selQtrName#" style="#selQtrCss#">
<cfloop from="1" to="#howManyOpts#" index="idxZWX">
    <option value="#raQtoDt[qtrZWX]##yrZWX#"
    <cfif len(selQtrDefault) and selQtrDefault eq "#raQtoDt[qtrZWX]##yrZWX#">selected</cfif>>#raQtoDt[qtrZWX]##yrZWX#</option>
    <cfset qtrZWX=qtrZWX-1>
    <cfif qtrZWX eq "0">
        <cfset qtrZWX=4>
        <cfset yrZWX=yrZWX-1>
    </cfif>
</cfloop>
</select>
</cfoutput>

My primary suggestion would be to try and make the code more intuitive.

While the current variable names are syntactically valid, they are a bit cryptic IMO. Consider using slightly longer and more descriptive names throughout the code. That will make it easier to understand when you come back to it a few months down the road.
Use date objects (not strings), when generating the quarter dates. Start by creating the initial quarter date with the CreateDate() function. (Use this formula to calculate the first quarter of the month). Then use DateAdd() to derive the remaining quarters. For comparisons, use DateCompare().
Consider using a non-ambiguous format like yyyy-mm-dd for the list value. You can still display a more user friendly format for the list text.
Be sure to explicitly scope all variables to avoid accidental scope conflicts. For example, if the variables are submitted via URL, use url.howManyOpts instead of just howManyOpts

<cfparam name="url.startDate" default="#now()#">
<cfparam name="url.howManyOpts" default="10">
<cfparam name="url.selQuarter" default="">

<!--- create starting quarter date --->
<cfset quarterMonth = Int((month(url.startDate) - 1) / 3) * 3 + 1>
<cfset currQuarter = createDate(year(url.startDate), quarterMonth, 1)>

<!--- if selected quarter was not defined, set a default --->
<cfif !len(trim(url.selQuarter))>
    <cfset url.selQuarter = currQuarter>
</cfif>

<select name="selQuarter">
   <cfoutput>
   <cfloop from="1" to="#url.howManyOpts#" index="x">
       <!--- separated for clarity --->
       <cfset optValue   = dateFormat(currQuarter, 'yyyy-mm-dd')>
       <cfset optText    = dateFormat(currQuarter, 'mm-dd-yyyy')>
       <cfset isSelected = dateCompare(currQuarter, url.selQuarter) eq 0>

       <option value="#optValue#" <cfif isSelQuarter>selected</cfif>>#optText#</option>

       <!--- move to next quarter --->
       <cfset currQuarter = dateAdd("m", 3, currQuarter)>
    </cfloop>
    </cfoutput>
</select>


A JavaScript isEven() function, defined recursively Can someone please take a look at these recursive functions, offer some advice on the code, or just critique my attempt into thinking recursively?

Problem:

  We’ve seen that % (the remainder operator) can be used to test whether a number is even or odd by using % 2 to see whether it’s divisible by two. Here’s another way to define whether a positive whole number is even or odd:

  Zero is even.
  One is odd.
  For any other number \$N\$, its evenness is the same as \$N - 2\$.

  Define a recursive function isEven corresponding to this description. The function should accept a single parameter (a positive, whole number) and return a Boolean. - eloquentJavascript, chapter 3

My attempt:

function isEven(x) {
  if (x < 0 ) {
        return false
  }
   else if (x % 2 == 0) {
        return true
  } else {
        return isEven(x-2)
  }
}

Offical Solution:

function isEven(n) {
  if (n == 0)
    return true;
  else if (n == 1)
    return false;
  else if (n < 0)
    return isEven(-n);
  else
    return isEven(n - 2);
}

I know there are many ways to write the same piece of functionality but I want to know if the following code holds up to scrutiny. The official solution I checked my code against is obviously different but I'm getting the correct solutions.

console.log(isEven(50));
// → true
console.log(isEven(75));
// → false
console.log(isEven(-1));
// → false

What events does

else if (n < 0)
    return isEven(-n);

handle that,

 else
    return isEven(n - 2);

doesn't?

I started reading 'the little Schemer' on the advice of Douglas Crockford in an attempt to learn how to think recursively but the examples are in Scheme which just looks like ((( n ) * atom)) to me...so not very useful until I can read the syntax.

The solutions are nowhere near equivalent.

You're using the remainder operator %, when the whole point - I assume - is to not do that (for the purposes of the exercise - in any other case, definitely use %).

The official solution only knows how to answer true/false for the numbers 0 and 1. For any other input, it uses recursion to reduce the input all the way to either 0 or 1. It also flips negative numbers, in order to get those to 0 or 1*.

Your solution answers immediately for any positive, even number, without recursion. So you're sidestepping the whole exercise. And your code ends up making little sense, since, if you can answer true immediately, it follows that you can answer false immediately too. Those are the only possible return values, so if it's not true it logically must be false. Since you're not recursing for the first case, why would you bother to recurse for the second one?

In addition, your code treats all negative numbers as odd, unlike the official solution. And math in general.

To put all this another way: The official solution approaches the problem with this knowledge:

0 is even
1 is odd
We don't know anything about the parity of other numbers
But any number can be reduced to either 0 or 1 by repeatedly subtracting 2 and/or flipping the sign if the number's negative.

Hence the need to recurse for numbers that are not 0 or 1.

Your approach says:

Numbers that can be cleanly divided by 2 are even (which is cheating)
Negative numbers are odd (which is wrong)
Any other number becomes negative if you keep subtracting 2

From the last point follows that any number that is not cleanly divisible by 2 is automatically an odd number. But you code doesn't make this leap, and instead uses (pointless) recursion.

Neither solution is by any means a good solution for determining the parity of a number(!), but the point (I assume, without knowing the context of the problem description) is to use recursion to drill down to something that can be answered categorically for specific numbers, i.e. 0 is even, 1 is odd. Your solution answers for sets of numbers, i.e positive even numbers and negative numbers.

*) For all of this we're assuming the input will always be a whole number, and not something else. Yes, you can do input validation, but that's beyond the scope of the exercise and this review


Queue implementation in ES6 Here's my Queue implementation in ES6,

class Queue{
    constructor(){
        this.head = null;
        this.tail = null;
    }
    offer(item){
        const p = new Node(item);
        if(!this.head){
            this.head = p;
            this.tail = p;
            return;
        }
        this.tail.next = p;
        this.tail = p;

    }
    poll(){
        const item = this.head;
        this.head = this.head.next;
        return item;

    }
    peek(){
        return this.head.val;
    }
    isEmpty(){
        return this.head === null;
    }
}

Please review on all fronts.

Does not work, can be mutated

You did not even test this object. Please take time to make sure the code you put up for review is at least working next time. :)

Bugs.

You don't tests if head is defined for the functions peek, and poll.

const q = new Queue();
q.peek(); // throws TypeError cannot read property val of null
q.poll(); // throws TypeError cannot read property next of null

Can only Get one object from the Queue per group of offer calls

q.offer("A");
q.offer("B");
q.poll(); // return "A"
q.peek(); // throws TypeError cannot read property val of null
q.poll(); // throws TypeError cannot read property next of null

// OR
q.offer("A");
q.offer("B");
q.poll(); // return "A"
q.offer("C");
q.poll(); // return "C"  What happened to "B"

Mutation

You have not encapsulated the object state and thus can be mutated in many ways to produce unexpected behaviours

const q = new Queue()
q.offer("foo");
q.head.tail = q.head;
while(!q.isEmpty()) { console.log(q.poll().val) } // never stops

// and
q.head = undefined;
q.isEmpty(); // incorrectly returns false

Bloated

The property Queue.tail does nothing and is just noise.

I am guessing you wanted to use it to make offer quicker without needing to step all the way down the links from head but that will require a doubly linked list.

You could just use an array.


Generating a random number of progress bars I have this JSFiddle that generates a random number of progress bars then assigns a random number and percent to them.

I am wondering if you know how to better optimize this code to have better run-time.

/*Creates a random amount of bars*/

function runBars() {
    var start = new Date().getTime();
    /*Sets the maximuim amount of bars, 10 if NaN or less than 1*/
    var max_bars = parseInt($('#max_bars').val());
    if (isNaN(max_bars) || max_bars < 1) {
        max_bars = 10;
    }
    /*Grabs a random number of bars*/
    var bars = getRandom(max_bars);
    /*clears bars container*/
    $('#bars-container').html('');
    /*Appends progress bars.*/
    for (var i = 0; i < bars; i++) {
        var element = $('Number ' + (i + 1) + '

');
        $('#bars-container').append(element);
    }
    var end = new Date().getTime();
    var time = end - start;
    $('#bar_time').text(time);
    $('#bar_amount').text(bars);
    /*Populates bars with percents*/
    runPercent();
}

/*Random int with max*/
function getRandom(max) {
    return Math.floor((Math.random() * max) + 1);
}

/*Gets percent based on maxium number, making the percent scale */
function getPercent(x, total, biggest) {
    return Math.floor(x / biggest * 100) + '%';
}

/*Populates percent in each progress bar*/
function runPercent() {
    var start = new Date().getTime();
    var length = $('.progress-bar').size();
    var data = [];
    var total = 0;
    var biggest = 0;
    /*pushes random ints to array*/
    for (var i = 0; i < length; i++) {
        var temp = getRandom(50);
        data.push(temp);
        total += temp;
        if (biggest < temp) {
            biggest = temp
        }
    }
    /*Each progress bar gets a width percentage and sets the text.*/
    $('.progress-bar').each(function (index) {
        $(this).width(getPercent(data[index], total, biggest));
        $(this).text(data[index] + ' / ' + total);
    });
    var end = new Date().getTime();
    var time = end - start;
    $('#percent_time').text(time);
}
/*handles button events*/
$('#percent').click(runPercent);
$('#bars').click(runBars);

/*initiates on page load*/
runBars();
.float {
    position:fixed;
    right:0px;
    text-align:right;
}

<link href="//maxcdn.bootstrapcdn.com/bootstrap/3.3.2/css/bootstrap.min.css" rel="stylesheet"/>

    <button id='percent'>Randomize %</button>
    <button id='bars'>Randomize Bars</button>

    <input id='max_bars' type='number' placeholder='Max Number of Bars' />

Result:

Amount of Bars:

In Miliseconds:

Bar Time:
Percent Time:

1. Native methods will perform better than jQuery equivalents

Replacing jQuery calls with the native JavaScript equivalents where possible will improve performance. This will also increase the verbosity of your code, which may be undesirable, although arguably it can make the code more readable/less ambiguous.

Examples:

Use document.getElementById('ID') instead of $('#ID') when retrieving a single element by its ID
use document.querySelector('.className') instead of $('.className') when retrieving a single element by a CSS selector
Use document.querySelectorAll('.className') instead of $('.className') when retrieving an array of elements by a CSS selector
Use Element.innerHTML to update the text/html inside an element instead of using $(.cssSelector).html() or $(.cssSelector).text()
If you're only appending HTML to an element, use the more efficient Element.insertAdjacentHTML('beforeend',YourHTML) instead of modifying Element.innerHTML

2. Don't query the DOM for elements any more than you need to

In some cases your code selects an element multiple times. You can improve performance by selecting it once and saving it to a variable, then referencing that variable instead of selecting it again. Otherwise the JavaScript is forced to crawl the DOM multiple times to retrieve the same element.

Examples:

In the runBars() method, $('#bars-container') is called once to clear its HTML content, then again several times (once for every bar) to append more HTML to it. This is causing performance to degrade steeply as the number of bars increases.
Similarly, in the runPercent() method, the array of progress bars is selected via $('.progress-bar') once to get its length, then selected a second time to set the width and text of each progress bar.

3. Include a Radix parameter when calling parseInt()

This one is more of a minor nitpick: when calling parseInt() you should make it a habit to include a radix parameter indicating the base of the number in the string.

Otherwise the browser will do its best to pick the right base system based on the string value... strings that start with '0x' will be considered hexadecimal, while strings that start with '0' will be considered octal, and any other strings will be considered decimals.

Making a habit of passing 10 explicitly as your radix parameter will not only save the browser the tiny bit of effort it expends on checking for a string prefix, but it will save you from headaches if you ever find yourself parsing a string with a zero preceding a decimal number.

Example Code

Check out the performance of the code below, which has no jQuery dependencies except for the progress bar CSS:

/*Creates a random amount of bars*/

function runBars() {
    var start = new Date().getTime();
    /*Sets the maximuim amount of bars, 10 if NaN or less than 1*/
    var max_bars = parseInt(document.getElementById('max_bars').value, 10);
    if (isNaN(max_bars) || max_bars < 1) {
        max_bars = 10;
    }
    /*Grabs a random number of bars*/
    var bars = getRandom(max_bars);
    /*clears bars container*/
    var container = document.getElementById('bars-container');
    container.innerHTML = '';
    /*Appends progress bars.*/
    for (var i = 0; i < bars; i++) {
        container.insertAdjacentHTML('beforeEnd', 'Number ' + (i + 1) + '

');
    }
    var end = new Date().getTime();
    var time = end - start;
    document.getElementById('bar_time').innerHTML = time;
    document.getElementById('bar_amount').innerHTML = bars;
    /*Populates bars with percents*/
    runPercent();
}

/*Random int with max*/
function getRandom(max) {
    return Math.floor((Math.random() * max) + 1);
}

/*Gets percent based on maxium number, making the percent scale */
function getPercent(x, total, biggest) {
    return Math.floor(x / biggest * 100) + '%';
}

/*Populates percent in each progress bar*/
function runPercent() {
    var start = new Date().getTime();
    var progressBars = document.querySelectorAll(".progress-bar");
    var data = [];
    var total = 0;
    var biggest = 0;
    /*pushes random ints to array*/
    for (var i = 0; i < progressBars.length; i++) {
        var temp = getRandom(50);
        data.push(temp);
        total += temp;
        if (biggest < temp) {
            biggest = temp;
        }
    }
    /*Each progress bar gets a width percentage and sets the text.*/
    for (var index = 0; index < progressBars.length; index++) {
        progressBars[index].style.width = getPercent(data[index], total, biggest);
        progressBars[index].innerHTML = data[index] + ' / ' + total;
    }
    var end = new Date().getTime();
    var time = end - start;
    document.getElementById('percent_time').innerHTML = time;
}
/*handles button events*/
var btnPercent = document.getElementById("percent");
var btnBars = document.getElementById("bars");
if (btnPercent.addEventListener) {
    btnPercent.addEventListener("click", runPercent);
    btnBars.addEventListener("click", runBars);
} else {
    btnPercent.attachEvent("onclick", runPercent);
    btnBars.attachEvent("onclick", runBars);
}

/*initiates on page load*/
runBars();
.float {
  position: fixed;
  right: 0px;
  text-align: right;
}
<link href="//maxcdn.bootstrapcdn.com/bootstrap/3.3.2/css/bootstrap.min.css" rel="stylesheet"/>

  <button id='percent'>Randomize %</button>
  <button id='bars'>Randomize Bars</button>

  <input id='max_bars' type='number' placeholder='Max Number of Bars' />

Result:

Amount of Bars:

In Miliseconds:

Bar Time:
Percent Time:


EF entity, extended with partial classes for actions Me colleague and I recently had a discussion on good practices in EF. So I showed one of mine.

He said it is a little bit muddler.

My practice consists of modifying the autogenerated class in a specific way. This is my starting model:

namespace PCServer.Data
{
    using System;
    using System.Collections.Generic;

    public partial class Post: IEntity
    {
        public int Id { get; set; }
        public string Title { get; set; }
        public string Message { get; set; }
        public DateTime Date { get; set; }

        public virtual Post ParentPost { get; set; }
        public virtual AspNetUser Author { get; set; }
    }
}

I like to extend it in this way:

public partial class Post
    {
        //This class "do" something, like adding a post or deleting a post
        public static class Do
        {
            public static void AddPost(ref ApplicationDbContext context, string postMessage)
            {
                //Create a post
                Post p = new Post();
                p.Title = "This is an example!";
                p.message = postMessage;
                p.Date = DateTime.UtcNow;

                //Adding to context
                BaseService.Add(post, out context);
            }

            public static void DeletePost(ref ApplicationDbContext context, int postId)
            {
                PostRepository postRepo = new PostRepository(context);

                postRepo.GetById(postId);

                //Removing from context
                BaseService.Remove(post, out context);
            }
        }

        //This class "Get" something, like all posts
        public static class Get
        {
            public static void GetPosts()
            {
                using(ApplicationDbContext context = new ApplicationDbContext())
                {
                    PostRepository postRepo = new PostRepository(context);
                    return postRepo.GetAllPosts();
                }
            }
        }

        //This class "Set" something, like title of the post or the post itself maybe
        public static class Set
        {
            public static void Title(ref ApplicationDbContext context, int postId, string title)
            {
                PostRepository postRepo = new PostRepository(context);
                Post post = postRepo.GetById(postId);
                post.Title = title;

                BaseService.Update(post, out context);
            }

            public static void ChangePost(ref ApplicationDbContext context, int postId, Post post)
            {
                PostRepository postRepo = new PostRepository(context);
                Post dbPost = postRepo.GetById(postId);
                dbPost = post;

                BaseService.Update(dbPost, out context);
            }
        }
    }

So, when I must do something with an Entity, I can (for example only):

ApplicationDbContext c = new ApplicationDbContext();

Post.Do.AddPost(ref c,"Hi!");
IEnumerable<Post> posts = Post.Get.GetPosts();

Post.Set.Title(ref c,100,"Changing title!");

And after all:

await BaseService.CommitAsync<Post>(c);

What do you think? Would you use it? Why?

Here is my original post of this on SO.

The best aspect of your approach is that you keep the autogenerated class as is, which makes it more readable and clean, and keeps your extra code safe from deletion if you modify the database schema.

You can extend this class with a partial class to add a constructor setting automatic properties like Date to DateTime.UtcNow.

The static classes Do, Get, and Set are not using any of the members of the entity class, so I see no reason of being part of an entity unless you mean to add some Fluent Interface to your code. But this has pros and cons, and these static classes are not the best way to do so. Also, would you do that for all entities in your model? too much repetition, complexity, error prone, and dependency on context.

In Entity Framework, the best practices is to keep the entities unaware of their context or repository for a variety of reasons like single responsibility, and testabilty.

By using EF designer, you automatically generate code like below, which you can modify or extend

public class BloggingContext : ApplicationDbContext
{
    public System.Data.Entity.DbSet<PCServer.Data.Post> Posts { get; set; }
    public System.Data.Entity.DbSet<PCServer.Data.User> Users { get; set; }
    ...
}

You can create multiple context classes, but usually I did not see any benefit of doing so. One context for all your entities is fine.

The context owns the repository here as Posts property here is your repository, no need to create a new one. And DbSet has out-of-the-box all the repository methods you need like Create, Add, Find, and Remove.

Setting entity properties and calling repository methods would then be simple and called directly as you did in the last lines in your question, like below:

BloggingContext context = new BloggingContext();

context.Posts.Add(new Post { Title = "Hi!" });
List<Post> posts = await context.Posts.ToListAsync();

Post post = context.Posts.First(p => p.Id == 100); // or context.Posts.Find(100) if Post.Id is marked as [Key]
post.Title = "Changing title!";

And after all:

await context.SaveChangesAsync();


Communication to Child Controller This is a simple AngularJs application with two nested controllers. We need to pass data from the parent to the child Controller.

The first thing you find when googling for a solution would be this, by calling $scope.$parent in the child controller:

angular.module("outerModule", [])
  .component('outerModule', {
      controller: function($scope) {
        $scope.innerModuleConfigData = {
          text : 'hello'
        }
      }
  })

angular.module("innerModule", [])
  .component('innerModule', {
      controller: function($scope) {
        console.log($scope.$parent.innerModuleConfigData.text);
      }
  })

The critique here is that the parent controller has to know exactly what to call its shared variable.

Another thing that comes to my mind is the thing with the nested scope when working with angular directives:

<inner-module configData="innerModuleConfigData"></inner-module>

I'm not sure if something like this works for nested controllers as well.

The consensus to a 'best practice' solution seems to be sending the data by a service, which is injected to both controllers. But this seems to be a little overheaded, if only small amounts of data are to be shared.

Having to refer to $scope.$parent is absolutely bad-practice, because dependencies will go up and down and all sorts of ways in your application. It will become a big mess. It is best to only pass dependencies down the scope chain, not up.

Exactly what you describe, you got two options. Go the nested scope way or use a service.

It is indeed best-practice to use a service when more logic is being handled. It's really up to you to choose between the two options. Both are good, but you need to think about the use-case. Will there be more data be passed along to both controllers; definitely go for the service. Is it just a simple pass this variable to the underlying controller go the nested scope route.


Movie database browser My code looks like "spaghetti code". I have yet to use OOP in PHP but I am really interested in how a good programmer would handle this many if statements. It looks like a mess.

<?php
    session_start();
    include './functions.php';

    if(empty($_FILES['browseMovie']) || empty($_FILES['browseThumbnail']) || empty($_GET['title']) || empty($_GET['description']))
    {
        die('Please insert all fields.');
    }

    $filePartsMovie = pathinfo($_FILES['browseMovie']['name']);
    $filePartsThumbnail = pathinfo($_FILES['browseThumbnail']['name']);

    if($filePartsMovie['extension'] !== "mp4")
    {
        die('Video has to be a .mp4 file.');
    }

    if($filePartsThumbnail['extension'] !== "jpg" && $filePartsThumbnail['extension'] !== "png")
    {
        die('Thumbnail has to be in jpg/jpeg or png format.');
    }

    if($_FILES['browseMovie']['size'] + $_FILES['browseThumbnail']['size'] > 1600600000)
    {
        die('filesize of video and thumbnail = higher 1.5 GB.');
    }

    $id = $_SESSION["id"];
    $name = $_SESSION["username"];
    $folderName = generateUniqueFolder();
    $title = mysql_real_escape_string($_GET['title']);
    $description = mysql_real_escape_string($_GET['description']);

    // the original file name will be stored in the database as well
    $thumbFileName = basename($_FILES['browseThumbnail']['name']);
    $vidFileName = basename($_FILES['browseMovie']['name']);

    $userDir = "../users/$name/";
    $videoDir = $userDir . "videos/" . $folderName . "/";

    $uploadMovieDir =  $videoDir . $vidFileName;
    $uploadThumbnailDir = $videoDir . $thumbFileName;

    if(!isLoggedIn() || !is_dir($userDir))
    {
        die('User does not have own folder. Please contact the administrator.');
    }

    if(!mkdir($videoDir))
    {
        die('Could not create unique video folder.');
    }

    if(!move_uploaded_file($_FILES['browseMovie']['tmp_name'], $uploadMovieDir))
    {
        rmdir($videoDir);
        die('Could not upload the video correctly.');
    }

    if(!move_uploaded_file($_FILES['browseThumbnail']['tmp_name'], $uploadThumbnailDir))
    {
        removeTreeDir($videoDir);
        die('Could not upload the thumbnail correctly.');
    }

    $con = connectDb();
    $videoQuery = "INSERT INTO video(userId, title, description, foldername, vidFileName, thumbFileName) VALUES ($id,'$title','$description','$folderName', '$vidFileName', '$thumbFileName')";

    if(!mysql_query($videoQuery))
    {
        removeTreeDir($videoDir);
        die('Could not add the video to the database.');
    }

    mysql_close($con);

    echo "true";
?>

Somewhere in the middle of my code I also need a bunch of variables. How would one clean that up? Create an object and aggregate them together?

Your first if can be simplifed with any function.

function any ($list, $pred)
{
     foreach ($list as $value)
     {
          if (call_user_func ($pred, $value) == TRUE)
             return TRUE;
     }
     return FALSE;
}

So it will be :

 $toTest = [$_FILES['browseMovie'],
            $_FILES['browseThumbnail'],
            $_GET['title'],
            $_GET['description'] ]
 if (any ($toTest, "empty")) die ("Please insert all fields")

Then you can simplify the code even more by creating a validate type function

function validateFile ($filePartsMovie, $filePartsThumbnail)
{
      if($filePartsMovie['extension'] !== "mp4")
      {
           die('Video has to be a .mp4 file.');
      }

      if($filePartsThumbnail['extension'] !== "jpg" &&
         $filePartsThumbnail['extension'] !== "png")
      {
          die('Thumbnail has to be in jpg/jpeg or png format.');
      }

     if($_FILES['browseMovie']['size'] + $_FILES['browseThumbnail']['size'] > 1600600000)
     {
              die('filesize of video and thumbnail = higher 1.5 GB.');
     }
}

Then you can create a validate function for the rest of the code.

TLDR is to split your code in more separate functions.

BTW I think you do not need brackets when having only one statement after if.


Todo application implementation I am new to c# and decided to write a classic todo app. The implementation of my application now is like this: there is a Task class that describes the task. The List class acts simultaneously as a storage and has methods for creating, deleting, and displaying tasks. How good is this approach, how can you do better (if possible)? I would like a full code review.
There is Program.cs:
using System;

namespace ToDo
{
    class Program
    {
        static void Main(string[] args)
        {
            List list = new List();
            list.AddTask("Task №1");
            list.AddTask("Task №2");
            list.AddTask("Task №3");
            list.AddTask("Task №4");
            list.AddTask("Task №5");
            list.WriteTasks();
            list.RemoveTask(2);
            list.WriteTasks();
        }
    }
}

List.cs:
using System;
using System.Collections.Generic;
using System.Text;

namespace ToDo
{
    class List
    {
        private List<Task> tasks = new List<Task>();
        public void AddTask(string body)
        {
            tasks.Add(new Task() { Body = body });
        }
        public void WriteTasks()
        {
            for (int i = 0; i < tasks.Count; i++)
            {
                Console.WriteLine($"[{i+1}]: {tasks[i].Body}");
            }

            Console.ReadKey();
        }
        public void RemoveTask(int number)
        {
            for (int i = 0; i < tasks.Count; i++)
            {
                if (number == i + 1)
                {
                    tasks.RemoveAt(i);
                }
            }
        }
    }
}

Task.cs:
using System;
using System.Collections.Generic;
using System.Text;

namespace ToDo
{
    class Task
    {
        public string Body { get; set; }
    }
}

As a very basic application this is fine, however, if the program becomes more complicated and you want to add a data base and asynchronous programming you will run into conflicts with some of the class names. For instance the Task class is supplied by the C# library for asynchronous programming.
If I was going to implement a task list application I would have a few more properties for the task class, such as:

Priority
Status (not started, started, in progress, completed).
Due date
Name
Description

These are the minimum I would use, some additional properties might be

Assigned to
Assigned by

if this was a multi-user task list.


Standard way of reading file contents to a buffer I have been trying to write a sample code block to read file contents into a buffer.

I am assuming that:

Code must be platform independent
Should work in all cases(or report proper error)
It should be readable and maintainable
It shouldn't be over-complex and shouldn't have errors

Do you think the return of fseek should be tested as well?

void
die( int error_code, char* message )
{
    fprintf( stderr, message );
    exit( error_code );
}

char *
read_certificate_file ( char *file_location )
{
    FILE *file_descriptor = NULL;
    size_t file_size = 0;
    size_t bytes_read = 0;
    char *certificate_content = NULL;
    file_descriptor = fopen ( file_location, "r" );
    if ( file_descriptor == NULL ) {
        perror ( "Opening certificate file" );
        die ( EXIT_FAILURE, "Error opening certificate file." );
    }
    fseek (file_descriptor, 0, SEEK_END);
    file_size = ftell ( file_descriptor );
    if ( file_size == 0 ) {
        fclose ( file_descriptor );
        die ( EXIT_FAILURE, "Certificate file is empty." );
    }
    certificate_content = (char *) malloc ( file_size+1 );
    if ( certificate_content == NULL ) {
        fclose( file_descriptor );
        die( EXIT_FAILURE, "Error allocating memory for " \
                          " certificate." );
    }
    certificate_content[ file_size ] = '\0';
    fseek ( file_descriptor, 0, SEEK_SET );
    bytes_read = fread ( certificate_content, 1, file_size,
                         file_descriptor );
    if ( bytes_read != file_size ) {
            perror ( "Reading Certificate file" );
            fclose( file_descriptor );
            die ( EXIT_FAILURE, "Error reading "
                                "certificate file." );
    }
    fclose ( file_descriptor );
    return certificate_content;
}

int main()
{
    printf( "%s", read_certificate_file ( "./certificate.pem" ));
    return EXIT_SUCCESS;
}

A few things stand out...

Function declarations

I'm not a huge fan of specify return type on previous line style:

void
die( int error_code, char* message )

However, if you're going to use it, be consistent (your main is defined differently).

should work in all cases

You're assuming that the file you're reading does't contain any nulls.  This may be OK, as you're not opening the file in binary mode, however if you decide to reuse the code for binary files, returning a null terminated buffer without a size field is going to cause issues.

Main is leaking

I know your main is just a test harness to demonstrate the code, however it's best to always use appropriate memory management.  Your function is allocating a buffer using malloc, you should be freeing it in your main.  By including this in your example it demonstrates that you recognise responsibility for the returned buffer is sitting with the caller.


Simple JS OOP - Clicking the box to fetch and display object content I'm relatively new to OOP with JS and want to find out:

If my code can be improved somehow, and
key question Identify how and where I can typically use OOP more in my job (as I can usually just use arrays and don't need objects)

I have built a basic demo that just stores some animal data as objects and displays them when clicking on the boxes.

Can anyone briefly review the code and spot any improvements? And more importantly, what are typical tasks that objects are better for rather than just using arrays? I'm a front-end developer and typically use arrays for tasks like storing html data and changing content with click events, hovers and other event listeners, and have never had to use objects although want to.

Thanks for any advice here. My code and CodePen URL are below:

let animals = document.querySelectorAll('.animal'),
    animalsObj = [],
    mainTitle = document.querySelector('#title'),
    mainDescription = document.querySelector('#desc');

class Animal {
  // constructor
  constructor (name, description) {
   this.name = name;
   this.description = description;
      animalsObj.push(this);
  }
}

for (let i = 0; i < animals.length; i++) {
  let name = animals[i].querySelector('.name').textContent,
      description = animals[i].querySelector('.description').textContent;

  // instantiate object
  new Animal(name, description);

  animals[i].addEventListener("click", () => {
    console.log('clicked on ' + animalsObj[i].name);
    mainTitle.textContent = animalsObj[i].name;
    mainDescription.textContent = animalsObj[i].description;
 });
}
.animals {
  display: flex;
}
.animal {
  border: 1px dashed grey;
  padding: 1em;
}
.animal:nth-of-type(odd) { background: #f0f0f0 }
.animal:nth-of-type(even) { background: #ccc }

Lion

A big cat.

Stoat

A weasel thing.

Chicken

Good to eat.

Animal

Description

Codepen: https://codepen.io/ns91/pen/qBWLYLp

OO-Design

  key question Identify how and where I can typically use OOP more in my job (as I can usually just use arrays and don't need objects

In your transformation towards OOP, you're still in the habit of resorting to arrays.
You let each new instance of this class be registered in some global array.

class Animal {
  constructor (name, description) {
    this.name = name;
    this.description = description;
    animalsObj.push(this);
  }
}

Don't do this as it pollutes your object-oriented design.

class Animal {
  constructor (name, description) {
    this.name = name;
    this.description = description;
  }
}

If you wish to store these instances in the array, let the consumer code handle it. This way, you can decide by use case how instances of objects should be handled.

const animals = [];
const animal = new Animal("Roger", "my pet");
animals.push(animal);

This would also keep you from instantiating an object without storing the instance in a variable, because the magic no longer happens behind the screens.

new Animal(name, description);  // no longer registers to a global array


Coding Challenge: Kattis Short problem description:

  A sequence of numbers are antiarithemtic if there exists no three
  numbers a_i, a_j, a_k – where a_i preceeds a_j which then again
  preceeds a_k in the sequence – such that a_i - a_j = a_j - a_k.
  Example: (5, 0, -1, 3, 1) is not antiarithemtic, because 5-3 =
  3-1, wheras the sequence (1, 5, 3, 0, -1) is antiarithemtic.

Link to the Kattis page.

My attempt:

import sys

lines = sys.stdin.readlines()
lines = [x[:-1] for x in lines]
for line in lines:
    if len(line) > 1:
        line = line.split(':')
        found = False
        visited = {}
        curr = line[1].split()
        r = len(curr)
        for i in range(r):
            visited_2 = {}
            if curr[i] in visited:
                continue
            else:
                visited[curr[i]] = True
                for j in range(i+1, r):
                    if curr[j] in visited_2:
                        continue
                    else:
                        visited_2[curr[j]] = True
                        tmp = int(curr[i]) - int(curr[j])
                        for k in range(j+1, r):
                            if int(curr[j]) - int(curr[k]) == tmp:
                                print("no")
                                found = True
                                break
        if not found:
            print("yes")
    else:
        break

I believe my attempt solves the problem of finding out whether a sequence is antiarithmetic or not, as I've made my own extensive example-set to test that. My optimizing step so far has been to include dictionaries of visited "nodes" so as to not repeat searches for numbers that we already know do not result in an arithmetic sequence. However, it is not fast enough for Kattis, so I would much appreciate any suggestions on how to improve this.

making your code harder to read:

lack of docstrings
lack of comments
unwarranted indentation using an else: after a "disruptive if"
(transferring execution with return, break, continue)
(Here, you are using this with otherwise empty "if-parts":
you could negate the conditions and just use the former else:-statements. Hello again, indentation.)


Broadcast event in angularJS How can I refactor this code to be more clean?

  $scope.unreserve = function () {
    var params = {
      'wishlist_product_id' : product.wishlist_product_id
    };

    WishlistService.unreserve(params, function (data) {
      if (data.success) {
        $rootScope.$broadcast('NotificationController.create', {type: 'success', message: 'Unreserve successfully'});
      } else  {
        $rootScope.$broadcast('NotificationController.create', {type: 'error', message: data.error_message});
      }
    });
  };

  $scope.createManualProduct = function () {
    var params = {};

    ProductService.addManualProduct(params, function (data) {
      if (data.success) {
        $rootScope.$broadcast('NotificationController.create', {type: 'success', message: 'Unreserve successfully'});
      } else{
        $rootScope.$broadcast('NotificationController.create', {type: 'error', message: data.error_message});
      }
    });
  };

There are 2 main changes I would make:

ProductService.addManualProduct seems to require a callback. A way that is more consistent with Angular, and with a number of benefits, would be to return a promise, so you can use it like:

ProductService.addManualProduct(params).then(function() {
  // Do something
});

Using $rootScope should be avoided if there is an alternative, as it's effectively global over the entire app. One way, would be to change the $rootScope.$broadcast to $scope.emit, which can be "picked up" by a directive on some parent element. So you could have a template like:

<div notification-center>
  <div ng-controller="myController">
     <!-- Buttons here to create/unreserve -->
  </div>
</div>

And in myController, $emit an event:

$scope.$emit('notificationCenter::create', someData);

Then in a notificationCenter directive's controller, you can listen to this

{
   controller: function($scope) {
     $scope.$on('notificationCenter::create', function(e, data) {
       // Do something, like display the message
     });
   }
}

An alternative approach would be to have the notificationCenter directive inside the template controlled by myController, and then use $scope.$broadcast to send messages to it.

The "clean" aspect of this is that you could potentially have separate areas of the app, with separate notification areas, if you wish.


Devise an algorithm of complexity O(N) finding No of 1's from a 2 dimensional matrix[N][N] containing 1's and 0's Assume a 2D [n][n] matrix of 1's and 0's. All the 1's in any row should come before 0's. The number of 1's in any row I should be at least the number of 1's row (i+1). Find a method and write a C program to count the number of 1's in a 2D matrix. The complexity of the algorithm should be order n.

The question is from Cormen's Algorithm Book. Kindly point out the mistakes in my algorithm and hopefully suggest a better way.

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

  int **map;
   int getMatrix();

  main()
  {
   int n,i,j,t;
 j=0;
 n=getMatrix();
   i=n-1;
    int sum[n];
   for(t=0;t<n;t++)
    sum[t]=0;
int count=0;
 while ( (i>=0) && (j<n) )
 {
  if ( map[i][j] == 1 )
   {
    j++;
    count=count+1;
   }
  else
  {
     if (i==(n-1))
    {
        sum[i]==count;
        count=0;
    }
    else
    {
        sum[i]=sum[i+1]+count;
        count=0;
        i--;
    }
  }
  }
       for (t=0;t<n;t++)
         {
             if ((t==(n-1)) && (sum[t]==0))
               sum[t]=0;
                  else if ((sum[t]==0) && (sum[t+1]>0))
               sum[t]=sum[t+1];
             }
       int s=0;
          for (t=0;t<n;t++)
        s=s+sum[t];
          printf("\nThe No of 1's in the given matrix is %d \n" ,s);
        }

  int getMatrix()
 {
   FILE *input=fopen("matrix.txt","r");
 char c;
     int nVer=0,i,j;
 while((c=getc(input))!='\n')
   if(c>='0' && c<='9')
    nVer++;
      map=malloc(nVer*sizeof(int*));
    rewind(input);
    for(i=0;i<nVer;i++)
 {
  map[i]=malloc(nVer*sizeof(int));
   for(j=0;j<nVer;j++)
  {
    do
    {
        c=getc(input);
    }while(!(c>='0' && c<='9'));
    map[i][j]=c-'0';
}
}
     fclose(input);
  return nVer;
  }

I think your main loop should be something more along the lines of

row = n-1;      // start at bottom row
for (col=0; col<n; col++) {   // read columns from left to right
    while ((row >= 0) && (map[row,col] == 0)) {   // while not out of rows, and on a 0
        sum += col;  //add count of 1s to total
        row--;       //move to next row up
    }
    // do nothing if we're on a 1, just move to next column.
}
if (row >= 0) sum += (row+1)*col; // add in any leftover rows of all 1s
printf("sum is %d\n",sum);


Finding associations for a set of keys I am writing my first programs in Python and want to be sure my code is good and not too C++-like. Consider the problem of finding all common 'associations' for set of 'keys'.

Data format is

first line: integer N
N lines: key: association 1 ... association P_1
N+2th line: integer K
K lines: key 1 ... key T_1

For each line after K, the program should output all common associations for all keys listed.

For example, the correct output for input

10
0: even
1: odd
2: even prime
3: odd prime
4: even composite notprime
5: odd prime
6: even composite notprime
7: odd prime
8: even composite notprime
9: odd composite notprime
2
8 4
2 3

is

composite even notprime
prime

My code is

f = open('input.txt', 'r') #input file
d = {} #associations
uni = set() #set of all associations

for i in range(int(f.readline())):
  t = f.readline().strip().split(': ') #split into list ('key', 'assoc1 ... assoc P_1')
  d[t[0]] = s = set(t[1].split(' ')) #d : key -> set('assoc1', ..., 'assoc P_1')
  uni |= s #add to uni

for i in range(int(f.readline())):
  #read keys as list and intersect uni and list's associations
  print reduce(lambda assoc, key: assoc & d[key], set(f.readline().strip().split(' ')), uni.copy())

How good is my code from the viewpoint of experienced Python programmers?

Your code looks pretty good for a newcomer to the language. Some comments:

I don't quite understand why you need uni.
References to t[0] and t[1]: It's more declarative to unpack tuples/lists and give names to its components -> something, another_thing = t
for i in range(int(f.readline())). I won't say this is bad, but certainly not very declarative. Note that itertools.islice can be used to read N lines from a file object -> itertools.islice(f, 10).
Python2: Unless you have a compelling reason, use Python 3.
reduce(lambda.... As much as I like functional style, code that uses reduce with long lambdas usually looks cryptic. But I think it's still ok to use reduce with named functions.
int(f.readline()). Personally I like to give names to values when it's not clear what they stand for. Granted, this may increase the LOC slightly, but I think it's worthwhile. In this case, if we write number_of_queries = int(f.readline()) and later use the variable, it's easy to see what's going on.

As a first refactor, I'd write:

import itertools
import functools

lines = open("input.txt")

# Parse associations
n_associations = int(lines.readline())
associations = {}
for line in itertools.islice(lines, n_associations):
    n, associations_string = line.split(":")
    associations[n] = set(associations_string.split())

# Parse and run queries
n_queries = int(lines.readline())
for line in itertools.islice(lines, n_queries):
    numbers = line.split()
    common = functools.reduce(set.intersection, (associations[n] for n in numbers))
    print(" ".join(common))

Ok, now let's take it a step further. Firstly, there are no functions at all, no modularization, that's no good. Secondly, I prefer a functional approach and there is some imperative stuff going on there. That's how I would write it in a more functional and declarative style:

def parse_and_execute_queries(path):
    def parse_association(line):
        number, associations_string = line.split(":")
        return (number, set(associations_string.split()))

    def execute_query(associations, query_string):
        numbers = query_string.split()
        common = functools.reduce(set.intersection, (associations[n] for n in numbers))
        return " ".join(common)

    lines = open(path)
    n_associations = int(lines.readline())
    association_lines = itertools.islice(lines, n_associations)
    associations = dict(parse_association(line) for line in association_lines)
    n_queries = int(lines.readline())
    queries_lines = itertools.islice(lines, n_queries)
    return (execute_query(associations, line) for line in queries_lines)

for query_result in parse_and_execute_queries("input.txt"):
    print(query_result)


Project Euler Problem 2 in Clojure I am in the process of learning Clojure. I am fairly new to functional programming and would like to know if my code smells or if there are any performance implications with my approach.

; Returns the the given sequence with the given item appended to it.
(defn snoc [xs x] (concat xs [x]))

; Returns the Fibonacci sequence up to the highest number less than max.
(defn fib [max]
  (loop [a 1, b 1, acc [1]]
    (if (> b max)
      acc
      (recur b (+ a b) (snoc acc b)))))

; Project Euler Problem 2: Attempt A
(defn pe2a []
  (reduce +
    (filter even? (fib 4000000))))

For reference:

  Each new term in the Fibonacci
  sequence is generated by adding the
  previous two terms. By starting with 1
  and 2, the first 10 terms will be:

  1, 2, 3, 5, 8, 13, 21, 34, 55, 89, ...

  By considering the terms in the
  Fibonacci sequence whose values do not
  exceed four million, find the sum of
  the even-valued terms.

(defn snoc [xs x] (concat xs [x]))

There is a reason snoc is not defined by default in clojure: Since appending at the end of a singly linked list takes O(n) time, this is actually quite expensive. When building up non-lazy lists tail-recursively in a functional language, you often build the list the wrong way around (using cons instead of snoc) and then reverse it at the end to avoid that cost.

However in this case there is actually a nicer way: by using a lazy sequence rather than a strict list, we can avoid the need for loop/recur and save the cost of building up the list. We can also separate the logic of creating the Fibonacci numbers from the logic which decides how many numbers we want by first creating a lazy sequence containing all Fibonacci numbers and then using take-while to take those less than the given maximum. This will lead to the following code:

;; A lazy sequence containing all fibonacci numbers
(def fibs
  (letfn
    [(fibsrec [a b]
      (lazy-seq (cons a (fibsrec b (+ a b)))))]
    (fibsrec 1 1)))

;; A function which returns all fibonacci numbers which are less than max
(defn fibs-until [max]
  (take-while #(<= % max) fibs))


Find the first reoccuring char from a String Can anyone give me advice on how I can improve my answer?
I have tested it out using "ABCA", "BCABA" and "ABC".
Two things I would like help on.
1.How can I move this code into a method
where it returns the reoccurring char? I know how to return the char once it has been found but what about the case where there is not char what do I return back in that case? Since I cannot return char.
2. Is using the break statement bad practice? I noticed that the code would with "BCABA" both "B" and "A" are printed.

Thanks.

  public static void main(String[] args) {
    String testString = "BCABA";

    Map<Character, Integer> recurringCharsMap = new HashMap<>();
    for (char character : testString.toCharArray()) {
        if (recurringCharsMap.containsKey(character)) {
            System.out.println("Recurring char is: " + character);
            break;
        } else {
            recurringCharsMap.put(character, 1);
        }
    }
}

The Character type already has everything you need. It is either a character or the special value null.

For this task you don't need a map, a simple set is enough.

When you move this code into a separate method, using break becomes bad style because you should directly return. In the currect code it's ok.

public static Character findFirstRecurring(String str, char ch) {
    // Left as an exercise
    …
    return null;
}


Custom checksum algorithm A while back, I reverse-engineered a checksum algorithm from an MMO used to check the validity of an item that's linked to chat (similar to WoW). The idea is that if the checksum is invalid then the game client would ignore the link when clicked. Otherwise clicking on the item link in in-game chat would display the stat and attrib for that item.

ushort16 CreateChecksum(const string &itemlink)
{
    stringstream parseitemlink(itemlink);
    uint32 hexform[ITEMLINKGROUPCOUNT] = {0};
    uint32 hexsum = 0;

    //Parse itemLink string into hexform array
    for (int i = 0; i < ITEMLINKGROUPCOUNT; ++i)
        parseitemlink >> hex >> hexform[i];

    //sum all the itemlink group together
    for (int i = 0; i < ITEMLINKGROUPCOUNT; ++i)
        hexsum += hexform[i];

    for (int i = 0; i < ITEMLINKGROUPCOUNT; ++i)
    {
        uint32 ebx = hexform[i], edi = ebx * i;

        //if loop iteration is odd store MSB 2-bytes.
        //Otherwise, store working hexgroup as is untouched
        if (i & 0x01)
            ebx = hexform[i] >> 16; // aka same as dividing hexform[i] by 65,536

        // (hexform[i] / 65,536) + (hexform[i] * i) + hexsum -- for odd groups
        //  hexform[i] + (hexform[i] * i) + hexsum           -- for even groups
        ebx += edi + hexsum;
        hexsum = ebx ^ hexform[i];
    }

    for (int i = 0; i < ITEMLINKGROUPCOUNT; ++i)
    {
        // the more familiar high-level form would be
        // hexform[i]^2 + hexform[i] * hexsum
        uint32 ecx = (hexform[i] + 1) * hexsum,
               eax = ecx * hexform[i];

        eax >>= 16;
        eax += ecx;
        hexsum = eax ^ hexform[i];
    }

    //return the lower 2-bytes of hexsum
    //as the final checksum
    return hexsum & 0xFFFF;
}//CreateChecksum

The format of the itemlink is comprised of a group of hexadecimal separated with a space in string format. It's passed into main() as an argument when the program is run.

Here's what an itemlink's hex string might look like:

const string EXAMPLELINK = "36c6a 0 3f000a54 d0f1 0 0 0 0 0 0 20d0";

Are there any code smells or readability issues in this code segment? Can any part(s) of it be improved?

If you are using the standard library classes of the same name, I would give the following names the correct namespace qualifier: std::string, std::stringstream, std::hex.

In C++, this works just as well, IMHO it's mildy more idiomatic.

uint32 hexform[ITEMLINKGROUPCOUNT] = {};

ebx, edi, ecx, eax are not good variable names, if you can give them more meaningful names, then do.

    uint32 ecx = (hexform[i] + 1) * hexsum,
           eax = ecx * hexform[i];

Personally, I think this is clearer:

    uint32 ecx = (hexform[i] + 1) * hexsum;
    uint32 eax = ecx * hexform[i];

The comment is really bad because it talks about hexform[i]^2 + hexform[i] * hexsum whereas ecx gets the value hexform[i] * hexsum + hexsum and eax gets the value hexform[i]^2 * hexsum + hexform[i] * hexsum. I think the comment needs a pair of parentheses if the code is doing what you meant.

To be robust, you should check whether the parse worked.

parseitemlink >> hex >> hexform[i];

You can trivially combine the first two for loops as well.


Objective-C class for placing UI elements based on screen size I'm always programmatically creating and laying out UI elements in multiple ViewControllers and I've wanted an easy way to get relevant Screen information (sizes/points) globally using a helper class.

I've created a FlexibleScreen class and would like some constructive feedback.  In addition to the typical code review feedback of format, syntax, etc I would really like to know how well this solves my problem functionally and what suggestions you have about the design or what additional functionality I could add.

FlexibleScreen.h

#import <Foundation/Foundation.h>

@interface FlexibleScreen : NSObject

+(CGSize)getScreenSize;
+(CGPoint)getCenter;
+(CGPoint)getCenterWithY:(double)y;
+(CGPoint)getCenterWithX:(double)x;

@end

FlexibleScreen.m

#import "FlexibleScreen.h"

@interface FlexibleScreen()

@property (assign) CGRect bounds;
@end

@implementation FlexibleScreen

+(CGSize)getScreenSize
{
    return CGSizeMake([[UIScreen mainScreen] bounds].size.width, [[UIScreen mainScreen] bounds].size.height);
}

+(CGPoint)getCenter
{
    return CGPointMake([[UIScreen mainScreen] bounds].size.width/2, [[UIScreen mainScreen] bounds].size.height/2);
}

+(CGPoint)getCenterWithY:(double)y
{
    return CGPointMake([[UIScreen mainScreen] bounds].size.width/2, y);
}

+(CGPoint)getCenterWithX:(double)x
{
    return CGPointMake(x, [[UIScreen mainScreen] bounds].size.height/2);
}

@end

Implementing Code

#import "FlexibleScreen.h"

...

CGRect headerFrame = CGRectMake(0, 0, [FlexibleScreen getScreenSize].width, HEADER_HEIGHT);
CGRect footerFrame = CGRectMake(0, [FlexibleScreen getScreenSize].height-FOOTER_HEIGHT, [FlexibleScreen getScreenSize].width, FOOTER_HEIGHT);

...

[label setCenter:[FlexibleScreen getCenterWithY:0]];

UPDATE:
As I used this more I realized I wanted it to be a lot simpler. Here is my latest version GitHub: UIScreen+Flexible.

Some thoughts on it:

Code: I'd suggest to create a Category on UIScreen. This is even more elegant than creating a separate class.
Naming: In Objective-C getter usually do not have the prefix "get".
General approach: Without knowing the background of your app and any detailed implementation, it's not common to layout UIViews relatively to the screen. As you're using UIViewControllers, any additional view should be added as a subview to the UIViewController's view. This ensures that layout and orientation changes are handled for you and are automatically being applied to all subviews by the UIViewController.


Which one should I choose to work with, Facebook PHP-SDK or Graph Api with Curl? I am new at facebook development using their PHP-SDK and Graph API. I am at the stage that I need to find which road should I take for a better/faster and efficient results.

I display the pages a user administrates in both ways. My question is which one should I choose?
Thank you community.

Using Facebook PHP-SDK

require 'src/facebook.php';
$results = $facebook->api('/fql', array('q'=>'SELECT page_id FROM page_admin WHERE uid = me()'));

    foreach($results['data'] as $result) {
        echo $result['page_id'], '
';
    }

Using CURL & Graph Api

$query = "SELECT page_id FROM page_admin WHERE uid = me()";
$encodedQuery = urlencode($query);

$url = "https://graph.facebook.com/fql?q=".$encodedQuery."&access_token=".$_SESSION['fb_100515270076928_access_token'];
$ch = curl_init($url);
curl_setopt($ch,CURLOPT_RETURNTRANSFER,1);
$results = curl_exec($ch);
$results = json_decode($results);

foreach ($results->data as $result) {
    echo $result->name, '
';
}

I think you'd be better off using the official API wrapper from Facebook. If they ever change their APIs you could download the latest version of their wrapper and continue on.

The best thing you can do for speed is cache the results. PHP can read files on the server a lot faster than downloading the information with CURL.


Regex to parse semicolon-delimited fields is too slow I have a file with just 3500 lines like these:

filecontent= "13P397;Fotostuff;t;IBM;IBM lalala 123|IBM lalala 1234;28.000 things;;IBMlalala123|IBMlalala1234"

Then I want to grab every line from the filecontent that matches a certain string (with python 2.7):

this_item= "IBMlalala123"
matchingitems =  re.findall(".*?;.*?;.*?;.*?;.*?;.*?;.*?"+this_item,filecontent)

It needs 17 seconds for each findall. I need to search 4000 times in these 3500 lines.  It takes forever.  Any idea how to speed it up?

.*?;.*? will cause catastrophic backtracking. See this post on more details on the problem: http://www.regular-expressions.info/catastrophic.html

To resolve the performance issues, remove .*?; and replace it with [^;]*;, that should be much faster.


transform columns to binary encoded columns in R I have some nominal variables encoded as integers (not ordinal), which I would like to encode as binary (not dummies nor one hot!). The following code is what I came up with (adapted from other code I found). Is this a valid/scalable approach? Thanks!
library(binaryLogic)

df <- data.frame(x1 = c(1, 1, 2, 3), x2 = c(1, 2, 3, 4))

encode_binary <- function(x, name = "binary_") {
    x2 <- as.binary(x)
    maxlen <- max(sapply(x2, length))
    x2 <- lapply(x2, function(y) {
        l <- length(y)
        if (l < maxlen) {
            y <- c(rep(0, (maxlen - l)), y)
        }
        y
    })
    d <- as.data.frame(t(as.data.frame(x2)))
    rownames(d) <- NULL
    colnames(d) <- paste0(name, 1:maxlen)
    d
}

df <- cbind(df, encode_binary(df[["x1"]], name = "binary_x1_"))
df <- cbind(df, encode_binary(df[["x2"]], name = "binary_x2_"))

df

If we test on larger vector your approach is quite slow:
test_vec <- 1:1e5
system.time(v1 <- encode_binary(test_vec, name = "binary_x1_"))
#  user  system elapsed
# 22.23    0.08   22.37

Based on this SO question
I managed to write code that performs a lot faster:
encode_binary2 <- function(x, name = "binary_") {
  m <- sapply(x, function(x) rev(as.integer(intToBits(x))))
  tm <- t(m)
  # remove empty bit cols
  i <- which(colSums(tm) != 0L)[1]
  tm <- tm[, i:ncol(tm)]
  # save to data.frame
  d <- as.data.frame(tm)
  rownames(d) <- NULL
  colnames(d) <- paste0(name, 1:ncol(d))
  d
}

system.time(v2 <- encode_binary2(test_vec, name = "binary_x1_"))
# user  system elapsed
# 0.61    0.02    0.63

# test that results are equal:
all.equal(v1, v2)
# [1] TRUE


Assist cleaning up messy HTML/PHP code I really hate messy code, but I seem to be rolling in it. I just want to see if anyone has some helpful advice on how to tidy this up.

Zona
Baptismos Mulheres
Conf. Mulheres
Batismos Homems
Conf. Homems

<?php
$currWeek = $CONF->getSetting('current_week');
$zones = $PDO->query("SELECT * FROM zones WHERE active = 1");
foreach($zones as $zone) {
$zoneName = $zone['name'];
$zoneUid = $zone['uid'];
?>

<?php print $zoneName; ?>

<?php

$areas = $PDO->query("SELECT * FROM areas WHERE zone = '".$zoneUid."'");
foreach($areas as $area) {
$areaName = $area['name'];
$areaUid = $area['uid'];
?>

<?php print $areaName; ?>

<?php

$missionaries = $PDO->query("SELECT missionary FROM missionary_areas WHERE semana = '".$currWeek."' AND area_uid = '".$areaUid."'");

$compArr = array();

foreach($missionaries as $missionary) {
$companions = $PDO->query("SELECT CONCAT(title, ' ', mission_name) as name from missionarios WHERE mid = '".$missionary['missionary']."'");

foreach($companions as $comp) {
$compArr[] = $comp['name'];
}

}

$compList = $CONF->commaSeparate($compArr);
?>
<?php print $compList; ?>

<?php
}
}

?>

I am still a bit inexperienced and would like some helpful tips. My MySQL directory stucture is necessaraly crazy so I have to do a bunch of queries to get some simple data. Using PDO is helping quite a bit though.

Edit:

Database structure

WKS is right, first of all you should split your layout from your logic and your logic from your database query. (see Model-View-Controller-Pattern)

Lets start with your model (Model.php): (actually you could split it in a model for each entity/database table)

<?php
class Model()
{
     public function __construct(PDO $connection)
     {
         $this->connection=connection;
     }

     public function getZones($week)  //find a nicer name
     {
         //... see Peters post for a sample join
         // prepare multi dimensional array as needed in your template
         return $zones;
     }
}

It is in general a bad idea to run SQL queries in a loop, so try to create one join for all your queries.

Now the simple Controller (index.php)

<?php
require 'Model.php';
$connection=new PDO(...);
$model=new Model($connection);
$currentWeek = ...;
$zones= $model->getZones($currentWeek);

include "view.php"

That's it! Nothing fancy. Now the view:

         Zona
         Baptismos Mulheres
         Conf. Mulheres
         Batismos Homems
         Conf. Homems

    <?php foreach($zones as $zone):?>

         <?php echo $zone['name']; ?>

        <?php foreach($zones['areas'] as $area):?>

                 <?php $area['name']; ?> <?php echo $area['companions']; ?>

        <?php endforeach?>
    <?php endforeach?>

Don't use all-capital-letter variable names. And don't use global variables. Both is considered bad practice.


Returning a string representation of the number of albums and songs available What this method should do is return a String that displays how many albums and songs there are. This method is a private method of my Artist class which has a method called albumCount() and songCount(). The whole class probably needs an improvement but I will try that later. First I'd like this single method to be improved if possible.

public String toAlbumSongCount() {
    StringBuilder builder = new StringBuilder();

    final int albumCount = albumCount();
    final int songCount = songCount();

    builder.append( albumCount );
    //Append albums instead of album if albumCount is greater than 1
    builder.append( (albumCount > 1) ? " albums" : " album" );
    builder.append( "," ); //Move this to the line above?
    builder.append( songCount );
    //Append songs instead of song if songCount is greater than 1
    builder.append( (songCount > 1 ) ? " songs" : " song" );

    return builder.toString();
}

I used the StringBuilder class because I heard a dozen of times that it will improve performance when concatenating Strings.
I wasn't sure about those two lines :

builder.append( (albumCount > 1) ? " albums" : " album" );
builder.append( (songCount > 1 ) ? " songs" : " song" );

Are they good enough with the comments above them?

I appreciate any help even if it means completely rewriting this method.

Text manipulation like that is always a pain. I like to extract functions for things like this, though.

private static final String simplePlural (int count, String noun) {
    if (count == 1) {
        return "1 " + noun;
    }
    return count + " " + noun + "s";
}

With that function, your code becomes:

public String toAlbumSongCount() {
    return simplePlural(albumCount(), "album")
       + ", " + simplePlural(songCount(), "song");
}

I know there's no StringBuilder in there, but the performance benefit for something that's seldom used, will be negligible, and this is more readable.

Note that the code above adds a space after the comma, which your code is missing.


Generating Random RGB Colors I have a function for generating RGB colors in VB6.0, and it is working properly.  Please suggest improvements for this, if any.

Public Function RandomRGBColor() As Long
    RandomRGBColor = RGB( _
        Int(Rnd() * 256), _
        Int(Rnd() * 256), _
        Int(Rnd() * 256))
End Function

You need to change/add just one little thing. You need to call the Randomize () method before using the Rnd () method. You should only call it once, so it would be good, to call the Randomize() method inside of the constructor of your class like:

Private Sub Class_Initialize()
    Randomize
End Sub

If you don't do this you will get for each iteration the same "random" number.
Don't get me wrong, this doesn't mean that all numbers will be the same but assume you start your application. For example you will get the RGB numbers 11, 54, 142 for the first call of your method. If you restart your application you will again get these same numbers on the first call.

To prevent this, you need to 'seed' the Rnd () method to get random values each time.


Testing a service that makes database calls, without a database I'm working on a new service, for the past year I haven't needed to write tests from scratch. I've either been bug fixing, or adding features that require modification of tests, but not writing everything from scratch.
I need to make sure I'm doing things right. The unit tests below test a service that queries a database for data.

The *Proc files use Springs @Repository annotation, and each one calls a different stored procedure in the database. For these tests I'm mocking everything. Have I done it correctly? Is there anything I have overlooked? Anything I've possibly missed? The tests all pass when I run them in my IDE or with maven. In this particular instance those method calls simply return the object(s) retrieved from the database, so there isn't any actual business logic in there apart from calling the get() method from the proc objects that I'm mocking.
Have I done things the correct way?

    @RunWith(MockitoJUnitRunner.class)
    @SpringBootTest
    public class FusServiceImplTest {

        @Mock
        private GetFileInfoByIdProc getFileInfobyIdProc;
        @Mock
        private GetFileInfoProc getFileInfoProc;
        @Mock
        private GetFileInfoByFileNameProc getFileInfoByFileNameProc;
        @Mock
        FileInfoVO voMock;
        @Mock
        FileInfoVO voMock2;

        @InjectMocks
        private FusServiceImpl service;

        @Test
        public void whenGetFileById_thenReturnFile(){
            final Long ID = 99999L;

            when(getFileInfobyIdProc.get(ID)).thenReturn(voMock);

    FileInfoVO voResult = service.getFile(ID);

        assertThat(voResult.getFileName())
                .isEqualTo(voMock.getFileName());

    }

    @Test
    public void whenGetFileByName_thenReturnFile(){
        final String NAME = "test.file";

        when(getFileInfoByFileNameProc.get(NAME)).thenReturn(voMock);

        FileInfoVO voResult = service.getFile(NAME);

        assertThat(voResult.getFileName())
                .isEqualTo(voMock.getFileName());

    }

    @Test
    public void whenGetAllFiles_thenReturnAllFiles(){

        List<FileInfoVO> list = new ArrayList<>();
        list.add(voMock);
        list.add(voMock2);

        when(getFileInfoProc.get()).thenReturn(list);

        List<FileInfoVO> voListResult = service.getAllFiles();

        assertThat(voListResult.size())
                .isEqualTo(list.size());

    }

}

Something that stands out is that calling voMock.getFileName() on mocked instances will always return null as no mock response has been defined. So your validation logic of

assertThat(voResult.getFileName())
            .isEqualTo(voMock.getFileName());

essentially winds up being

assertThat(voResult.getFileName())
            .isEqualTo(null);

This could end up giving false positives where voResult is incorrect, but the test passes because the filename is null.

You would need to specify a filename for the response object by calling when(voMock.getFileName()).thenReturn("TestFile"). This way your test can actually determine that the returned object is the expected one by checking the unique expected value of voMock.getFileName().

Alternatively, if the requirement is that the instance returned from the service method is the same instance that the *Proc method returns, just avoid calling getFileName when you're verifying the test result. You could instead use something like assertSame(voMock, voResult) to ensure that the instance returned by the service method is the same one that is returned by the *Proc method.

I would always also recommend writing fail/error cases for your class as well. I couldn't give an example without seeing the FusServiceImpl class, but it can sometimes help with spotting issues like the one I listed above. For instance, if you have a test case like

@Test
public void whenGetFileById_thenReturnFile(){
    final Long ID = 99999L;

    when(getFileInfobyIdProc.get(ID)).thenReturn(voMock);

    FileInfoVO voResult = service.getFile(ID - 1);

    assertThat(voResult.getFileName())
            .isNotEqualTo(voMock.getFileName());

}

and that test case passes, you know you've probably made a mistake in the test logic. You can also validate that the method fails in the correct manner - does it return an object with null filename? A null return value? Or raise an exception? It's the non-ideal cases that will crash applications in production, so make sure to test for those cases as well.


benchmarking sorting algorithms: std::sort vs my naive radix I'm comparing performance of two sorting algorithms applied to sort integers using google benchmark.
I'm quite surprised by the results so wanted to ask if you see some mistakes in the way I measure the performance. I would expect performance of my naive radix sort to be much worse that std::sort because it is supposed to be bandwidth bound.
#include <benchmark/benchmark.h>

#include <random>
#include <vector>
#include <array>
#include <algorithm>

#define TEST_SIZE DenseRange(50000, 1000000, 50000)

class SortingBmk : public benchmark::Fixture
{
public:
    using T = int32_t;
    std::vector<T> m_vals;

    void SetUp(const ::benchmark::State& state)
    {
        const auto n = state.range(0);
        m_vals.resize(n);
        std::iota(m_vals.begin(), m_vals.end(), 0);

        std::random_device rd;
        std::mt19937 g(rd());
        std::shuffle(m_vals.begin(), m_vals.end(), g);
    }

    void TearDown(const ::benchmark::State& state) {}
};

BENCHMARK_DEFINE_F(SortingBmk, StdSort)(benchmark::State& state)
{
    const auto n = state.range(0);

    std::vector<T> values(m_vals.size());
    for (auto _ : state)
    {
        std::copy(m_vals.begin(), m_vals.end(), values.begin());

        std::sort(values.begin(), values.end());
        benchmark::DoNotOptimize(values);
        benchmark::ClobberMemory();
    }
}
BENCHMARK_REGISTER_F(SortingBmk, StdSort)->Unit(benchmark::kMicrosecond)->TEST_SIZE;

inline int getBits(SortingBmk::T v, SortingBmk::T i) { return (v >> i)&0b11; }
void radixSort_count(std::vector<SortingBmk::T>& data, std::vector<SortingBmk::T>& buf) {
    const int n = data.size();
    std::array<int, 8> psum = {0};
    constexpr auto sz = sizeof(SortingBmk::T) * 8 - 2;
    for (SortingBmk::T i = 0; i < sz; i += 2) {
        //count sort
        for (int v : data) {
            auto bits = getBits(v, i);
            ++psum[bits];
        }
        for (int i = 1; i < 8; ++i)
            psum[i] += psum[i-1];
        for (int j = n - 1; j>= 0; --j) {
            auto bits = getBits(data[j], i);
            --psum[bits];
            assert(psum[bits] < buf.size());
            buf[ psum[bits] ] = data[j];
        }
        swap(buf, data);
        psum = {0};
    }
}

BENCHMARK_DEFINE_F(SortingBmk, NaiveRadixSort)(benchmark::State& state)
{
    const auto n = state.range(0);

    std::vector<T> values(m_vals.size());
    std::vector<T> buffer(m_vals.size());
    for (auto _ : state)
    {
        std::copy(m_vals.begin(), m_vals.end(), values.begin());

        radixSort_count(values, buffer);
        for (int i = 1; i < values.size(); ++i)
            assert(values[i-1] < values[i]);
        benchmark::DoNotOptimize(values);
        benchmark::ClobberMemory();
    }
}
BENCHMARK_REGISTER_F(SortingBmk, NaiveRadixSort)->Unit(benchmark::kMicrosecond)->TEST_SIZE;

BENCHMARK_MAIN();

UPD: X-axis is number of elements in the array, Y-axis is time spend on computations in average for all the times I run the code within for loop. It is in kMicrosecond.

I'm not too surprised to see the radix sort being faster in this use case, 32 bits is very short for a key.
You may want to look at this comparison as well as at the last paragraph of this History section.
About the bench method, I would:

Try with values already sorted.
Try with values sorted in reverse.
Try with multiple sets of random values, each set identified by its seed (so no random seed).
Try with the same as above but with the first half filled with random values and the second half being a copy of the first half.

I would also test the end result, of course.


Is it correct to extend a bean class and write a class which holds logic to populate the bean we extend? I need to extract some metrics from ManagedBeans and store the collected metrics in an object bean.

The object bean is defined with member variables to hold the metrics. In this case, is it right to extend the ObjectBean and write the extractor class?

For example:

class ThreadMetricsBean {
    private long liveCount, daemonCount;

    public void record(long liveCount, long daemonCount) {
        this.liveCount = liveCount;
        this.daemonCount = daemonCount;
    }
}

// Here to avoid code redundancy I feel it's correct to extend the  ThreadMetricsBean class
// Am I wrong ??

class ThreadMetricsExtractor {
    private long liveCount, daemonCount;
    private ThreadMetricsBean bean = new ThreadMetricsBean();

    public void run() throws Exception{
        extractMetrics();
        recordToBean();
    }

    private void extractMetrics() {
        ThreadMXBean threadMXBean = ManagementFactory.getThreadMXBean();
        this.liveCount = threadMXBean.getThreadCount();
        this.daemonCount = threadMXBean.getDaemonThreadCount();
    }

    private void recordToBean() {
        bean.record(liveCount, daemonCount);
    }
}

I would declare variables on separate lines.

private long liveCount, daemonCount;

From Code Complete, 2nd Edition, p761:

  Use only one data declaration per line

  [...]

  It’s easier to modify declarations because each declaration is self-contained.

  [...]

  It’s easier to find specific variables because you can scan a single
  column rather than reading each line. It’s easier to find and fix
  syntax errors because the line number the compiler gives you has
  only one declaration on it.

The code has a temporal coupling here:

public void run() throws Exception{
    extractMetrics();
    recordToBean();
}

A maintainer easily can change the order of the called methods which brokes it. See: Clean Code by Robert C. Martin, G31: Hidden Temporal Couplings, p302
For this:

// Here to avoid code redundancy I feel it's correct to extend
// the  ThreadMetricsBean class
// Am I wrong ??

It's hard to say that which would be better without knowledge about clients and typical/possible usages of these classes. You might be able to remove redundancy in the following way too:

class ThreadMetricsExtractor {
    private final ThreadMetricsBean bean = new ThreadMetricsBean();

    public void run() throws Exception {
        storeMetrics();
    }

    private void storeMetrics() {
        final ThreadMXBean threadMXBean = ManagementFactory.getThreadMXBean();
        final int liveCount = threadMXBean.getThreadCount();
        final int daemonCount = threadMXBean.getDaemonThreadCount();
        bean.record(liveCount, daemonCount);
    }
}

I've found that composition usually leads better design, cleaner and testable code than inheritance. A reference:  Effective Java, Second Edition, Item 16: Favor composition over inheritance.
The throws Exception is unnecessary here, as far I see nothing throws any checked exception inside the run method:

public void run() throws Exception{
    extractMetrics();
    recordToBean();
}

You could use integers here:

this.liveCount = threadMXBean.getThreadCount();
this.daemonCount = threadMXBean.getDaemonThreadCount();

Both called methods returns int, so liveCount and daemonCount could be ints too.


Simple port scanner in Python 3 I wrote a simple python port scanner today and I would like to get some advice on how to improve on the code in different ways.

I am aware that I can implement threading to reduce runtime but I won't for now as it feels a bit advanced at this time. Rather I would like tips/opinions on how to improve the program in other ways.

You can find the code on my GitHub.

I am aware that the logging is a bit redundant as it doesn't log anything now, I simply forgot to remove it.

# Network port scanner
# Focus first will be on making the functionality of the software.
# Second focus will be on lowering the runtime of the software.

import socket
import logging
import time

class SConnect:

    def __init__(self, ip, port=None):
        self.ip = ip
        self.port = port
        self.address = (self.ip, self.port)
        self.s_connection = socket.socket(socket.AF_INET,       socket.SOCK_STREAM)
        self.s_connection.settimeout(0.3)

    def portscan(self):

        return self.s_connection.connect_ex(self.address)

def main():

    logging.basicConfig(filename="errlog.log", format="%(asctime)s : %(message)s")
    logging.info("Start")
    print("\nHello user and welcome to Network Port Scanner!")
    print("Please insert a IP address that you want to scan for open and     closed ports.")
    print("The range of ports scanned is 1-65535.")
    u_ip = input("\nTarget IP: ")

    open_pcounter = 0
    closed_pcounter = 0

    if u_ip is not None:
        for p in range(1, 65536):
            start_ptime = time.time()
            c = SConnect(u_ip, p)
            if c.portscan() == 0:
                print("Port {} is open".format(p))
                open_pcounter += 1
            else:
                print("Port {} is closed".format(p))
                closed_pcounter += 1
            print("--- %s seconds ---" % (time.time() - start_ptime))
    else:
        print("You failed, terminating.\n")

    print("Total open ports:%s".format(open_pcounter))
    print("Total closed ports:%s".format(closed_pcounter))
    logging.info("Finished")

if __name__ == '__main__':
    start_time = time.time()
    main()
    print("--- %s seconds ---" % (time.time() - start_time))

Connections

You don't close your sockets and connections. The best way to ensure you close them is with a context manager (with-statement)

You can also reuse the socket you make, and connect it to other ports. This would make a SocketConnection:

class SocketConnection:
    def init(self, ip, port=None):
        self.socket = None
        self.ip = ip

    def __enter__(self):
        self.socket = socket.socket(socket.AF_INET, socket.SOCK_STREAM)

    def __exit__(self, type, value, traceback):
        self.socket.close()
        self.socket = None

    def portscan(self, port):
        if self.socket is None:
            raise ConnectionError('No socket open')
        try:
            return not self.socket.connect_ex((self.ip, port))
        finally:
            self.socket.detach()

This can be used like this:

def scan_ports(ip, port_range):
    """yields the open ports in `port_range` (port_range is half-open) """
    with SocketConnection(ip) as connection:
        for port in range(*ports):
            if connection.portscan(port):
                yield port

Validate input

You don't validate the input of your client. To do this, you can do something like this:

def validate_ip(ip):
    return True # needs implementation
def get_ip():
    while True:
        ip = input("Target IP: ")
        if validate_ip(ip):
            return ip

You can do ctrl+C to get out of this while True loop

and then the main() function:

def main():
    logging.basicConfig(filename="errlog.log", format="%(asctime)s : %(message)s")
    logging.info("Start")
    message = """
    Hello user and welcome to Network Port Scanner!
    Please insert a IP address that you want to scan for open and closed ports.
    The range of ports scanned is 1-65535.
    """
    print(message)
    ip = get_ip()
    open_ports = list(scan_ports(ip, (1, 65536)))

    print(
        f"""open ports: ({len(open_ports)})
        {open_ports}""")

Caveat: I don't know a lot about socket programming. I just used the python documentation, so there might be socket-related bugs


Writing a Python class that provides default instance succinctly I have a Python class for which I'm often going to want an instance with the default arguments, but I want the class to allow creation of instances. For brevity I'd prefer that the default instance be accessible as a property, so that it can be accessed without a method call. However, the code to achieve this in the way I know how is complicated (see below); it feels too complicated for such a simple task. The solution that suggests itself to me is to give up on using properties (in my context a multiton would even work), but I wonder: is there a better way?

# https://stackoverflow.com/a/1383402/2829764
class ClassProperty(property):
    def __get__(self, cls, owner):
        return self.fget.__get__(None, owner)()

class Example:
    _default_instance = None

    def __init__(self, customizable=0):
        self.customizable = customizable

    def get_default_instance(cls):
        if cls._default_instance is None:
            cls._default_instance = cls()
        return cls._default_instance
    def set_default_instance(cls):
        raise NotImplementedError("Can't do this")
    default_instance = ClassProperty(classmethod(get_default_instance), classmethod(set_default_instance))

    def do_something(self):
        print("*** {} ***".format(self.customizable*2))

foo = Example(10)
foo.do_something()

foo.default_instance.do_something()

Output:

*** 20 ***
*** 0 ***

NOTE: I don't care about default_instance being read-only; that was just a byproduct of the ClassProperty code I used from https://stackoverflow.com/a/1383402/2829764.

I'd say a much easier way to accomplish this is to use a module instead of a class.  In a language like Java the primary form of encapsulation is a class, while in Python, the primary form of encapsulation is a module.  For example,

foo.py

class Example:

    def __init__(self, customizable):
        self.customizable = customizable

    def do_something(self):
        print("*** {} ***".format(self.customizable*2))

default_instance = Example(0)

main.py

import foo

my_foo = foo.Example(20)
my_foo.do_something()

foo.default_instance.do_something()

Output:

*** 20 ***
*** 0 ***

If you really wanted the default instance to be a class level attribute of Example, then you could add this line to foo.py

Example.default_instance = Example(0)

Personally, I don't think making this read only adds that much value; in general we let people do what they want in Python (that's why we don't have language defined private/protected/internal/public/etc features).  If the user wants to mess with something they shouldn't, that's on them.


Powershell Script to remove printers and drivers I am learning Powershell Scripting as I move my work environment to Windows 10. The script below is to be edited by the SysAdmin (for the list of Print Queues and Printer Drivers) to specify what needs to be removed, then outputs what it did. I am looking for whether the Powershell syntax / convention is followed. This script will be distributed to other SysAdmins in my team when completed.

The script currently starts by declaring variables that will be used throughout the script. The variables tell the script which print queues to remove, and what print drivers to remove. It then loops through the arrays, removing the printers requested.

# Remove old printers by name - Written by Luke Barone
#
# VARIABLES TO EDIT
# $PrintersToRemove - (REQUIRED) An array of printers to cycle through and
#     remove. Use the exact name of the printer to have it included in the list
# $PrintDriversToRemove - (optional) An array of printer drivers to cycle
#     through and remove. You can obtain a list by running the "prndrvr.vbs -l"
#     command on a computer with drivers to remove. Alternatively, you can
#     leave the array empty, and the script will remove any unused (determined
#     by the system) print drivers automatically.
$PrintersToRemove = @("Printer1", "Printer2")
$PrintDriversToRemove = @("Manufacturer1", "Manufacturer2")

# The variables below should only be changed from the default if the scripts
# are not in this location!
$PrintingAdminScriptsFolder = "C:\Windows\System32\Printing_Admin_Scripts\en-US"
$PrintScriptFiles = @("prnmngr.vbs", "prndrvr.vbs")

# SANITY CHECKS
If ($PrintersToRemove.Length -eq 0) {
    Write-Host "Required variable not filled! Please edit the script and re-run"
    Exit 1
}
If (($PrintScriptFiles | ForEach {Test-Path ($PrintingAdminScriptsFolder + "\" + $_)}) -contains $false) {
    Write-Host "Required scripts not found in specified folder! I need the two
    VBS scripts ($(PrintScriptFiles)) inside of $($PrintingAdminScriptsFolder)
    in order to function!"
    Exit 2
}

# SCRIPT START
Write-Host "Starting at $(Get-Date -Format g) on client $($env:COMPUTERNAME)"
C:\Windows\System32\cscript.exe //H:CSCRIPT //S | out-null
cd $PrintingAdminScriptsFolder
Write-Host ""
Write-Host "Removing selected printers..."
$PrintersToRemove | foreach {
    & .\prnmngr.vbs -d -p "$_".ToString() | findstr "0x80041002" | out-null
    If ($LASTEXITCODE -eq 0) {
        Write-Host "    $_ does not exist"
    } else {
        Write-Host "    $_ removed"
    }
}
Write-Host ""

If($PrintDriversToRemove.count -gt 0) {
    Write-Host "Removing selected Printer Drivers..."
    $PrintDriversToRemove | foreach {
        & .\prndrvr.vbs -d -m "$_" -v 3 -e "Windows x64".ToString() | findstr "0x80041002" | out-null
        If ($LASTEXITCODE -eq 0) {
            Write-Host "    $_ does not exist"
        } else {
            Write-Host "    $_ driver removed"
        }
    }
} else {
    Write-Host "Removing excess Printer Drivers..."
    .\prndrvr.vbs -x
}

Before relying on old VBS scripts, it pays to check what native PowerShell tools you have available:

PS> help printer

Name                              Category  Module                    Synopsis
----                              --------  ------                    --------
Add-Printer                       Function  PrintManagement           Adds a printer to the specified computer.
Add-PrinterDriver                 Function  PrintManagement           Installs a printer driver on the specified computer.
Add-PrinterPort                   Function  PrintManagement           Installs a printer port on the specified computer.
Get-Printer                       Function  PrintManagement           Retrieves a list of printers installed on a computer.
Get-PrinterDriver                 Function  PrintManagement           Retrieves the list of printer drivers installed on the specified computer.
Get-PrinterPort                   Function  PrintManagement           Retrieves a list of printer ports installed on the specified computer.
Get-PrinterProperty               Function  PrintManagement           Retrieves printer properties for the specified printer.
Read-PrinterNfcTag                Function  PrintManagement           Reads information about printers from an NFC tag.
Remove-Printer                    Function  PrintManagement           Removes a printer from the specified computer.
Remove-PrinterDriver              Function  PrintManagement           Deletes printer driver from the specified computer.
Remove-PrinterPort                Function  PrintManagement           Removes the specified printer port from the specified computer.
Rename-Printer                    Function  PrintManagement           Renames the specified printer.
Set-Printer                       Function  PrintManagement           Updates the configuration of an existing printer.
Set-PrinterProperty               Function  PrintManagement           Modifies the printer properties for the specified printer.
Write-PrinterNfcTag               Function  PrintManagement           Writes printer connection data to an NFC tag.
Out-Printer                       Cmdlet    Microsoft.PowerShell.U... Sends output to a printer.

So your script reduces to something like this:

$PrintersToRemove = @("Printer1", "Printer2")
$PrintDriversToRemove = @("Manufacturer1", "Manufacturer2")

Remove-Printer -Name $PrintersToRemove -WhatIf

$drivers = Get-PrinterDriver | ? Manufacturer -in $PrintDriversToRemove

foreach ($driver in $drivers)
{
    Remove-PrinterDriver -Name $driver.Name -WhatIf
}

Note that I didn't put in the removing excess printer driver logic. I'm not exactly sure what that does.

Note also the -WhatIf parameters. They stop the commands from actually executing. You would remove those when you are satisfied that the script will do what you want it to do.

Instead of putting comments at the top of the script, you can use the PowerShell help system:

<#
.SYNOPSIS
  Remove old printers by name

.DESCRIPTION
  [Add stuff here if you like.]

.PARAMETER PrintersToRemove
  (REQUIRED) An array of printers to cycle through and
  remove. Use the exact name of the printer to have it included in the list

.PARAMETER PrintDriversToRemove - (optional) An array of printer drivers to cycle
  through and remove. You can obtain a list by running the "prndrvr.vbs -l"
  command on a computer with drivers to remove. Alternatively, you can
  leave the array empty, and the script will remove any unused (determined
  by the system) print drivers automatically.

.NOTES
  Written by Luke Barone

.EXAMPLE
  Example 1

.EXAMPLE
  Example 2
#>

That way you can do a help MyScript -detail

It's probably a good idea to use parameters to the script rather than getting people to edit it:

[CmdletBinding()]
param(
    [Parameter(Mandatory=$true)]
    [string[]]$PrintersToRemove,

    [Parameter(Mandatory=$false)]
    [string[]]$PrintDriversToRemove
    )

That way if you update the script, you can just copy it over the old one. The users don't have to merge them. You would make another script that contains the actual parameters, and that one would call this one.

One last thing, I like to put the following near the top of my scripts. It helps to avoid some bugs.

Set-StrictMode -Version Latest
$ErrorActionPreference = 'Stop'