Basic Start of a React App I'm new to react and am building a basic react app and have just built a basic area where navigation links render on a page but wonder whether my current start could be optimised - for example, I wonder whether an area like this:
render() {
    return (
        <Root navLinks={this.state.navLinks}></Root>
    );
  }

Could be changed to something like this:
render() {
    return (
        <Root>
           <NavLinks navLinks={this.state.navLinks} />
        </Root>
    );
  }

And I wonder about other aspects of the app such as do I need an App.js file or index.js file and could have one rather than the other?
Any quick basic beginner advice would be much appreciated. The code can be better found in this Stackblitz demo or below:
App.js:
import React, {Component} from "react";
import {navLinks} from "./components/nav-links";
import Root from "./components/Root";

export default class App extends Component {
  constructor() {
    super();

    this.state = {
      navLinks: navLinks
    }
  }

  render() {
    return (
        <Root navLinks={this.state.navLinks}></Root>
    );
  }
}

index.js:
import React from 'react';
import ReactDOM from 'react-dom';
import App from './App';

ReactDOM.render(
  <App />,
  document.getElementById('root')
);

/components/Root.js:
import React from "react";
import NavMenu from "./nav-menu";
import BodyCode from "./body-code";

export default class Root extends React.Component {
    render() {
        return (

                    <NavMenu navLinks={this.props.navLinks} />

                    <BodyCode />

        )
    }
}

/components/body-code.js:
import React from "react";

export default class bodyCode extends React.Component {
    constructor(props) {
        super(props);
        this.state = {isToggleOn: true}; // boolean for event listener alternative

        // This binding is necessary to make `this` work in the callback
        this.handleClick = this.handleClick.bind(this);
      } // bind is saying make the ‘this’ keyword bind(this), where the 2nd ‘this’ is the ‘this’ of the constructor

      handleClick() {
        this.setState(state => ({
          isToggleOn: !state.isToggleOn // sets state to the opposite of whatever it is
        }));
      }

  render() {
    return (

some body text

          <button onClick={this.handleClick}>
            {this.state.isToggleOn ? 'ON' : 'OFF'}
            </button>

    )
  }
}

/components/nav-links.js:
export const navLinks = [
  {
    name: "home",
    href: "/"
  },
  {
    name: "subs",
    href: "/subs"
  }
];

/components/nav-menu.js:
import React from "react";

export default class navBar extends React.Component {
  render() {
    return (
      this.props.navLinks.map((link) =>  {link.name} )
    )
  }
}

Questions

Should Root take props or wrap children components?

<Root navLinks={this.state.navLinks}></Root>

vs
<Root>
  <NavLinks navLinks={this.state.navLinks} />
</Root>

I think this primarily comes down to the purpose of Root. If it is a general purpose wrapper container component, then the latter pattern makes sense. I.E. it is providing the layout but no actual UI, this works well. If on the other hand it isn't very generalizable, i.e. it is more single purpose, then passing it props is preferred. I understand this doesn't sound like an answer, but like many things in javascript, there's no clear, hard line between them and this could also simply come down to personal preference/opinion.

Do I need an App.js file or index.js file and could have one rather than the other?

Again, these both exist by convention, but you could easily merge the two if you so desired. The distinction basically comes down to separation of concerns. index.js is primarily concerned with rendering the application into the DOM whereas App.js is primarily concerned with being the root application container. This is typically where redux stores & providers, react contexts & providers (well, all "providers" in general), root routers/navigation, theming, etc, etc... would/could reside and provide functionality to the app.
Code Review
App.js

For such simple components (no state updates or no state, no lifecycle functions, etc..) it is recommended to instead use functional components
There is no compelling reason to store the static link data in react state, just pass it directly to props
navLinks isn't a component so it probably shouldn't be stored in that directory, better suited for a utility directory or similar
Root doesn't wrap any children components so it can be self-closed

Suggestion
import React from "react";
import { navLinks } from "./components/nav-links";
import Root from "./components/Root";

export const App = () => <Root navLinks={navLinks} />;

/components/Root.js

Many of the same comments as for App.js
If the element ids are actually used (CSS, testing) then keep them, otherwise they just clutter the JSX
BodyCode is already wrapped in a div, so the div here is extraneous and helps increase clutter in the DOM

Suggestion
import React from "react";
import NavMenu from "./nav-menu";
import BodyCode from "./body-code";

export const Root = ({ navLinks }) => (

      <NavMenu navLinks={navLinks} />

    <BodyCode />

);

/components/body-code.js

By convention react component names are PascalCased
handleClick can be defined as an arrow function, which would make binding this to it in the constructor unnecessary
Great job on using a functional state update to toggle the isToggleOn state value
While not a necessity, the constructor can be removed and state defined as a class property
Also not a necessity, this component is still simple enough and doesn't utilize any lifecycle functions that it is also a good candidate for conversion to functional component

Class-based suggestion
import React, { Component } from "react";

export class BodyCode extends Component {
  state = {
    isToggleOn: true,
  };

  handleClick = () => this.setState(
    prevState => ({ isToggleOn: !prevState.isToggleOn })
  );

  render() {
    return (

some body text

        <button onClick={this.handleClick}>
          {this.state.isToggleOn ? 'ON' : 'OFF'}
        </button>

    )
  }
}

Functional component suggestion
import React, { useState } from "react";

export const BodyCode = () => {
  const [isToggleOn, setIsToggleOn] = useState(true);

  const handleClick = () => setIsToggleOn(on => !on);

  return (

some body text

      <button onClick={handleClick}>
        {isToggleOn ? 'ON' : 'OFF'}
      </button>

  );
};

/components/nav-menu.js

Should follow react component naming convention
Simple and should probably be converted to functional component

Suggestion
import React from "react";

export const NavBar = ({ navLinks }) => navLinks.map(link => (

    {link.name}

));

Other Suggestions
Use a routing/navigation solution versus using anchor () tags as these actually reload the app. These typically provide all the functionality of links and navigating around your app by manipulating the browser's history stack but won't reload the page between navigations.
react-router/react-router-dom is a common routing solution. The idea being you define Routes, wrapped in a Router/Switch, to render each "page" of your app, and instead of anchor tags a Link is used to go to a specified path.
A sample router in BodyCode
import { BrowserRouter as Router, Route } from 'react-router-dom';

...

some body text

  <button onClick={this.handleClick}>
    {isToggleOn ? 'ON' : 'OFF'}
  </button>
  <Router>
    <Route path="/subs" component={Subs} />
    <Route path="/" component={Home} />
  </Router>

NavMenu
import { Link } from 'react-router-dom';

...

navLinks.map(link => (
  <Link key={link.href} to={link.href}>
    {link.name}
  </Link>
))


Whitelist input data for a lookup by name I'm whitelisting the input data to an LDAP/AD search on 'sn' (=surname). The purpose is to stop penetration attacks through the app and into the LDAP servers (i.e. no brackets, please).

// whitelist input data.
string partialName = Regex.Replace(partialName, "[^. a-zA-Z-']", string.Empty);

Do you think this is a useful subset? I don't want to allow anything bad through but it does need to be usable.

UPDATE

Instead of whitelisting I should be escaping characters (@200_success):

// escape LDAP filter string characters.
partialName =
    partialName
    .Replace(@"\", @"\5c")
    .Replace(@"*", @"\2a")
    .Replace(@"(", @"\28")
    .Replace(@")", @"\29");

This works against my LDAP server. I can't pass an asterisk anyway as the interface is a REST url and that causes a Request.Path exception.

Don't ever make assumptions of what to allow or disallow based on what you have seen in your experience.  Always consult the specification, which is, in this case, RFC 4515 Sec 3.  The spec will tell you which characters require escaping, and what the escaping mechanism is.


Find first unique character in string I intentionally avoided Python tools which would make this even more trivial.  I would like to be reviewed on efficiency, style, and obviously if there is a bug I would like to know.

def unique_char(s):
  ''' Find first non-repeated char in a string '''
  myDict = {}
  first = len(s)
  for c in s:
    if c in myDict.keys():
      myDict[c] += 1
    else:
      myDict[c] = 1
  if 1 in myDict.values():
    for k,v in myDict.items():
      if v == 1:
        if s.index(k) < first:
          first = s.index(k)
    return s[first]
  return(False)

The docstring is not quite right: "Find first non-repeated char in a string" — and then what? It should say explicitly that it returns the character.
The function returns False if all characters are repeated. This is a bad idea: it would be easy for the caller to forget to check. It's better to raise an exception in exceptional cases.
myDict is an uninformative choice of variable name. This dictionary contains counts of characters, so it should have name like counts or character_counts.
c in myDict.keys() can be simplified to c in myDict.
Building a dictionary of counts can more easily done using the built-in collections.Counter. It's not clear why you would want to avoid using this. Trivial functions should be trivial.
There's no point in testing if 1 in myDict.values(). Dictionaries have efficient lookup by key, but not by value, so the in operator here has to look through all the values. Since you're going to look through all the values anyway, this doesn't save you anything.
The runtime is \$Θ(n^2)\$ because s.index(k) has to search through the string for the character k. But there is a \$Θ(n)\$ algorithm:

from collections import Counter

def unique_char(s):
    """Return the first non-repeated character in the string s, or raise
    ValueError if all characters are repeated.

    """
    counts = Counter(s)
    for c in s:
        if counts[c] == 1:
            return c
    raise ValueError("all characters are repeated")


Basic neural network I just programmed a basic neural network in F# to learn the logical OR function. As I am very new to F# and especially functional programming, I did it the imperative way. And even tho it works, I find it highly unattractive. I would like to improve it to make it as functional-like as possible. I though about overloading operators to make scalar products between weights and neuroninput but i don't think its the classier way to make my program nicer.

I want to make my code functional-like.

module nnbasic

let mutable neuroninput = [0.0;0.0]
let mutable weight = [0.4;0.6]
let rate = 0.2
let threeshold = 2.0

// [input1; input2; desiredoutput]
let matrix = [
             [0.0;0.0;0.0];
             [0.0;1.0;1.0];
             [1.0;0.0;1.0];
             [1.0;1.0;1.0]
             ]

let display output real =
    if output = real then printfn "yes"
    else printfn "no"

let output (_ni: float list, _wi: float list) =
    if threeshold > _ni.[0]*_wi.[0] + _ni.[1]*_wi.[1] then 0.0 else 1.0

let mutable iter = 0
let mutable out = 0.0

while iter < 100 do
    for row in matrix do
        neuroninput <- [row.[0];row.[1]]

        out <- output (neuroninput, weight)
        weight <- [weight.[0]+rate*(row.[2]-out);weight.[1]]
        display out row.[2]

        out <- output (neuroninput, weight)
        weight <- [weight.[0];weight.[1]+rate*(row.[2]-out)]

        if threeshold > neuroninput.[0]*weight.[0] + neuroninput.[1]*weight.[1] then display 0.0 row.[2] else display 1.0 row.[2]
        iter <- iter+1

Printing a list of items to the console is inherently imperative. But keeping track of lots of state between iterations using higher-order functions is often inelegant. Regardless, there are some things you can do to clean this up and (if you're compelled) get rid of mutables. Most notably, you can use tuples instead of arrays/lists with an assumed width.

I'm not familiar with this algorithm, so I've merely translated what you have. You can implement it using a pair of mutually recursive functions. This allows you to "persist" the weight values between iterations.

let rate = 0.2
let threshold = 2.0

let inline output a b weightA weightB = if threshold > a * weightA + b * weightB then 0.0 else 1.0
let inline display output real = printfn <| if output = real then "yes" else "no"
let inline computeWeight weight c out = weight + rate * (c - out)

let matrix =
  [|
    0.0, 0.0, 0.0
    0.0, 1.0, 1.0
    1.0, 0.0, 1.0
    1.0, 1.0, 1.0
  |]

let rec iter n weightA weightB =
  if n > 0 then
    loop n 0 weightA weightB
and loop n i weightA weightB =
  if i < matrix.Length then
    let a, b, c = matrix.[i]
    let out = output a b weightA weightB
    let weightA = computeWeight weightA c out
    display out c
    let out = output a b weightA weightB
    let weightB = computeWeight weightB c out
    display out c
    loop n (i + 1) weightA weightB
  else iter (n - 1) weightA weightB

Usage:

let initialWeightA = 0.4
let initialWeightB = 0.6
iter 100 initialWeightA initialWeightB

loop could be implemented as a fold, but fold is not typically used with side-effects.


Author class, with constructor validations and rich comparisons I wrote the following Author class in Python for a project I'm working on. I wanted to make sure that the rich comparisons and the methods I used to validate the constructor parameters were done properly.

Author class:

class Author:

    def __init__(self, id, firstname, lastname):
        self._validateid(id, "ID")
        self._validate_author(firstname, "First Name")
        self._validate_author(lastname, "Last Name")

        self._id = id
        self._firstname = firstname
        self._lastname = lastname

    def _validateid(self, id, error_message):
        if id < 1:
            raise ValueError(error_message + " is invalid")

    def _validate_author(self, parameter, error_message):
        if not parameter:
            raise TypeError(error_message + " is missing")

    @property
    def iden(self):
        return self._id

    @property
    def first_name(self):
        return self._firstname

    @property
    def last_name(self):
        return self._lastname

Rich Comparisons:

    def __lt__(self, other):
        if isinstance(other, Author):
            return (self.iden, self.first_name.lower(), self.last_name.lower()) < (other.iden, other.first_name.lower(), other.last_name.lower())
        return NotImplemented

    def __le__(self, other):
        if isinstance(other, Author):
            return (self.iden, self.first_name.lower(), self.last_name.lower()) <= (other.iden, other.first_name.lower(), other.last_name.lower())
        return NotImplemented

    def __eq__(self, other):
        if isinstance(other, Author):
            return (self.iden, self.first_name.lower(), self.last_name.lower()) == (other.iden, other.first_name.lower(), other.last_name.lower())
        return NotImplemented

    def __ne__(self, other):
        if isinstance(other, Author):
            return (self.iden, self.first_name.lower(), self.last_name.lower()) != (other.iden, other.first_name.lower(), other.last_name.lower())
        return NotImplemented

    def __ge__(self, other):
        if isinstance(other, Author):
            return (self.iden, self.first_name.lower(), self.last_name.lower()) >= (other.iden, other.first_name.lower(), other.last_name.lower())
        return NotImplemented

    def __gt__(self, other):
        if isinstance(other, Author):
            return (self.iden, self.first_name.lower(), self.last_name.lower()) > (other.iden, other.first_name.lower(), other.last_name.lower())
        return NotImplemented

    def __hash__(self):
        return hash((self.iden, self.first_name.lower(), self.last_name.lower()))

Personally using a namedtuple is the best way to strip most of your code.

If you wish to be able to mutate your data, then inheriting from a list, rather than namedtuple, could be a better idea.
I'm not a fan of _validateid or _validate_author, just write them out in the constructor.

And so I'd personally change your code to:

from collection import namedtuple

class Author(namedtuple('AuthorBase', 'id first_name last_name')):
    def __new__(cls, id, first, last):
        if id < 1:
            raise ValueError('id is not a valid id')
        if not first:
            raise ValueError('first_name has not been set')
        if not last:
            raise ValueError('last_name has not been set')
        return super().__new__(cls, id, first, last)


SMB Script which collect remote files, mount locations, unmount locations, and show directory file tree I've written a script that is able to accomplish the following:

mount/unmount a remote SMB location using the subprocess module
List the file contents of a specified directory
copy a single file out from a remote directory to a specified destination folder.
List subdirectories within a mounted SMB location.

I've done my best to make as much of my functionality reusable within functions, but there's a part of me that worries functions like selectSingleFile() is getting a little convoluted.

Here is my code:

import os
import shutil
import subprocess
from os import walk
from shutil import copy2
from glob import glob

def mount(remote_dir, local_dir):
    """Mounts a remote directory and chooses a local file in which to mount the directory against"""
    retcode = subprocess.call(["/sbin/mount", "-t", "smbfs", remote_dir, local_dir])

def unmount(local_dir):
    """Unmounts the local SMB directory"""
    retcode = subprocess.call(["/sbin/umount", local_dir])

def getFileContents(directory):
    """Creates a list of files within a directory"""
    f = []
    for (dirpath, dirnames, filenames) in walk(directory):
        f.extend(filenames)
        break
    return f

def printChoices(local_dir):
    """Used to print a list of numbered list of files in a directory"""
    num = 1
    a = getFileContents(local_dir)
    for item in a:
        print(str(num) + ".", item)
        num +=1

def listDirectory(directory):
    return glob(directory)

def selectSingleFile(local_dir, destination):
    '''Used to get the contents of a directory and select a single file to copy to a destination
    --requires printChoices() and getFileContents()
    '''
    a = getFileContents(local_dir)
    printChoices(local_dir)
    b = int(input("please select your choice: "))
    copy2(local_dir+(a[b-1]), destination)

''' The following was an example usage of the functions above. It allowed for mounting of a remote
file location, within the work network, and a listing/transfer of a single file within the directory.
'''
local_dir = '/Users/ParanoidPenguin/smb/' #Specifies the local directory to mount
destination = '/Users/ParanoidPenguin/Desktop/JPEGs'#For copy2, destination where file will be stored
smb_location = '//Guest@192.168.1.167/Documents' #smb location to be mounted

mount(smb_location, local_dir)

selectSingleFile(local_dir, destination)

input("Press any key to unmount: ")
unmount(local_dir)

I didn't try to run your program. So some things may not work.

Imports. There is no need to do them twice like what you did with os and shutil. Just write import os or import shutil and then use them like os.walk and shutil.copy2. Same goes for glob.
PEP 8.

  Surround top-level function and class definitions with two blank
  lines.

  Limit all lines to a maximum of 79 characters. For flowing long blocks
  of text with fewer structural restrictions (docstrings or comments),
  the line length should be limited to 72 characters.

  Function names should be lowercase, with words separated by
  underscores as necessary to improve readability.

  Inline comments should be separated by at least two spaces from the statement. They should start with a # and a single space.

Docstrings should be surrounded by """. In selectSingleFile you used '''.

Naming conventions. Variables should have self-explanatory names. Your a, b, f don't say anything. getFileContents probably should be called something like get_files_list or get_dir_contents?
Type hints. With type hints you can help other people and yourself to understand what type of data your functions get as input and what type of output is going to be produced. For example, signature of your getFileContents will look like this:

def getFileContents(directory: str) -> List[str]:
    # Your code...

Don't forget to include from typing import List in the beginning.
You have not used variables called retcode. You can safely delete them. Also listDirectory is never used.
Some of your docstrings say for example: "Used to print...". Why not just write "Prints..."?
Instead of using subprocess.call you could use subprocess.check_call. So if something goes wrong you will get a CalledProcessError instead of nothing.
Your mount and unmount are quite similar. We could use partials. (I hope I didn't go too far with this). For example:

import subprocess
from functools import partial

def check_call(*args) -> None:
    subprocess.check_call(args)

mount = partial(check_call,
                '/sbin/mount',
                '-t',
                'smbfs')
mount.__doc__ = ('Mounts a remote directory '
                 'and chooses a local file '
                 'in which to mount the directory against')

unmount = partial(check_call,
                  '/sbin/umount')
unmount.__doc__ = 'Unmounts the local SMB directory'

Probably, something should be done about hard-coded call arguments but I'm not sure what.
In getFileContents instead of creating an empty list and then filling it we could create it by list comprehension. I propose two versions:

def getFileContents(directory: str) -> List[str]:
    return [file
            for *_, files_list in os.walk(directory)
            for file in files_list]

Note the *_. We use _ for throwaway variables. And with the asterisk we say that there are multiple throwaway variables like in your case: dirpath and dirnames.
Another way is to use itertools.chain:

def getFileContents(directory: str) -> List[str]:
    files_lists = [files_list
                   for *_, files_list in os.walk(directory)]
    return list(itertools.chain(*files_lists))

In selectSingleFile it would be good to add checking input. Take a look at the first answer here.
In the same function you calculate list of files twice. First, in the function itself and the second is in the printChoices. So, instead of passing local_dir to printChoices, pass already calculated list of files (a - how you call it)
Instead of joining paths with a + use os.path.join
By this point your function should look like this:

def select_single_file(local_dir: str,
                       destination: str) -> None:
    """
    Gets the contents of a directory
    and selects a single file to copy to a destination.
    """
    files = get_file_contents(local_dir)
    print_choices(files)

    # TODO: add checking
    file_index = int(input("Please select your choice: "))
    file_path = os.path.join(local_dir,
                             files[file_index - 1])
    shutil.copy2(file_path,
                 destination)

Finally, you have a counter variable in printChoices. Why not use enumerate? So, you will have:

def print_choices(files: List[str]) -> None:
    """Prints a numbered list of files in a directory"""
    for index, filename in enumerate(files, start=1):
        print(str(index) + ".", filename)

P.S.: "Please select your choice: " This is tautology.

Edit: You asked if it would be a good idea to expand functions to deal with exceptions. If you want to ask user to enter paths until he gives you correct ones then yes. But if you just want to throw an error if the entered path by user is incorrect, you should test your functions with valid and invalid input data. If for invalid input data you get a traceback from which it is not clear what goes wrong, you could add catching exceptions in those functions. I think you will need to use something from os.path.isfile, os.path.isdir, os.path.ismount and os.path.exists. Check the docs.


Simple XML Creator in Java package xml.impl;

import java.io.FileWriter;
import java.io.IOException;
import java.util.HashMap;

public class XMLCreator {

private String parent;
private String child;
private Integer ctr = 0;
private HashMap<Integer, String> xml;
private String root;
private HashMap<Integer, String> roots;
private StringBuilder sB;

private HashMap<Integer, String> getRoot() {
    this.roots.put(1, "<" + root + ">");
    this.roots.put(2, "</" + root + ">");
    return roots;
}

public void setRoot(String root) {
    this.root = root;
}

public XMLCreator() {
    if (this.xml == null) {
        this.xml = new HashMap<Integer, String>();
    }
    if (this.roots == null) {
        this.roots = new HashMap<Integer, String>();
    }
    if (this.sB == null) {
        this.sB = new StringBuilder();
    }
}

/*
 * private void startParentField(String parent) { this.parent = "<" + parent
 * + ">"; this.ctr++; addToXML(getParentField()); }
 *
 * private void endParentField(String parent) { this.parent = "</" + parent
 * + ">"; this.ctr++; addToXML(getParentField()); }
 *
 * public void setParent(String parent) { startParentField(parent);
 * endParentField(parent); }
 */

public void addChildField(String child) {
    this.child = "<" + child + ">" + "</" + child + ">";
    this.ctr++;
    addToXML(getChildField());

}

public void addChildField(String child, String value) {
    this.child = "<" + child + ">" + value + "</" + child + ">";
    this.ctr++;
    addToXML(getChildField());
}

private String getParentField() {
    return parent;
}

private String getChildField() {
    return child;
}

private void addToXML(String element) {
    xml.put(ctr, element);
}

public void buildXMLPreview() {

    // Set<Integer> keys = xml.keySet();
    getRoot();
    System.out.println(roots.get(1));
    sB.append(roots.get(1));
    for (int x = 1; x <= xml.size(); x++) {
        sB.append(xml.get(x));
    }
    sB.append(roots.get(2));
    System.out.println(sB.toString());
    sB.delete(0, sB.length());
}

public void buildXMLToFile() throws IOException {

    FileWriter writer = new FileWriter("try.xml");
    getRoot();
    writer.append(roots.get(1));
    for (int x = 1; x <= xml.size(); x++) {
        writer.append(xml.get(x));
    }
    writer.append(roots.get(2));
    writer.flush();
    writer.close();
}

}

Sample Implementation:

package xml.impl;

import java.io.IOException;

public class XMLCreatorTester {
public static void main(String[] args) throws IOException {
    XMLCreator xmlCreator = new XMLCreator();
    xmlCreator.setRoot("Art");
    xmlCreator.addChildField("name", "Mona Lisa");
    xmlCreator.addChildField("artist", "Leonardo Da Vinci");
    xmlCreator.addChildField("adsa");
    xmlCreator.buildXMLPreview();
    xmlCreator.buildXMLToFile();
}
}

Output:

<Art>
<name>Mona Lisa</name>
<artist>Leonardo Da Vinci</artist>
<adsa></adsa>
</Art>

As you can see as of now I can only create the root element and child elements(note that the parent element implementation is commented out).

Good enough? Suggestions? Violent reactions? Thanks. :)

What's this?

public XMLCreator() {
    if (this.xml == null) {
        this.xml = new HashMap<Integer, String>();
    }
    if (this.roots == null) {
        this.roots = new HashMap<Integer, String>();
    }
    if (this.sB == null) {
        this.sB = new StringBuilder();
    }
}

The null checks are always true, where should a value come from?

Why

this.child = "<" + child + ">" + "</" + child + ">";

and not

this.child = "<" + child + "/>";

?


Thin controller design We have asked to keep the MVC controller as thin, so we decided to move
  all the service calling/business logic into the ViewModel class.

But is it good approach to use a ViewModel class to make a service call?

Does this follows the OOPS/SOLID principle?

public class EmployeeController:Controller
{

  private IEmployeeService EmpServ{get;set;}
  public EmployeeController(IEmployeeService service)
  {
   EmpServ=service;
  }

 public ActionResult GetEmployee(int Id)
 {
  EmployeeViewModel vm=new EmployeeViewModel(EmpServ);
  vm= vm.GetData(Id);
  //some 10 lines of code for validation logic, etc
  return View(vm);
 }

}

The view model class is calling the business service method

public class EmployeeViewModel
{
  public string Name{get;set;}
  //...etc

  private IEmployeeService EmpServ{get;set;}
  public EmployeeViewModel(IEmployeeService service)
  {
   EmpServ=service;
  }

  public EmployeeViewModel GetData(int Id)
  {
   Employee e= EmpServ.GetEmpDetails(id);
   EmployeeViewModel vm=ConvertEmpToViewModel(e);//another helper method
   return vm;
  }
}

We have asked to keep the MVC controller as thin, so we decided to move all the service calling/business logic into the ViewModel class.

The concept of the "thin" controller is a guideline to help you avoid having business logic in your presentation layer - it is not as simple as "having more than a little code in controller actions is bad."

In moving this code to your view models, you have effectively coupled them to your service layer without solving the real problem. The controller is responsible for mediating between your services and your presentation logic (views and view models) - that is the entire reason for its existence. Do not just shift code around arbitrarily because of some naive desire to reduce the amount of code in your controller actions.

If you find your controller has too much code in it (or, more importantly, that it has business logic in it), then what you need is an application facade layer that sits between your controller and your service layer, co-ordinates your services and provides a simpler interface for your controllers.

Alternatively, if the code you're trying to move is presentation-layer logic (such as model validation, construction of view models, processing of form values, mapping of view models to service-layer entities etc.), then it does belong in your controller - even if there are more than a couple of lines of it. You can still simplify things by breaking it up into helpers if it's beneficial to do so, but don't do this just because someone told you that your controller actions shouldn't have more than five lines of code in them.


C# Function: IsAvailable(T value) I'm sick of writing negative code statements like String.IsNullOrEmpty(value) and so I'm writing a function that checks for availability. It should answer the question, "Is this value usable?" without validating the data.

This function has been exceptionally difficult to write but I have a solution I feel comfortable with using. I'm hoping someone can poke holes in it and give me constructive feedback.

public static bool IsAvailable<T>(T value)
{
    if (value == null)
        return false;

    var type = typeof(T);
    if (type.IsValueType)
        return true;

    if (typeof(IEnumerable).IsAssignableFrom(type))
    {
        var enumerator = (value as IEnumerable).Cast<object>();
        return enumerator.Any(a => IsAvailable(a));
    }

    var valueString = Convert.ToString(value);
    return !String.IsNullOrWhiteSpace(valueString);
}

I'm not so sure about this method:

Why would you call this for a value type, if you already know that you're dealing with a value type? You mentioned something about 'obviously wrong default values', but that probably depends on the context.
If you do find a general definition of what exactly being 'available' means, you may want to document it. Chances are that what's obvious to you isn't obvious to someone else, or even to yourself a few months later.
What's the point of checking if an IEnumerable contains Any available items? It could still contain many unavailable items, so you'd still have to check each item before use. All would be more appropriate, but that would make this method very 'eager'.
Iterating an IEnumerable can cause problems when you're working with lazily evaluated sequences. You'd have to be careful with where you use this method, or you could accidentally fetch data from a database twice, or do other duplicated work. Letting the caller check each item before use seems a much safer (and cheaper) alternative.
That Cast<object>() call causes T in the inner call to be object, so the IsValueType and IEnumerable checks won't work.
You'll want to know the actual type of the value you're checking, so use value.GetType() instead of typeof(T). And instead of calling IsAssignableFrom, you can use is, or just use as immediately and then check if the result isn't null.
What's the point of converting the given value/object to a string? It's basically checking the result of value.ToString(). Any class that overrides ToString and that returns null or an empty string will always be seen as unavailable, which is probably a subtle bug lying in wait. I'm not sure what this check is meant to accomplish.

All in all, I don't really see what value this method provides. If you've got a value type then you probably already know it, and you most likely know better which values are valid and which ones aren't, depending on context. In any other case, you can use the Elvis operator (?.) or do a null check of some sort.


Scoping resources per HTTP request I am using this code in an MVC application to manage objects that should only have one instance per request.

/// <summary>
/// Manage all the resources that we want to scope to a single HTTP request.
/// </summary>
/// <remarks>
/// This class should not be directly referenced in many places.
/// In general, infrastructure code is OK whereas application code is not.
/// </remarks>
sealed class RequestScoped : IDisposable
{
    private static readonly object key = typeof(RequestScoped);

    /// <summary>
    /// Gets the RequestScoped instance for the given HttpContext.
    /// Creates and returns a new instance if it doesn't exist yet.
    /// This method is not thread-safe.
    /// The assumption is that there will only be one thread per request.
    /// </summary>
    public static RequestScoped GetOrCreate(System.Web.HttpContext context)
    {
        if (!context.Items.Contains(key))
        {
            context.Items[key] = new RequestScoped();
            context.AddOnRequestCompleted(Cleanup);
        }
        return (RequestScoped)context.Items[key];
    }

    private static void Cleanup(System.Web.HttpContext context)
    {
        if (context.Items.Contains(key))
        {
            var scope = (RequestScoped)context.Items[key];
            scope.Dispose();
        }
    }

    private readonly Lazy<IDbConnection> connection;
    private readonly Lazy<SessionRepository> sessionRepo;

    private RequestScoped()
    {
        connection = new Lazy<IDbConnection>(() => Utilities.MakeOpenConnection());
        sessionRepo = new Lazy<SessionRepository>(() => new SessionRepository(connection));
    }

    public IDbConnection OpenConnection { get { return connection.Value; } }
    public SessionRepository SessionRepo { get { return sessionRepo.Value; } }

    public void Dispose()
    {
        if (connection.IsValueCreated)
        {
            try
            {
                if (connection.Value.State == ConnectionState.Open)
                {
                    connection.Value.Close();
                }
            }
            catch (Exception)
            {
                // ignore
            }
            connection.Value.Dispose();
        }
    }
}

Example usage is like this:

protected IDbConnection OpenConnection
{
    get { return RequestScoped.GetOrCreate(HttpContext.Current).OpenConnection; }
}

(I am aware of DI containers but I have chosen not to use one in this application.)

Why is it specific to the two properties you have currently? I think it would make sense to make it more generic, but I can't think of a good design to do that.

sealed class RequestScoped : IDisposable

Is it okay for users to call Dispose() by themselves? If not, don't make this type IDisposable and don't make Dispose() public.

private static readonly object key = typeof(RequestScoped);

Since other pieces of code have access to typeof(RequestScoped), they could potentially read or overwrite the RequestScoped instance that's stored in the context by accident. To be on the safe side, I would use new object() here.

/// This method is not thread-safe.

I'm not sure you need to call this out here. Especially since this method is thread-safe, as long as different threads use different contexts.

public static RequestScoped GetOrCreate(System.Web.HttpContext context)

The fact that this method either gets existing object or creates a new one sounds like an implementation detail to me. I would probably call it just Get().

Why are you repeating the namespace for HttpContext instead of using using?

public IDbConnection OpenConnection { get { return connection.Value; } }

Is "open" here supposed to be a verb or an adjective? If verb, then that's not appropriate for a property. If adjective, then it's I think it's not necessary.

public SessionRepository SessionRepo { get { return sessionRepo.Value; } }

I don't see any reason to shorten "repository" to "repo" here, SessionRepository is not that long.

public void Dispose()
{
    if (connection.IsValueCreated)
    {
        try
        {
            if (connection.Value.State == ConnectionState.Open)
            {
                connection.Value.Close();
            }
        }
        catch (Exception)
        {
            // ignore
        }
        connection.Value.Dispose();
    }
}

Why are you not disposing SessionRepository? Is that not necessary?

Also, why are you trying to call Close() and then also call Dispose()? Isn't just Dispose() enough?


Code for sums in a vector I posted this code in an answer earlier today, but thought it was worth seeing if somebody could suggest further improvements:

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

template <class InIt, class OutIt>
OutIt adjacent_sum(InIt begin, InIt end, OutIt result) {
    ++begin;
    while (begin != end) {
        *result = *begin + *(begin-1);
        ++result;
        ++begin;
    }
    return result;
}

int main() {
    std::vector<int> inputs { 1, 2, 3, 4, 5, 10, 9, 8, 7, 6};

    adjacent_sum(inputs.begin(), inputs.end(),
        std::ostream_iterator<int>(std::cout, "\t"));

    std::cout << "\n";

    std::transform(inputs.begin(), inputs.begin()+inputs.size()/2,
        inputs.rbegin(),
        std::ostream_iterator<int>(std::cout, "\t"),
        std::plus<int>());
}

Here I've placed fixed input into the vector inputs. The intent is to first produce the sums of adjacent items in the input, and second produce the sum of the first item added to the last item, second item added to the second to last, and so on.

You make the assumption of random accesses iterators.
It would be nice to be able to do this with any iterator (including input iterator)

template <class InIt, class OutIt>
OutIt adjacent_sum(InIt begin, InIt end, OutIt result)
{
    if (begin == end)
    {     return result;    // From @Corbin
    }

    auto  last = (*begin);  // Get the value now before incrementing the iterator.
    ++begin;
    while (begin != end)
    {
        auto next = (*begin);
        *result   = next + last;
        last      = next;

        ++result;
        ++begin;
    }
    return result;
}

Now the following works:

adjacent_sum(std::istream_iterator<int>(std::cin), std::istream_iterator<int>(),
             std::ostream_iterator<int>(std::cout, "\t"));


"The python that ate a calculator" In a YouTube video I watched, the speaker said it is important to know how to calculate between numbers of different bases; i.e. two, ten, and sixteen. I was then inspired to write a small C program to ensure I understand how such conversions are done. I then wrote a short Python2 script to test the program. I'd like any constructive feedback on the coding style, layout, efficiency, and anything else that stands out. Debug lines are commented out.

calc.c

#include <math.h>
#include <stdio.h>
#include <string.h>

typedef enum {
    AND = '&',
    OR = '|',
    XOR = '^',
    NONE = 0
} operator;

typedef enum {
    B02 = 'b',
    B10 = 'd',
    B16 = 'x'
} type;

const char zero = '0';
const char _A = 'A';
const char _a = 'a';

int main(int argc, char **argv) {
    int i, offs, len;
    char *nend;
    long result = 0, tmp, optmp;
    operator op = NONE;
    type ntype;

    for (i = 1; i < argc; i++) {
        len = strlen(argv[i]);
        nend = argv[i] + len - 1;
        if (len == 1) {
            op = *nend;
            continue;
        }
        ntype = *nend;
        nend--;
        offs = 0;
        optmp = 0;
        while (offs < len - 1) {
            switch (ntype) {
            case B02:
                tmp = (*(nend - offs) - zero) * pow(2, offs);
                break;
            case B10:
                tmp = (*(nend - offs) - zero) * pow(10, offs);
                break;
            case B16:
                tmp = *(nend - offs);
                if (tmp >= _a) tmp -= _a - 10;
                else if (tmp >= _A) tmp -= _A - 10;
                else tmp -= zero;
                tmp *= pow(16, offs);
                break;
            }
            optmp += tmp;
            offs++;

        }
//printf("%s: offs: %i tmp: %li optmp: %li len: %i result: %li\n", argv[i], offs, tmp, optmp, len, result);
        switch (op) {
        case AND:
            result &= optmp;
            break;
        case OR:
            result |= optmp;
            break;
        case XOR:
            result ^= optmp;
            break;
        case NONE:
            result += optmp;
            break;
        }
//printf("\n");
    }
    printf("%li\n", result);
    return 0;
}

makecalc.sh

gcc -Wall calc.c -o calc

testcalc.py

import subprocess as subp
import time

_bin = lambda x: bin(x)[2:] + 'b'
_dec = lambda x: str(x) + 'd'
_hex = lambda x: hex(x)[2:] + 'x'

ops = ('&', '|', '^')
types = (_bin, _dec, _hex)
RANGE = 20

tmp = None

def test(op, t, x, y):
    call = ["./calc", t(x), op, t(y)]
    print call
    proc = subp.Popen(call, stdout=subp.PIPE, stdin=subp.PIPE)
    data = proc.stdout.readline().replace("\n", "")
    exec("tmp = %s %c %s;" % (str(x), op, str(y)))
    if tmp != int(data):
        print ("fail", tmp, data)
        quit()

for op in ops:
    for t in types:
        for x in range(RANGE):
            for y in range(RANGE):
                test(op, t, x, y)

I suppose you don't indent the printf debug lines to make them easier to spot. That's a sensible idea (although not one I've seen before). However, I treat Code Review answers as a "just before checkin" thing, so I'd suggest removing these by this point.

You pre-declare your variables. Don't do this; write them as close to point-of-use as reasonable.

Your NONE op seems to be rather useless - you could just use OR instead. So remove it.

Your

const char zero = '0';
const char _A = 'A';
const char _a = 'a';

variables might seem like a good adherence to "no magic constants", but in reality they have effectively no useful purpose. If you wanted to update your code to support other character sets, you'd want to redesign the whole parser anyway. IMHO, it's simpler to inline these.

You're not required to return 0; in modern C, and it's somewhat typical not to.

Your integer parser should be generic in the base. I suggest extracting this into a function like int parse_digit(char character, int base, int place):

int parse_digit(char character, int base, int place) {
    int value;

    if (character >= 'a') {
        value = character - 'a' + 10;
    }
    else if (character >= 'A') {
        value = character - 'A' + 10;
    }
    else {
        value = character - '0';
    }

    return value * pow(base, place);
}

while (offs < len - 1) {
    optmp += parse_digit(*(nend - offs), base, offs);
    offs++;
}

base can be calculated with a simple lookup on ntype:

int type_to_base(type ntype) {
    switch (ntype) {
    case B02: return 2;
    case B10: return 10;
    case B16: return 16;
    }
}

Although this makes the type enum rightly look silly: it's just an integer after all!

int flag_to_base(char flag) {
    switch (flag) {
    case 'b': return 2;
    case 'd': return 10;
    case 'x': return 16;
    }
}

Note that this is a somewhat silly way to do things, though. First of all, pow takes doubles! I'd rather avoid that if possible. Further, this is more work than needs to be done. Instead, one can do this:

  1*base^4 + 0*base^3 + 1*base^2 + 0*base^1 + 1*base^0  // how you're currently doing it
= (((1 * base + 0) * base + 1) * base + 0) * base + 1          // simpler way to do it

This can be done as:

int parse_digit(char character) {
    if (character >= 'a') {
        return character - 'a' + 10;
    }
    else if (character >= 'A') {
        return character - 'A' + 10;
    }
    else {
        return character - '0';
    }
}

while (offs < len - 1) {
    optmp *= base;
    optmp += parse_digit(*(nend - offs));
    offs++;
}

Parsing integers should be in another function, and can be slightly simplified:

long parse_int(char *start, char *end) {
    int base = flag_to_base(*end);

    long ret = 0;
    for (char *digit = start; digit < end; digit++) {
        ret *= base;
        ret += parse_digit(*digit);
    }

    return ret;
}

I would also put application of optmp into a function, and stop calling it optmp (in general, most variables are temporary - stating this is pointless):

long apply(operator op, long left, long right) {
    switch (op) {
    case AND: return left & right;
    case OR:  return left | right;
    case XOR: return left ^ right;
    }
}

I would then change (generally) long to uint64_t and int to uint32_t (or even uint8_t) - there's no reason to be using variable-length integers in this day and age for these types, and you don't need signed values. When you do want variable-length values, you'd probably rather stick with size_t. (If you find these unwieldy, typedef them to u64, u32 and u8. Just don't use int and long 'cause they're prettier.)

Compiling with warnings, I get

calc.c: In function ‘apply’:
calc.c:18:1: warning: control reaches end of non-void function [-Wreturn-type]
 }
 ^
calc.c: In function ‘flag_to_base’:
calc.c:26:1: warning: control reaches end of non-void function [-Wreturn-type]
 }
 ^

This is because the new method allows better case analysis - we've forgotten to check that such operations are actually valid!

#include <math.h>
#include <stdint.h>
#include <stdio.h>
#include <string.h>

typedef enum {
    AND = '&',
    OR = '|',
    XOR = '^',
} operator;

int apply(operator op, uint64_t *left, uint64_t right) {
    switch (op) {
    case AND: *left &= right; break;
    case OR:  *left |= right; break;
    case XOR: *left ^= right; break;
    default: return -1;
    }
    return 0;
}

int flag_to_base(char flag, uint8_t *base) {
    switch (flag) {
    case 'b': *base = 2; break;
    case 'd': *base = 10; break;
    case 'x': *base = 16; break;
    default: return -1;
    }
    return 0;
}

int parse_digit(char digit, uint8_t *value) {
    if ('a' <= digit && digit <= 'z') {
        *value = digit - 'a' + 10;
    }
    else if ('A' <= digit && digit <= 'Z') {
        *value = digit - 'A' + 10;
    }
    else if ('0' <= digit && digit <= '9') {
        *value = digit - '0';
    }
    else {
        return -1;
    }

    return 0;
}

int parse_int(uint64_t *ret, char *start, char *end) {
    uint8_t base;
    if (flag_to_base(*end, &base)) {
        return -1;
    }

    *ret = 0;
    for (char *digit = start; digit < end; digit++) {
        uint8_t value;
        if (parse_digit(*digit, &value) || value >= base) {
            return -1;
        }

        *ret *= base;
        *ret += value;
    }

    return 0;
}

int main(int argc, char **argv) {
    uint64_t result = 0;
    operator op = OR;

    for (int i = 1; i < argc; i++) {
        size_t len = strlen(argv[i]);
        char *end = argv[i] + len - 1;

        if (len == 1) {
            op = *end;
            continue;
        }

        uint64_t operand;
        if (parse_int(&operand, argv[i], end)) {
            printf("Can't parse argument '%s'\n", argv[i]);
            return -1;
        }

        if (apply(op, &result, operand)) {
            printf("Can't parse argument '%c'\n", op);
            return -1;
        }
    }

    printf("%li\n", result);
}

Note that I would be wary of casting to enums where this might result in an "invalid" enum, but at least it is well-defined behaviour.

For the Python, your

_bin = lambda x: bin(x)[2:] + 'b'
_dec = lambda x: str(x) + 'd'
_hex = lambda x: hex(x)[2:] + 'x'

can be just

_bin = "{:b}b".format
_dec = "{:d}d".format
_hex = "{:x}x".format

Your embedded loops can be simplified to

from itertools import product
for args in product(ops, types, range(RANGE), range(RANGE)):
    test(*args)

You don't use time - remove it. Rather than rename subprocess to subp, I would directly import Popen and PIPE.

Use new-style formatting, please! Also, use eval instead of exec - using neither would be best, although its use is mildly understandable in this case (even if still saddening).

Don't call quit - this is a function only for interpreter usage. I actually suggest just asserting in this case.

Your subprocess call only wants output, so drop stdin=PIPE. Further, you can just call check_output:

import subprocess
from itertools import product

def test(op, t, x, y):
    call = ["./calc", t(x), op, t(y)]
    print call
    data = subprocess.check_output(call)
    assert eval("{}{}{}".format(x, op, y)) == int(data)

ops = "&|^"
types = "{:b}b".format, "{:d}d".format, "{:x}x".format
count = 20

for args in product(ops, types, range(count), range(count)):
    test(*args)


Counting Balanced numbers in a given array I am trying a code challenge. Here is the problem description:

When an element is deleted from an array, the higher-indexed elements shift down one index to
fill the gap. A "balancing element" is defined as an element that, when deleted from the array,
results in the sum of the even-indexed elements being equal to the sum of the odd-indexed
elements. Determine how many balancing elements a given array contains.

Example
n=5 where n is the size, arr = [5, 5, 2, 5, 8]

When the first or second 5 is deleted, the array becomes [5, 2, 5, 8]. The
sumeven = 5 + 5 = 10
and sumodd = 2 + 8 = 10. No other elements of the original array have that
property.

There are 2 balancing elements that is arr[0] and arr[1].

And here is my solution:
public class Challenge {
  public static void main(String[] args) {
    countBalancingElements(new int[] {1, 1, 1});
  }
  static int countBalancingElements(int[] arr) {
    int count = 0;
    for (int i = 0; i < arr.length; i++) {
      if (evenIndexValEqualsOddIndexVal(removeElementInArr(arr, i))) {
        count++;
      }
    }
  }
  static int[] removeElementInArr(int[] arr, int index) {
    int[] result = new int[arr.length - 1];
    for (int i = 0, j = 0; i < arr.length && j < result.length; i++, j++) {
      if (i == index) {
        result[j] = arr[i + 1];
        i++;
      } else {
        result[j] = arr[i];
      }
    }
    return result;
  }
  static boolean evenIndexValEqualsOddIndexVal(int[] arr) {
    int evensum = 0;
    int oddsum = 0;
    for (int i = 0; i < arr.length; i++) {
      if (i % 2 == 0) {
        evensum += arr[i];
      } else {
        oddsum += arr[i];
      }
    }
    return evensum == oddsum && evensum != 0 && oddsum != 0;
  }
}

It looks like its runtime is \$O(n^2)\$.
evensum != 0 && oddsum != 0; is added to satisfy this input [0, 0], but it breaks for input {-5, 7, 0, 5 } whose evensum and oddsum is 0. So, cases like [0,0] are to be dealt with separately.

return evensum == oddsum && evensum != 0 && oddsum != 0;

Why can't sums be zero?  With this array ... {-5, 7, 0, 5 } ... if you remove the 7, then the even elements -5 and 5 would sum to zero, and the odd elements 0 also sum to zero, giving a "balanced" array.

Move tests out of loops wherever possible.

    for (int i = 0; i < arr.length; i++) {
      if (i % 2 == 0) {
        evensum += arr[i];
      } else {
        oddsum += arr[i];
      }
    }

Here, with a 1 million element array, i % 2 == 0 is being done 1 million times.  If you summed the even and odd elements separately ...

    for (int i = 0; i < arr.length; i += 2) {
      evensum += arr[i];
    }
    for (int i = 1; i < arr.length; i += 2) {
      oddsum += arr[i];
    }

... you never have to even test i % 2 == 0, which should be a speed improvement.

The above also applies to copying elements in removeElementInArr():

    for (int i = 0, j = 0; i < arr.length && j < result.length; i++, j++) {
      if (i == index) {
        result[j] = arr[i + 1];
        i++;
      } else {
        result[j] = arr[i];
      }
    }

You know you'll be copying elements up to index, and then copying the elements after index, but you do an i == index inside the loop.  With a million elements, that is a lot of extraneous testing.

    // Copy elements before index:
    for (int i = 0; i < index; i++) {
        result[i] = arr[i];
    }

    // Copy elements after index:
    for (int i = index + 1, j = index; i < arr.length; i++, j++) {
        result[j] = arr[i];
    }

But this is still a lot of manual copying.  The JVM can do the copying for you, a lot more efficiently using System.arraycopy

    System.arraycopy(arr, 0, result, 0, index);
    System.arraycopy(arr, index + 1, result, index, result.length - index);

Now the JVM does all the bound-checking once for each call, instead of two million times when done manually on an array of one million entries.  Much faster.

If you add elements of int[], you may overflow, especially if you have a lot of elements in those arrays.  You may want to use long accumulators.

Algorithmic Improvement

While the above optimizations will speed up the code, the algorithm is still \$O(N^2)\$.  I believe you can improve this to \$O(N)\$.

The problem is you are doing a whole lot of additions, and the partial results of those additions aren't changing.  Given an array of one million entries, when you are deleting the element around index 500,000 or so, the sums of the first 200,000 even indices and the first 200,000 odd indices don't change.

Consider the array:

1, 3, 5, 7, 9, 11, 13, 15, 17, 19, ...

If you were asked to find the sum of various spans of elements, say 4rd through 8th, how would you go about doing it?  How about 2nd through 9th? Can you save any work?

If you maintained a list of cumulative sums:

1, 4, 9, 16, 25, 36, 49, 64, 81, 100, ...

The sum of the 4rd through 8th element would be:

                 7 + 9 + 11 + 13 + 15
= (1 + 3 + 5) + (7 + 9 + 11 + 13 + 15) - (1 + 3 + 5)
= (1 + 3 + 5  +  7 + 9 + 11 + 13 + 15) - (1 + 3 + 5)
=                    64                -      9
=               cumulative[8]          -  cumulative[3]

We've preprocessed the input into a cumulative sum array, which is done once \$O(N)\$, and then any subrange sum can be computed in \$O(1)\$ by a simple subtraction.

You've got some issues to work out, such as how and where to store cumulative sums of only the even/odd indices, how to compute the sum of all even indices after an element at index x is removed, etc. But given this is a programming challenge, the implementation is left to student.


Check whether all textboxes are filled I'm doing some simple validations using jQuery to check if all textboxes are filled.

$(document).ready(function () {
$("#<%= btnInsertCountry.ClientID %>").click(function () {
    var codeValue = $("#<%= txtCountryCode.ClientID %>").val();
    var nameValue = $("#<%= txtCountryName.ClientID %>").val();

    if (codeValue == "" || nameValue == "") {
        if (codeValue == "") {
            $("#divCodeValidation").show();
        }
        else {
            $("#divCodeValidation").hide();
        }

        if (nameValue == "") {
            $("#divNameValidation").show();
        }
        else {
            $("#divNameValidation").hide();
        }

        return;
    }
});
});

The divs have the property display: none and contains the error message. This code works but I don't like it because I use an if to check if the values are empty and inside I have an if for each variable.

Is there a better way to do this validation?

The two validations are independent.  There's no point to if (codeValue == "" || nameValue == "") { … }.

Instead of calling $.show() and $.hide(), use $.toggle(display):

  display
  Type: Boolean
  Use true to show the element or false to hide it.

You can also write $(function() { … }) instead of $(document).ready(function() { … }).

$(function () {
    $("#<%= btnInsertCountry.ClientID %>").click(function () {
        $("#divCodeValidation").toggle(
            "" == $("#<%= txtCountryCode.ClientID %>").val()
        );
        $("#divNameValidation").toggle(
            "" == $("#<%= txtCountryName.ClientID %>").val()
        );
    });
});


Python 3.6+ function to ask for a multiple-choice answer # Standard multi choice question template
def multiChoiceQuestion(options: list):
    while True:
        print("\nEnter the number of your choice - ")
        for x in range(len(options)):
            print(str((x + 1)) + ". " + options[x])
        print("\n")
        try:
            answer = int(input())
        except ValueError:
            print("Doesn't seem like a number! Try again!")
            continue
        if answer < 1 or answer > len(options):
            print("That option does not exist! Try again!")
            continue
        return answer

I created a template to ask a multi choice question in python. The loop will never reach it's end, since there is always a continue or a return statement. Is the while True condition appropriate for it?

The while True is fine, and is probably the best way to do it. However, the rest of the flow control is a bit clumsy.  By rearranging a few statements, you can eliminate the continues.

PEP 8, the official Python style guide, recommends lowercase_with_underscores for function names unless you have a good reason to deviate.

The loop to print the numbered menu would be better written using enumerate().  Also, Python supports double-ended comparisons for validating that the answer is in range.

def multi_choice_question(options: list):
    while True:
        print("\nEnter the number of your choice - ")
        for i, option in enumerate(options, 1):
            print(f'{i}. {option}')
        print("\n")
        try:
            answer = int(input())
            if 1 <= answer <= len(options):
                 return answer
            print("That option does not exist! Try again!")
        except ValueError:
            print("Doesn't seem like a number! Try again!")


Protecting a database from bad data I'm just getting into SQL injection and data sanitization and seeking some advice on my script to get started. I have made this simple program which allows the user to enter their name into a form and the information gets saved into the database. There is also a button that lists all the current names in the database.

<?php
require 'db.php';

//If the "name" form was submitted, enter it into the database.
if (isset($_POST['submit'])) {
    $name = $_POST['name'];
    echo $name . ', you submitted it!';

    $con->query("INSERT INTO name (name) VALUES ('$name')");

}

//Delete name from database.
if (isset($_POST['del_submit'])) {
    $con->query("DELETE FROM name WHERE name='$del'");
    echo $del . ' deleted successfully!';
}

?>

<form id="devvo" method="post">

Enter your name:

<input type="text" name="name" />

<input type="submit" value="Submit" />

<input type="hidden" name="submit" />

</form>

<form id="list" method="post">
    <input type="submit" value="List Everyone...">
    <input type="hidden" name="list" />

</form>

<?php

//If the "list" form was submitted, get all names from database and display them to the user.
if (isset($_POST['list'])) {
    $result = $con->query("SELECT * FROM name");

    while ($row = $result->fetch_assoc()) {
        $del = $row['name'];

        echo 'Name:
';
        echo $row['name'];
        echo '<form id="del" method="post"><input type="submit" name="del_submit" value="X" /><input type="hidden" name="del" value="'.$del.'" /></form>';
        echo '
';
    }
}
?>

My knowledge of PHP is very limited (as you can tell from the program) but I'm definitely seeking some best practices and simple ways to improve the security of a simple program such as this. Feel free to tell me all the different ways this program could be exploited!

A few quick things:

$del is never defined when you delete based on it
Use prepared statements

Not technically necessary here in your script (depending on $del)

a good habit to have even when data is safe
If you remove the filter later, then the escaping will already be there

Don't silently change user's input

If the user provides invalid input, tell him; don't silently change it.
Not a hard rule by any means, but if a user provides a name and you change it for him, that might be a bit confusing.

Strive for valid HTML

Probably just meant to be a toy-esque script, but it's CodeReview, so might as well be pedantic :-).
You do have a stray
 though after the list input

It's a good habit to separate business logic and presentation

MVC is basically what I'm hinting at, but there's no need to go all the way
New data should never be fetched or generated inside of HTML
The best description of what I'm getting at that I've ever heard is probably (summarized):

If you can't reskin your site without repeating PHP code other than simple output, something is wrong.

A bit hard to explain if you've never seen it, so if you want, I can do a little example.

Escape HTML

Any time you put a variable into html, it should be escaped with htmlspecialchars or htmlentities.
echo '
' . htmlspecialchars($row['name']) . '	';

Just because one input is set does not mean that another is

$_POST['submit'] and $_POST['name'] are not magically linked
A user could exploit this to cause a notice
Never directly access anything a user can control the existence or content of

Use filter_input like showerhead suggested
Or, if you want to do it more directly:

$name = (isset($_POST['name']) && is_string($_POST['name'])) ? $_POST['name'] : null;
Note: everything comes in as either a string or an array even if it's an integer.
$id = (isset($_POST['id']) && is_string($_POST['id'])) ? (int) $_POST['id'] : null;'

(Yes, I am 100% crazy-level paranoid)

Edit: This is a response to the comment below

  if you could elaborate on a couple of your points that would be great.
  Particularly: "Just because one input is set does not mean that another is $_POST['submit'] and $_POST['name'] are not magically linked A user could exploit this to cause a notice"

Consider this code:

if (isset($_POST['submit'])) {
    $name = $_POST['name'];
    ...
}

As discussed above, you should never access a key that your not sure exists.  I'm assuming the logic here is, "well if the form was submitted, then the name field must exist."

That's not the case though.  Consider this HTML:

<form action="http://yoursite.tld/yourpage.php" method="POST">
    <input type="submit" name="submit" value="Submit">
</form>

Now imagine that someone submits this form.

Reading $_POST['name'] will trigger an "undefined index" notice.

So, in short, what I was getting at is that there is no link between the submit and name indexes.  It's completely possible for one to be set and not the other.

Would this actually ever matter?  Probably not.  But still, it can have bad effects like a malicious user seeing file path or trying to fill up a log file.

The code should look like this:

if (isset($_POST['submit'])) {
    $name = (isset($_POST['name']) && is_string($_POST['name'])) ? $_POST['name'] : null;
    ...
}

(Or you could use filter_input.)

  And: The $del issue. The script works great even though you say it isn't defined?

The first time $del appears is on line 15 ($db->query("DELETE FROM name WHERE name='$del'");).  This means that it is either defined in db.php, or it's not defined.

If it's defined in db.php, it shouldn't be.

If it's not defined, that means that it's issuing a notice and being treated as null.

(There's a third, very unlikely possibility that register globals is enabled.  If that abomination is enabled, immediately disable it, or if your hosting company has enabled it without you requesting it, promptly switch companies.)

When null is casted to a string, it becomes an empty string, so the query is probably running like:

$db->query("DELETE FROM name WHERE name=''");

Also, this query falls under my "Use prepared statements" point above.

Imagine if someone submits the following form:

<form action="http://yoursite.tld/page.php" method="POST">
    <input type="submit" name="del_submit" value="Delete">
    <input type="hidden" name="del" value="' OR 1">
</form>

That means that this query would be executed:

DELETE FROM name WHERE name='' OR 1

1 is always true, thus the entire table would be cleared.


Pausing a loop with a Raspberry Pi 3 I am trying to perform a loop and then stop the loop when the last line of code in the if statement is printed to screen saving on CPU cycles.

I want to start a while loop and then kill the loop after it is complete, but not breaking out of the loop so I can re-initiate the loop once again by pressing a GPIO button "B1".

My end goal is to have the RGB LED cycle and not continue to loop once it is complete.

Setup: RPi 3, RGB LED, button and breadboard.

#include <stdio.h>
#include <string.h>
#include <sys/types.h>
#include <bcm2835.h>

#define LED_RED RPI_GPIO_P1_12 //GPIO 18
#define LED_GRN RPI_GPIO_P1_16 //GPIO 23
#define LED_BLU RPI_GPIO_P1_18 //GPIO 24
#define B1 RPI_GPIO_P1_22 //GPIO 25

int main(int argc, char **argv)
{
    if (!bcm2835_init()) return 1;
    bcm2835_gpio_fsel(LED_RED, BCM2835_GPIO_FSEL_OUTP);
    bcm2835_gpio_fsel(LED_GRN, BCM2835_GPIO_FSEL_OUTP);
    bcm2835_gpio_fsel(LED_BLU, BCM2835_GPIO_FSEL_OUTP);
    bcm2835_gpio_fsel(B1, BCM2835_GPIO_FSEL_INPT);
    bcm2835_gpio_set_pud(B1, BCM2835_GPIO_PUD_UP);

    bcm2835_gpio_len(B1);

    unsigned int delay = 1000;
    while (1){
        printf("Light cycle machine\n");

        if(bcm2835_gpio_eds(B1)!=0){

            printf("Button Pressed!!!\n");
            printf("\n");
            bcm2835_gpio_set(LED_RED);
            printf("Red LED on!!!\n");
            bcm2835_delay(delay);
            bcm2835_gpio_clr(LED_RED);
            printf("Red LED off!!!\n");
            bcm2835_gpio_set(LED_GRN);
            printf("Green LED on!!!\n");
            bcm2835_delay(delay);
            bcm2835_gpio_clr(LED_GRN);
            printf("Green LED off!!!\n");
            bcm2835_gpio_set(LED_BLU);
            printf("Blue LED on!!!\n");
            bcm2835_delay(delay);
            bcm2835_gpio_clr(LED_BLU);
            printf("Blue LED off!!!\n");
            delay(delay);
            bcm2835_gpio_set_eds(B1);
            printf("\n");
            printf("System Idle\n");
            }

        else{

            delay(delay);
            //printf("Done!\n");

            }
        if (bcm2835_gpio_eds(B1)!=1)
            {
            }
        else{

            }

}
    return 0;
}

Naming

Keep your #define constant names consistent. You specify that LED_RED, LED_GRN, and LED_BLU are LEDs, but you don't specify what B1 is. A better name would be BTN_B1.

Bracing

Keep your bracing consistent. Stick to one indentation style. For example in your code, you have both

if(...){
        ...
        }

and also

if (...)
    {
    ...
    }

Delay Variable

Use the const keyword if you're not going to be changing delay.

Main Args

If you're not going to be using int argc, char **argv, then they don't need to be there.

Use Functions

The first 7 lines of main are setting up the GPIO pins, you should put that into a setup() function.

Make good use of whitespace

The long block of statements inside the if(bcm2835_gpio_eds(B1)!=0) would be better off separated by some spaces, to make it more readable.

Resulting Code

#include <stdio.h>
#include <string.h>
#include <sys/types.h>
#include <bcm2835.h>

#define LED_RED RPI_GPIO_P1_12 //GPIO 18
#define LED_GRN RPI_GPIO_P1_16 //GPIO 23
#define LED_BLU RPI_GPIO_P1_18 //GPIO 24
#define BTN_B1 RPI_GPIO_P1_22 //GPIO 25

void setup_gpio(void);

int main()
{
    if (!bcm2835_init()) return 1;
    setup_gpio();

    unsigned int delay = 1000;
    while (1) {
        printf("Light cycle machine\n");

        if (bcm2835_gpio_eds(BTN_B1)!=0) {
            printf("Button Pressed!!!\n\n");

            bcm2835_gpio_set(LED_RED);
            printf("Red LED on!!!\n");
            bcm2835_delay(delay);
            bcm2835_gpio_clr(LED_RED);
            printf("Red LED off!!!\n");

            bcm2835_gpio_set(LED_GRN);
            printf("Green LED on!!!\n");
            bcm2835_delay(delay);
            bcm2835_gpio_clr(LED_GRN);
            printf("Green LED off!!!\n");

            bcm2835_gpio_set(LED_BLU);
            printf("Blue LED on!!!\n");
            bcm2835_delay(delay);
            bcm2835_gpio_clr(LED_BLU);
            printf("Blue LED off!!!\n");

            delay(delay);
            bcm2835_gpio_set_eds(BTN_B1);
            printf("\nSystem Idle\n");
            }
        else {
            delay(delay);
            //printf("Done!\n");
            }
        }

    return 0;
}

void setup_gpio()
{
    bcm2835_gpio_fsel(LED_RED, BCM2835_GPIO_FSEL_OUTP);
    bcm2835_gpio_fsel(LED_GRN, BCM2835_GPIO_FSEL_OUTP);
    bcm2835_gpio_fsel(LED_BLU, BCM2835_GPIO_FSEL_OUTP);
    bcm2835_gpio_fsel(BTN_B1, BCM2835_GPIO_FSEL_INPT);

    bcm2835_gpio_set_pud(BTN_B1, BCM2835_GPIO_PUD_UP);
    bcm2835_gpio_len(BTN_B1);
}

As for your question, you say you want to kill the loop but not break out of it. I'm not exactly sure what you mean. But, perhaps you're looking for interrupts?


Implementing blockchain block in C++11 I want to rewrite blockchain source code from Golang to C++11. Please, review my code and help me to improve it.

Source in Golang:

package main

import "time"

type Block struct {
  Timestamp     int64
  Data          []byte
  PrevBlockHash []byte
  Hash          []byte
  Nonce         int
}

func NewBlock(data string, prevBlockHash []byte) *Block {
  block := &Block{ time.Now().Unix(), []byte(data), prevBlockHash, []byte{}, 0}

  proof := NewProofOfWork(block)
  nonce, hash := proof.Run()

  block.Hash = hash[:]
  block.Nonce = nonce

  return block
}

func NewGenesisBlock() *Block {
  return NewBlock("Genesis Block", []byte{})
}

My source in C++:

blockchain.h:

#ifndef BLOCKCHAIN_H
#define BLOCKCHAIN_H

#ifdef USE_BLOCK

#include <cstdint>
#include <vector>
#include <array>
#include <string>

using sha_t  = std::array<unsigned char, 32>;
using data_t = std::vector<unsigned char>;

struct Block
{
    uint64_t timestamp;
    data_t   data;
    sha_t    prevBlockHash;
    sha_t    hash;
    int      nonce;
};

Block make_block(std::string const& data,
                 sha_t const& prevBlockHash);
Block make_genesis_block();

#endif

#endif

block.cpp:

#define USE_BLOCK

#include "blockchain.h"

#include <chrono>

Block make_block(std::string const& data,
                 sha_t const& prevBlockHash)
{
    using namespace std::chrono;

    milliseconds ms = duration_cast<milliseconds>(system_clock::now().time_since_epoch());
    uint64_t timestamp = ms.count();
    data_t d(data.begin(), data.end());
    Block block{timestamp, d, prevBlockHash, sha_t(), 0};

    return block;
}

Block make_genesis_block()
{
    return make_block("Genesis Block", sha_t());
}

My questions:

How to improve code structure and architecture?
How to improve performance? Use constexpr or something else?
How to store the data that all are byte arrays, but some of arrays have fixed length (such as SHA256 hashes), and some have dynamic size (such as data)?
How to pass empty byte array? sha_t() is ok?
How to deal with time and make code short and fast?
What SHA256 library should I use?

How to improve code structure and architecture?

Three suggestions...

1. Recognize your code's logical concepts and do so as idiomatically as possible.

For example, the make_block and make_genesis_block seem to both be making a block. Here making-the-block is the concept.

You can more idiomatically recognize this make block concept via parametric polymorphism. 200_success suggests this via renaming make_genesis_block so it has the same name as the other block making function: make_block. And then having the genesis making block function take no arguments.

Now you have two make_block functions which the compiler distinguishes by the parameters (ADL):

Block make_block();
Block make_block(std::string const& data, sha_t const& prevBlockHash);

2. Recognize your code's structural invariants, and then enforce those using encapsulation.

Take the Block class for instance. I'd guess that the hash field is dependent on the data field. If correct, then make these inter-dependent fields private. Provide accessor methods for these values and a mutator method for the data field which updates the hash.

If nothing else depends on the data or hash fields, then I'd suggest making this tuple of info its own class and put the encapsulation within it. Then compositionally incorporate this new class into your block class. This way Block can still be a struct with its members publicly accessible.

Use Non-Member functions unless the function needs direct access to the representation of a class (C.4). For my favorite explanation of what I'm suggesting here, please see Scott Meyer's article: How Non-Member Functions Improve Encapsulation.

3. Prefer value semantics over reference semantics.

You haven't shown any code which exemplifies reference semantics but for the sake of C++ developers everywhere (or at least for my preference), please keep with value semantics as long as you can: don't expose to your API any classes that inherit from other class (see inheritance is the base class of evil).

How to improve performance? Use constexpr or something else?

Setup a benchmarking system for your code for the platforms you need the performance on.
Make sure to compile your code with the highest available optimization settings turned.
If your compiler will take profiling data for optimizing code, give it that too.
Understand the difference between return value optimization (RVO) and named return value optimization (NRVO).
Understand how RVO and NRVO fits in with copy elision and when you're guaranteed copy elision (not until C++17).

As an example of NRVO vs. RVO, your first make_block definition does:

Block block{timestamp, d, prevBlockHash, sha_t(), 0};
return block;

Here block is a named return value. This can be rewritten instead as:

return Block{timestamp, d, prevBlockHash, sha_t(), 0};

Which is an anonymous/unnamed return value.

While your question is tagged C++11, I'll opine that it's a nice practice still to be aware of the newer standards and that with C++17 you're guaranteed copy elision. I suspect that compilers of earlier standard implementation are also likely more able to support copy elision when using unnamed return values. Copy elision tends to help with performance.

Moreover, returning output through return values encourages value semantics, aids with constness, and I find it makes it easier for people to reason about the code.

As to constexpr, I haven't seen it increase performance much of code I've used it with. OTOH, I love what recognizing code as constexpr means — computable at compile time — and I agree with Scott Meyers recommendation: use constexpr whenever possible (from Item 15 of his Effective Modern C++ book). A related nicety IMO is that types having constexpr constructors can then satisfy the LiteralType concept.

How to store the data that all are byte arrays, but some of arrays have fixed length (such as SHA256 hashes), and some have dynamic size (such as data)?

I like how you've already done this — you're using std::array for the fixed length types, and std::vector for the dynamic types. It's a preferable starting point so long as these types' access don't allow your data invariants to be violated.

Prior to C++17, you may just want to update the type you're using in the std::array and std::vector containers to std::uint8_t. Since C++17, we can use std::byte for this type as one of the commentators suggests.

Deeper in, you may need to answer whether speed is more important or data correctness is. A structure of arrays (SOA) often lends itself better to speed, while an array of structures (AOS) can lend itself better IMO to correctness.

How to pass empty byte array? sha_t() is ok?

It's not clear what you're asking with this question. Sounds like you may want to read about value initialization (introduced in C++03).

How to deal with time and make code short and fast?

Seems like this question is asking again about how to improve performance. If so, please see my response in that section.

What SHA256 library should I use?

The code from the most recent OpenSSL library is what I'd go with just simply because I suspect it's had the most eyes on it. If your code is license compatible, you may find it easier to just copy the OpenSSL code for SHA256 into your code instead of trying to build the entire OpenSSL library and then linking with it.


Changing visibility of ToolbarItem - Xamarin.Forms I want to fill collection of ToolbarItems from XAML, but change visibility of some ToolbarItem from view model.

I implemented BasePage with some wrapper around ToolbarItems collection:

public class BasePage : ContentPage
{
    public IList<CustomToolbarItem> CustomToolbar { get; private set; }

    public BasePage()
    {
        var items = new ObservableCollection<CustomToolbarItem>();
        items.CollectionChanged += ToolbarItemsChanged;

        CustomToolbar = items;
    }

    private void ToolbarItemsChanged(object sender, System.Collections.Specialized.NotifyCollectionChangedEventArgs e)
    {
        ToolbarItems.Clear();

        foreach (var item in CustomToolbar)
        {
            item.PropertyChanged += OnToolbarItemPropertyChanged;
            if (item.IsVisible)
            {
                ToolbarItems.Add(item);
            }
        }
    }

    private void OnToolbarItemPropertyChanged(object sender, PropertyChangedEventArgs e)
    {
        if (e.PropertyName == CustomToolbarItem.IsVisibleProperty.PropertyName)
        {
            UpdateToolbar();
        }
    }

    private void UpdateToolbar()
    {
        foreach (var item in CustomToolbar)
        {
            if (item.IsVisible)
            {
                ToolbarItems.Add(item);
            }
            else
            {
                ToolbarItems.Remove(item);
            }
        }
    }

    protected override void OnDisappearing()
    {
        base.OnDisappearing();

        ToolbarItems.Clear();
        CustomToolbar.Clear();
        foreach (var item in CustomToolbar)
        {
            item.PropertyChanged -= OnToolbarItemPropertyChanged;
        }
    }
}

CustomToolbarItem.cs:

public class CustomToolbarItem : ToolbarItem
{
    public static readonly BindableProperty IsVisibleProperty =
            BindableProperty.Create(nameof(IsVisible),
                                    typeof(bool),
                                    typeof(CustomToolbarItem),
                                    true);
    public bool IsVisible
    {
        get { return (bool)GetValue(IsVisibleProperty); }
        set { SetValue(IsVisibleProperty, value); }
    }
}

How I use it in XAML:

<local:BasePage x:Name="Page"
            xmlns="http://xamarin.com/schemas/2014/forms"
            xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml"
            xmlns:local="clr-namespace:ToolbarExtensionSample;assembly=ToolbarExtensionSample"
            x:Class="ToolbarExtensionSample.SubPage">

  <local:BasePage.CustomToolbar>
    <local:CustomToolbarItem Name="Alloha"
                             IsVisible="{Binding IsItemVisible, Source={x:Reference Page}}"/>
  </local:BasePage.CustomToolbar>

  <Button Text="Click!"
          Clicked="OnButtonClicked"/>
</local:BasePage>

My code is working, but I'm wondering is it okay for performance? Maybe, exists some better solution.
Thanks in advance for suggestions to improvements or better approaches.

Two things:

You clear ToolbarItems and CustomToolbar when page dissapearing, this causes problem: when you show next page and then go back, you won't see your items anymore, because they were cleared.
Handlers OnToolbarItemPropertyChanged will not be removed in foreach cycle, because you clear list before this cycle.


is_palindrome function that ignores whitespace and punctuation from string import punctuation, ascii_lowercase

def is_palindrome(string):
    """Return True if string is a palindrome,
    ignoring whitespaces and punctuation.
    """
    new = ""
    for char in string:
        lc = char.lower()
        for x in lc:
            if x in ascii_lowercase:
                new += x

    return new[::-1] == new

# string = "Was it a rat I saw?"
# print(is_palindrome(string))

You are unnecessarily importing punctuation
Avoid reassigning module names such as string in your code
A list comprehension would be a simpler way of filtering out the data you don't want

from string import ascii_letters

def is_palindrome(candidate):
    """
    Returns True if candidate is a palindrome, ignoring whitespaces and punctuation.
    """
    stripped = [char.lower() for char in candidate if char in ascii_letters]
    return stripped == stripped[::-1]


More elegant way to show | hide in jQuery What is the best way to optimize this jQuery code?

$('#hde_test').hide();

$('#shw_test').click(function() {
    $('#shw_test').hide();
    $('#hde_test').show();
    $('.class1').show();
});
$('#hde_test').click(function() {
    $('#shw_test').show();
    $('#hde_test').hide();
    $('.class1').hide();
});

You can shorten it down:

$('#shw_test').click(function() {
    $('#hde_test, #shw_test').toggle();
    $('.class1').show();
});
$('#hde_test').click(function() {
    $('#hde_test, #shw_test').toggle();
    $('.class1').hide();
});

Likely, you can shorten it to:

$('#shw_test, #hde_test').click(function() {
    $('#hde_test, #shw_test, .class1').toggle();
});

Just depends how the initial state of things are.


Class for measuring distance as feet and inches My homework question states:

  Develop a class to measure distance as feet (should be int), inches (should
  be float).  Include member functions to set and get attributes.  Include
  constructors.  Develop functions to add two distances.

  The prototype of the sum function is:

Distance Sum(Distance d);

Please check out my coding style and other aspects that can make this program better.

#include <iostream>
using namespace std;
class Distance
{
        private:
                int feet;
                float inch;
        public:
        Distance();
        Distance(int a,float b);
        void setDistance();
        int getFeet();
        float getInch();
        void distanceSum(Distance d);
};
int main()
{
        Distance D1,D2;
        D1.setDistance();
        D2.setDistance();
        D1.distanceSum(D2);
        return 0;
}
/*Function Definitions*/
Distance::Distance()
{
        inch=feet=0;
}
Distance::Distance(int a,float b)
{
        feet=a;
        inch=b;
}
void Distance::setDistance()
{
        cout<<"Enter distance in feet";
        cin>>feet;
        cout<<endl<<"Enter inches:";
        cin>>inch;
}
int Distance::getFeet()
{
        return feet;
}
float Distance::getInch()
{
        return inch;
}
void Distance::distanceSum(Distance d)
{
        cout<<"feet="<<d.feet+feet<<endl;
        cout<<"inches="<<d.inch+inch<<endl;
}

When designing a class for your program, whether it's for homework or a real production application, you want to always consider how that class is going to be use and what its responsibilities should be. Each function and method should do one thing/task and it should be reflected by the method's name. Additionally, you want to keep to a consistent easy-to-read coding style when you actually start writing the code. With those two points in mind here's some things to consider in your code:

Distance::distanceSum is doing two tasks here.

It's not only summing feet and inches but it's printing them out as well.

Unnecessary type conversion.

In your default Distance() ctor, there's an implicit conversion because you're assigning 0 to a float type. You should have gotten a warning from your compiler. Consider using an initializer list like so:

Distance::Distance() : feet(0), inch(0.0)
{
}

Code doesn't take advantage of const correctness.

Which parameters aren't suppose to change? Which methods will be modifying your Distance class? For example, your Distance::Distance(int a,float b) isn't changing a or b. Have the compiler enforce that promise by using const:

Distance::Distance(const int a, const float b)

Similiarly:

void Distance::distanceSum(const Distance &d);

Inconsistent indentation and spacing.

Consider indenting the methods under public: the same way you did with private:. Add spaces to your assignments to help readability. eg. feet = a;

No module separation by file.

class Distance should probably be in a separate header/implementation file rather than putting everything in one main file.

Ninja comments.

Comments? What comments? Exactly. Consider adding a block comment at the top of your Distance class that explains the purpose for its existence. The block comment should answer questions like how is this class suppose to be used and what details is it abstracting away? Adding a comment to clarify how the feet and inch data members are going to be used. For example, it's not clear if your distance class is maintaining the same distance measurement but with different units or it's really meant to be used as one whole unit. eg. 6 feet 2 inches or 6 feet 72 inches?

With the above considerations, here's one way I would refactor your code:

In distance.h header file:

#ifndef DISTANCE_H
#define DISTANCE_H
class Distance
{
    private:
       // feet and inch is one unit.
       // invariant: inch_ < 12.
       int   feet_;
       float inch_;
    public:
       Distance(const int feet = 0, const float inches = 0.0);
       void      setDistance(const int feet, const float inches = 0.0);
       int       getFeet() const;
       float     getInch() const;

       // returns this instance. Permits method chaining for Distance class.
       Distance& Add(const Distance &d);
};
#endif

In distance.cpp implementation:

#include "distance.h"
Distance::Distance(const int feet, const float inches)
         : feet_(feet + inches / 12), inch_(inches % 12)
{
}

void Distance::setDistance(const int feet, const float inches)
{
    feet_ = feet + inches / 12;
    inch_ = inches % 12;
}

int Distance::getFeet() const
{
    return feet_;
}

float Distance::getInch() const
{
    return inch_;
}

Distance& Distance::Add(const Distance &d)
{
    setDistance(getFeet() + d.getFeet(), getInch() + d.getInch());

    return *this;
}

Here are the major changes above:

Distance no longer uses cin/cout for explicit IO. You can push that code into main.
class definition and implementation are now in their respectively named files.
Removed an extra constructor definition by taking advantage of default parameters.
feet and inch data members are used together to represent the measurement in distance. inch cannot be > 12 because that would mean there's enough for a foot. We enforce this by dividing feet and inches by 12 when setting distance's data.
const is used to clearly indicate what can and can't change the distance object.
Changed distanceSum to Add to better reflect what it's doing. Notice that Add is implemented only through Distance's public methods -- it does not manipulate feet_ and inch_ directly.


Python Combinatory Algorithm - A Binomial Coefficient application with n mutable and k fixed to 2 This question came from a real use case.
I had a data frame with different columns each one containing data from a data source and I wanted to design a hypothesis test to prove or not if the data had the same mean.
So I had to compute the Kolmogorov-Smirnov test for each couple of columns.
Now the problem can be generalized to any combinatory task.
It follows that I had to implement a Binomial Coefficient like
$$ \binom{n}{k} $$
Where n is the number of columns
and k is = 2
My question is: if exists a more efficient way to apply a function on permutated samples taken from a list? And how to do this permutation  eg. Given a func and a list [a,b,c,d]
func(a,b)
func(a,c)
func(a,d)
func(b,c)
func(b,d)
func(c,d)

I created an algorithm to solve this issue, but I am wondering if there is a better way to do that in Python.
In my algorithm, I simply multiply each n element in an explanatory array with another element i of the same array, with n!=i, instead of computing the statistical test.
to_do=[1,2,3,4,5]
#done will store information on the elements already combined
done=[]
#j will store information on the results of the combination
j=[]

#iterating over the to_do array
for n in to_do:

  #checking if we already computed the n element
  if n not in done:
    print(n)
    #taking another i element from the array
    #where n!=i
    for i in to_do:
      print(i)
      #if the condition is satisfied
      if i!=n:
        #combine the two elements
        m=n*i

        #append the result on the "j" array
        j.append(m)
    #updating the array with the "done" elements
    done.append(n)

print(len(done))
print(len(j))

If you just want the length of the list, your code can be reduced to one line:
result = len(to_do) * (len(to_do) - 1)

Also, you're comments are, at the very least, excessive. You should really only use comments to explain a design choice or a complicated equation/algorithm. Comments like #if the condition is satisfied just clutters your code.
As explained in a comment, actual computations need to be made. This can be done using itertools.permutations.
import itertools
from typing import List

def func(nums: List[int]) -> List[int]:

    return list(set(
        x * y for x, y in itertools.permutations(nums, 2)
    ))

While this doesn't avoid the extra computations, it's a short and sweet solution. And below is how you would use it:
to_do = [1, 2, 3, 4, 5]
func(to_do)


Toggling button & content with jQuery This hides div.extended then creates a button that toggles its text and div.extended's visibility.

While the below works, I was wondering if there was a more concise way of writing this code.

(function() {

var extended = $('.extended').hide();

$('<button></button>', {
    text: 'Read more'
    }).appendTo('.intro')
        .on('click', function(){
            extended.slideToggle();
        }).toggle(
            function(){
                $(this).text('Read Less')
            },
            function(){
                $(this).text('Read More')
            });

})()

$(function () {
    var extended = $('.extended').hide(),
        hidden = true;
    $('<button></button>', {
        text: 'Read more'
    }).appendTo('.intro').on('click', function () {
        extended.slideToggle();
        hidden = !hidden;
        $(this).text(hidden ? 'Read More' : 'Read Less');
    });
});

You only need to bind the click event once (toggle is a shortcut for binding to click and attaching two alternating event handlers).


toString() in a model or view layer? I would like to ask those of you, who are a little bit more expirienced in development. I would like to ask about toString() method usage. In what situations do you use it?

I've found some info about it, for example here, but it's not what I'm looking for.

Is there any convension which says when should I use it for my custom objects? In model layer or in view? Let me show a simple example...

I've got a class called "Component". Component has some name, values etc... One of the fields can be a perfect information for the user about object's state. The rest is also important, but not nessesary to display on the screen (in a view layer). Moreover, this field is a String and should be translated depending on the language settings. It's also required to present this field as an upperCase().

So, the question is:
Should I put all the logic connected with forming a "user-friendly" string (locals, upperCase...) in a toString() method? Or maybe I should create a getUserFriendlyName() method instead and use toString(), for instance, for debugging (so in model). So, should toString() method have any influence on what the end user can see on the screen?

    @Override
    public String toString() {
        String toReturn;
        try {
            toReturn = language.getString("LOCAL_STRING_KEY_" + this.compInfo);
        } catch (MissingResourceException e) {
            logger.warning("No key found for " + compInfo);
            toReturn = this.compInfo;
        }
        return toReturn;
    }

Is there any convension which says when should I use it for my custom objects? In model layer or in view?

If your question is 'When should I override the default toString() method and use create my own?' then there is one set of answers. If your question is 'when should I call the toString() method (implicitly like "This is my object: " + myobj or explicitly)?' then there is a different set of answers.

So, when should you override the toString()? From the Javadoc for Object:

  In general, the toString method returns a string that "textually represents" this object. The result should be a concise but informative representation that is easy for a person to read. It is recommended that all subclasses override this method.

I strongly agree with this statement. Every custom class you create should override the toString method, and should find a way to textually represent the class content in a meaningful (but not necessarily exhaustive) way. For example, a 'Point' class representing a co-ordinate x and y where x==3 and y==4 should return something like Point(3,4). On the other hand, I believe that some classes are over-exhaustive with toString, like ArrayList does too much. I would prefer ArrayList(size 10: [sample0 .... sample9]). Edit: The Java tutorial for Object also says the same thing about toString():

  The Object's toString() method returns a String representation of the object, which is very useful for debugging

The reasons for making every class have a toString is because of the times you should use the toString() method.... and, as far as I am concerned, there are only three places in general programming where using toString() is legitimate (and two 'special' cases):

In exception handling: throw new IllegalStateException("Failed to process " + value);
for logging program progress using things like log4j.
when the JavaDoc for the toString() Method for the class you are using defines a specific, and non-alterable format for the result. e.g. String.toString() or Integer.toString(). If the class you are using does not specify what the toString() returns, and does not guarantee that it won't change, then create your own way to present the data.

There are two special cases for using toString():

I think, even though this is a special case, that this is the most important reason: when running your Java program through a debugger like the one built in to Eclipse. When you are stepping through your program the debugger will use the toString() method to show you the state of all your variables. This is where, in many cases, you will value the toString() more than anything else.
when developing or debugging a program it is often convenient for you to add 'println' statements, and they can be things like: System.out.println("Processing customer " + customer);`

In summary, the toString() method should produce enough data to debug problems with the object content. It is used occasionally for a 'contractual' representation of a value (like String.toString()).

But, in general, toString() Is a tool for the PROGRAMMER, not for the user. It is there to assist you when things go wrong, not when they go right.

In my real Job I file bugs against code (and pursue the bugs relentlessly) which throws exceptions where the exception has an Object.toString() in the message. I also mentally +1 people who's programs produce good detail during exceptional conditions.

Now, in your particular situation, I think that you should use the toString() method to represent the state of your Customer, and, because the display-conditions for your customer are different to the internal state (e.g. display needs to be UPPERCASE,) I think that you should have a different mechanism for displaying the Customer than the toString() method.

In other words, the information you will need to debug a situation involving a Customer (something printed in a stack trace or debug session) is different than the information you want to display on the screen.